From patchwork Wed Apr 26 13:14:37 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mirsad Todorovac X-Patchwork-Id: 87868 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp248343vqo; Wed, 26 Apr 2023 06:36:57 -0700 (PDT) X-Google-Smtp-Source: AKy350YuDT7/iJxhZ0J2zTaszNhn7Kz41+3veNUE1ESXMym50G9rBuidL08I5UwvLFTn79ByXWEO X-Received: by 2002:a17:903:41c2:b0:1a9:7033:443f with SMTP id u2-20020a17090341c200b001a97033443fmr14361274ple.14.1682516217254; Wed, 26 Apr 2023 06:36:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682516217; cv=none; d=google.com; s=arc-20160816; b=UkNwSoT7n6u9IXbLJdGZYUFTqexpTbUjZahkrN2ureJyxiSrAtQ8QUEqsspcblojvr z9+bRtPofglrfJFPmh4enGYHHMmLYczA8EZvdZnetcoOr1n+Fvmg8wj48jc5NmQi8Q4k slnHyTvi9H3XdMUHqcaZ6xldaX5w+ioOi8SCLo76OpHSRFTMN4xz6gKfsY9B5SqOtJKe ouPsoZLRoHa69gqu/CZXxKDPFbar4A4sxXXJiPChXv76onAtKA+CsFWEnoBJ0z7/HohN qF7V8eNk3L/qJyxC3TH8jn/dOzvoJ7rZqXMKxnXRW8LwW8XI4BJz509WaXo009cqx/gl QAeQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature:dkim-signature; bh=LeeQrNHBmoh5yX/iqrDhJJJcS5kS1lreqR182HV857Y=; b=cqJLqv+pQ/wUzgaPcXZsiKx/0qsOmhIWkSzhXB7uHq55DoSzeDCyC5jlSznmr5YDbA vlvZflV0sAxk70/mN3cYyVFZt/zmziuQaa0Ejh91bGkEPgajEIpRiE4bkqAXDWa9I1qu nb7PnFE/FwD69B+XD9GxMzixFU6VKb8wJaf8Mp9y1lXJaLr6eIAc+PYx2lOVzUzA06qu qohBV9vi7gjqkiFI9aTM8WNce3bSDsNwsyXaO/MXFbwp8NNuf6uXrMh/XDcSHbjWSk01 1A/m80dmtVKZZYGkyk2W4fWXlTCwoQL0RHiBrdQiAOhsjm8vIw6QSJjtx8Z42VglpbaL isWQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=haM6Jal+; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=Z+CG57hM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alu.unizg.hr Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id q7-20020a17090311c700b001a526bc2b84si16679331plh.620.2023.04.26.06.36.41; Wed, 26 Apr 2023 06:36:57 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=haM6Jal+; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=Z+CG57hM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alu.unizg.hr Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240949AbjDZNPT (ORCPT + 99 others); Wed, 26 Apr 2023 09:15:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48612 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240724AbjDZNPS (ORCPT ); Wed, 26 Apr 2023 09:15:18 -0400 Received: from domac.alu.hr (domac.alu.unizg.hr [IPv6:2001:b68:2:2800::3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B090E4C20; Wed, 26 Apr 2023 06:15:14 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by domac.alu.hr (Postfix) with ESMTP id C5D526017C; Wed, 26 Apr 2023 15:15:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682514912; bh=KPZQypbGgzI0Uu0XwSNF25wVht5AlW8b9I12H+fXD3k=; h=From:To:Cc:Subject:Date:From; b=haM6Jal+LvAX2qletPJuXWPCFKV1m0fxKwf6GFlaAj3K7BLvuVQ+oNhdZpbRKHTgj e+c1w4aI6nA1ufo6fAx5CE5HSsjvsly/1LDs6HWYBUO55q9uO5ynfzBgvP891ZUBfV uiQa4F23u0kEQTTrlYiAVhLoayMnG6cvaScbQejxXz4sSJ2N6OAbMGki0vyV9FYOY+ CnoduxQFiTbiNMMXhMhU8V+f+7aKU4jwMHcJFlAlNyyw2C2ou4R9Qcyuv/syhaR7C4 Tg+VDMHCpI6MFKoEWtmubZ7TiSb7Wnj5uj4ZE/fJxU9Zfk/zh4L0IgHsvGt9ohfuve gglubFsIIR89A== X-Virus-Scanned: Debian amavisd-new at domac.alu.hr Received: from domac.alu.hr ([127.0.0.1]) by localhost (domac.alu.hr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id MUxykxBY5qAx; Wed, 26 Apr 2023 15:15:09 +0200 (CEST) Received: by domac.alu.hr (Postfix, from userid 1014) id 186216017E; Wed, 26 Apr 2023 15:15:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682514909; bh=KPZQypbGgzI0Uu0XwSNF25wVht5AlW8b9I12H+fXD3k=; h=From:To:Cc:Subject:Date:From; b=Z+CG57hMuumbMi0/YDANdX8MtnK+tEt6GGXOzm0zVcrHbsD3jVi1HXm44I+e9N2nI dBRVnP1gWGT1yMU4igD5ptdAsI3XlhTi/S5VQjq1xeoK1ew0mSqldUESYs9jZIvk8o Nq4kZ7+LxJqz1jTZoERpQQxqOsyGBMpvONlvJRvOa0fTWkrIu3DaPezXIr40DJ7DgZ x3wBJPp/YK3qh7Di4sVp/a2Ypjj23Pf3YDPyYkOSheyDKFtyFBfC0Kb67ib9gNJkUU pjr94KSwAz6RG2n1WXO23WdX4by8txuJRponnTxwt6CDBtITXzlTgxnc7xHRl7H/oW Dxwaof12Z3c/w== From: Mirsad Goran Todorovac To: Mirsad Goran Todorovac , linux-kernel@vger.kernel.org Cc: Luis Chamberlain , Greg Kroah-Hartman , Russ Weight , Takashi Iwai , Tianfei Zhang , Shuah Khan , Colin Ian King , Randy Dunlap , linux-kselftest@vger.kernel.org, stable@vger.kernel.org, Dan Carpenter Subject: [PATCH v5 1/3] test_firmware: prevent race conditions by a correct implementation of locking Date: Wed, 26 Apr 2023 15:14:37 +0200 Message-Id: <20230426131438.16936-1-mirsad.todorovac@alu.unizg.hr> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764246124818923905?= X-GMAIL-MSGID: =?utf-8?q?1764246124818923905?= Dan Carpenter spotted a race condition in a couple of situations like these in the test_firmware driver: static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) { u8 val; int ret; ret = kstrtou8(buf, 10, &val); if (ret) return ret; mutex_lock(&test_fw_mutex); *(u8 *)cfg = val; mutex_unlock(&test_fw_mutex); /* Always return full write size even if we didn't consume all */ return size; } static ssize_t config_num_requests_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { int rc; mutex_lock(&test_fw_mutex); if (test_fw_config->reqs) { pr_err("Must call release_all_firmware prior to changing config\n"); rc = -EINVAL; mutex_unlock(&test_fw_mutex); goto out; } mutex_unlock(&test_fw_mutex); rc = test_dev_config_update_u8(buf, count, &test_fw_config->num_requests); out: return rc; } static ssize_t config_read_fw_idx_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { return test_dev_config_update_u8(buf, count, &test_fw_config->read_fw_idx); } The function test_dev_config_update_u8() is called from both the locked and the unlocked context, function config_num_requests_store() and config_read_fw_idx_store() which can both be called asynchronously as they are driver's methods, while test_dev_config_update_u8() and siblings change their argument pointed to by u8 *cfg or similar pointer. To avoid deadlock on test_fw_mutex, the lock is dropped before calling test_dev_config_update_u8() and re-acquired within test_dev_config_update_u8() itself, but alas this creates a race condition. Having two locks wouldn't assure a race-proof mutual exclusion. This situation is best avoided by the introduction of a new, unlocked function __test_dev_config_update_u8() which can be called from the locked context and reducing test_dev_config_update_u8() to: static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) { int ret; mutex_lock(&test_fw_mutex); ret = __test_dev_config_update_u8(buf, size, cfg); mutex_unlock(&test_fw_mutex); return ret; } doing the locking and calling the unlocked primitive, which enables both locked and unlocked versions without duplication of code. The similar approach was applied to all functions called from the locked and the unlocked context, which safely mitigates both deadlocks and race conditions in the driver. __test_dev_config_update_bool(), __test_dev_config_update_u8() and __test_dev_config_update_size_t() unlocked versions of the functions were introduced to be called from the locked contexts as a workaround without releasing the main driver's lock and thereof causing a race condition. The test_dev_config_update_bool(), test_dev_config_update_u8() and test_dev_config_update_size_t() locked versions of the functions are being called from driver methods without the unnecessary multiplying of the locking and unlocking code for each method, and complicating the code with saving of the return value across lock. Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf") Cc: Luis Chamberlain Cc: Greg Kroah-Hartman Cc: Russ Weight Cc: Takashi Iwai Cc: Tianfei Zhang Cc: Shuah Khan Cc: Colin Ian King Cc: Randy Dunlap Cc: linux-kselftest@vger.kernel.org Cc: stable@vger.kernel.org # v5.4 Suggested-by: Dan Carpenter Signed-off-by: Mirsad Goran Todorovac --- lib/test_firmware.c | 52 ++++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 05ed84c2fc4c..35417e0af3f4 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -353,16 +353,26 @@ static ssize_t config_test_show_str(char *dst, return len; } -static int test_dev_config_update_bool(const char *buf, size_t size, +static inline int __test_dev_config_update_bool(const char *buf, size_t size, bool *cfg) { int ret; - mutex_lock(&test_fw_mutex); if (kstrtobool(buf, cfg) < 0) ret = -EINVAL; else ret = size; + + return ret; +} + +static int test_dev_config_update_bool(const char *buf, size_t size, + bool *cfg) +{ + int ret; + + mutex_lock(&test_fw_mutex); + ret = __test_dev_config_update_bool(buf, size, cfg); mutex_unlock(&test_fw_mutex); return ret; @@ -373,7 +383,8 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val) return snprintf(buf, PAGE_SIZE, "%d\n", val); } -static int test_dev_config_update_size_t(const char *buf, +static int __test_dev_config_update_size_t( + const char *buf, size_t size, size_t *cfg) { @@ -384,9 +395,7 @@ static int test_dev_config_update_size_t(const char *buf, if (ret) return ret; - mutex_lock(&test_fw_mutex); *(size_t *)cfg = new; - mutex_unlock(&test_fw_mutex); /* Always return full write size even if we didn't consume all */ return size; @@ -402,7 +411,7 @@ static ssize_t test_dev_config_show_int(char *buf, int val) return snprintf(buf, PAGE_SIZE, "%d\n", val); } -static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) +static int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) { u8 val; int ret; @@ -411,14 +420,23 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) if (ret) return ret; - mutex_lock(&test_fw_mutex); *(u8 *)cfg = val; - mutex_unlock(&test_fw_mutex); /* Always return full write size even if we didn't consume all */ return size; } +static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) +{ + int ret; + + mutex_lock(&test_fw_mutex); + ret = __test_dev_config_update_u8(buf, size, cfg); + mutex_unlock(&test_fw_mutex); + + return ret; +} + static ssize_t test_dev_config_show_u8(char *buf, u8 val) { return snprintf(buf, PAGE_SIZE, "%u\n", val); @@ -471,10 +489,10 @@ static ssize_t config_num_requests_store(struct device *dev, mutex_unlock(&test_fw_mutex); goto out; } - mutex_unlock(&test_fw_mutex); - rc = test_dev_config_update_u8(buf, count, - &test_fw_config->num_requests); + rc = __test_dev_config_update_u8(buf, count, + &test_fw_config->num_requests); + mutex_unlock(&test_fw_mutex); out: return rc; @@ -518,10 +536,10 @@ static ssize_t config_buf_size_store(struct device *dev, mutex_unlock(&test_fw_mutex); goto out; } - mutex_unlock(&test_fw_mutex); - rc = test_dev_config_update_size_t(buf, count, - &test_fw_config->buf_size); + rc = __test_dev_config_update_size_t(buf, count, + &test_fw_config->buf_size); + mutex_unlock(&test_fw_mutex); out: return rc; @@ -548,10 +566,10 @@ static ssize_t config_file_offset_store(struct device *dev, mutex_unlock(&test_fw_mutex); goto out; } - mutex_unlock(&test_fw_mutex); - rc = test_dev_config_update_size_t(buf, count, - &test_fw_config->file_offset); + rc = __test_dev_config_update_size_t(buf, count, + &test_fw_config->file_offset); + mutex_unlock(&test_fw_mutex); out: return rc; From patchwork Wed Apr 26 13:14:41 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mirsad Todorovac X-Patchwork-Id: 87867 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp247499vqo; Wed, 26 Apr 2023 06:35:43 -0700 (PDT) X-Google-Smtp-Source: AKy350axlck8Es0+I8i0pSE7YMgvxxHv8/oBSdSIGn3XeEFru+3PB0/Ua7G2o/EEqoRlqTESs/Ii X-Received: by 2002:a17:902:f543:b0:1a9:7365:fc2a with SMTP id h3-20020a170902f54300b001a97365fc2amr14964990plf.26.1682516143246; Wed, 26 Apr 2023 06:35:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682516143; cv=none; d=google.com; s=arc-20160816; b=ooLPfCXzb8yAkpEbYNqLgH6lhBBc/2lbb/qtf3oLGAv8H1h2G2tH5WyoqhinwxEtiy GFzFCkJ8cyHoq6egwdRb0JPFN0+p/QU71OVL6gu1IiTjBif281QSrsdNxN5KLwYmtd5Y WrCLkzIPMkG5nrlBXhd+Rz398ji5Mg77Q0UEn89LbhwPu1ZES7HdCqcrSsV81rgT257y 6Zh6+oR440t3mFcQF0rRbXJtEsTm9iFGJKbke5Rrf7VWInam95VcoJyXbCSdaZUqxjgq 4/XNaNC9d6OtW3sL2Ol+cOMmg23hsNYsnmJFhWfjOEhC0kTFbnkWhlwnlnLKDArBaONm yNOg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature:dkim-signature; bh=0ac8rMeVTEEXqWDAl8bMXL3UcHJCz484R4J4Bfdy7pI=; b=Vi9MkXuVS8K+1+NFzb/fFLbIJyXS952YS3+WitJBDgIIrwsnhtdnkhfnBIWiy7Nz4r fGgvhU5pOqsNwAXj9IuIPftoBrXt9bRALuYB7ktGMYKG7iAYP66HSqdDWqm365to1DdM KAHTogrOKsPv27H0k2UDXC/k8o5eti0dTGPE65+4BoYWQR/NCxAa38FapWR4VfYAmhuo uSqPgHLI+Xm2V5NEZHk/LbjEV5ziG23+gY9hAAfldoxlaMmfRsgjZ34EvF+PErI18iTw NqpkMC2EqztWMDzwGQY6RQQYO9jNY9VBOMc7Jhx9zptXS2lQFgHYFiJyd4H22t/qLNHl zD0w== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=f3PBIre1; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=W2PYGjp0; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alu.unizg.hr Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 5-20020a170902c20500b001a64a25c808si13257682pll.5.2023.04.26.06.35.17; Wed, 26 Apr 2023 06:35:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=f3PBIre1; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=W2PYGjp0; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alu.unizg.hr Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241140AbjDZNQJ (ORCPT + 99 others); Wed, 26 Apr 2023 09:16:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49270 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241125AbjDZNP4 (ORCPT ); Wed, 26 Apr 2023 09:15:56 -0400 Received: from domac.alu.hr (domac.alu.unizg.hr [IPv6:2001:b68:2:2800::3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DD74A5FC0; Wed, 26 Apr 2023 06:15:53 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by domac.alu.hr (Postfix) with ESMTP id 7C8E66017C; Wed, 26 Apr 2023 15:15:52 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682514952; bh=BkbDGciTS3W//1DGsvWYvladd4chFAo6enUQtEEQQqg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=f3PBIre12N8S7+pAiYxpBnotF1xNuHJiB9WFD3DUQyy2zG+VLFAjHYCj0i4WEsu68 xMLy5h2FG9SUC/UiN9CdAfMoVzO0TVINpx3ghAH3gn/tk1EX1a1LKnogP6QZ4+9gsq N8phvi94or/bweFqGAeEKvc9SqYGmSJDMJrBJ8uRzpqaoN3jdji/uX38SxeO3RSe37 aKzRqvv8B/L6o5Znov3D9f8Ew9+MNk91ogjzZ9Toy99aLUwYI5HcGpr4pCNhtMQz+g 3etOCjfPN1MM9Nm3ZLJlosGTBadZ6tnmfH2JH8ZXl3NK6rOBEcQivIQiUwlqA66yO0 /u4MXMZRQDAYA== X-Virus-Scanned: Debian amavisd-new at domac.alu.hr Received: from domac.alu.hr ([127.0.0.1]) by localhost (domac.alu.hr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5dbKQDu-qTY3; Wed, 26 Apr 2023 15:15:50 +0200 (CEST) Received: by domac.alu.hr (Postfix, from userid 1014) id 595406017E; Wed, 26 Apr 2023 15:15:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682514950; bh=BkbDGciTS3W//1DGsvWYvladd4chFAo6enUQtEEQQqg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=W2PYGjp02QXGEwIrsbzABoB8RC9N63W+Au8q3DPzM6xAWOlUKfDgEYJZj2U/7/Iqe ANXorKJH9xllVago/Fo5zE0K6ETk4c775VIQi0letwp3q8Z8Ohk13ZdxHqxLEDuWNc Jl0/r1SJin0DWdxnJip+IPZg85KrbfhJ4h22X1DoZNUzlB5/ppKiTSa8CqJL39Ab/Z o8Cs7+KRfvogXfQx9VJtaLDdyK7k6P6jFNngjDqmbO/dysJPY8ukL8mt36OHpMiJ8H B3pv307BqdEiRfx9jyRRiT6meCiJhdE2N6q2OrsID/7TOYmPRmm4VHoXPADsVYZJ04 vxSSJJyo53LwQ== From: Mirsad Goran Todorovac To: Mirsad Goran Todorovac , linux-kernel@vger.kernel.org Cc: Luis Chamberlain , Greg Kroah-Hartman , Russ Weight , Tianfei Zhang , Shuah Khan , Colin Ian King , Randy Dunlap , linux-kselftest@vger.kernel.org, stable@vger.kernel.org, Dan Carpenter , Takashi Iwai Subject: [PATCH v5 2/3] test_firmware: fix a memory leak with reqs buffer Date: Wed, 26 Apr 2023 15:14:41 +0200 Message-Id: <20230426131438.16936-2-mirsad.todorovac@alu.unizg.hr> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230426131438.16936-1-mirsad.todorovac@alu.unizg.hr> References: <20230426131438.16936-1-mirsad.todorovac@alu.unizg.hr> MIME-Version: 1.0 X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764246047748672623?= X-GMAIL-MSGID: =?utf-8?q?1764246047748672623?= Dan Carpenter spotted that test_fw_config->reqs will be leaked if trigger_batched_requests_store() is called two or more times. The same appears with trigger_batched_requests_async_store(). This bug wasn't trigger by the tests, but observed by Dan's visual inspection of the code. The recommended workaround was to return -EBUSY if test_fw_config->reqs is already allocated. Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf") Cc: Luis Chamberlain Cc: Greg Kroah-Hartman Cc: Russ Weight Cc: Tianfei Zhang Cc: Shuah Khan Cc: Colin Ian King Cc: Randy Dunlap Cc: linux-kselftest@vger.kernel.org Cc: stable@vger.kernel.org # v5.4 Suggested-by: Dan Carpenter Suggested-by: Takashi Iwai Signed-off-by: Mirsad Goran Todorovac --- lib/test_firmware.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 35417e0af3f4..91b232ed3161 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -913,6 +913,11 @@ static ssize_t trigger_batched_requests_store(struct device *dev, mutex_lock(&test_fw_mutex); + if (test_fw_config->reqs) { + rc = -EBUSY; + goto out_bail; + } + test_fw_config->reqs = vzalloc(array3_size(sizeof(struct test_batched_req), test_fw_config->num_requests, 2)); @@ -1011,6 +1016,11 @@ ssize_t trigger_batched_requests_async_store(struct device *dev, mutex_lock(&test_fw_mutex); + if (test_fw_config->reqs) { + rc = -EBUSY; + goto out_bail; + } + test_fw_config->reqs = vzalloc(array3_size(sizeof(struct test_batched_req), test_fw_config->num_requests, 2)); From patchwork Wed Apr 26 13:14:43 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mirsad Todorovac X-Patchwork-Id: 87866 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp237447vqo; Wed, 26 Apr 2023 06:21:31 -0700 (PDT) X-Google-Smtp-Source: AKy350bidJts2Wy0T9/Xt8ZrpmqNCL6fOdGKw0KZHSmDP73KEgGZmztZ/iFGCnBiH+IPn7LljETf X-Received: by 2002:a17:90a:9a97:b0:247:862d:a224 with SMTP id e23-20020a17090a9a9700b00247862da224mr20689749pjp.27.1682515291483; Wed, 26 Apr 2023 06:21:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682515291; cv=none; d=google.com; s=arc-20160816; b=RJNgkWP9iQEYsB13G2tEZzpykDUQ0X4LdvV12tAX969yJajiDDMnqxUREb82eghlms e2HmWA/b5kKTVQUJERobj1gDUyTmejlZnwqOJJDN7LX0KJrEvvvWvG9y64NhlEO9hA/x V0vUBYvvIpRd+f04HQd+Mq5Eojs+v1/3HjvEQXXr0vwbFMffvXxse8Yak2uGkiO35T1N Wl+E5ztHx3yB4LpVqwgOmrUlSd7m3eowu5fONvuqcoJVXCAzMFEeAk59QzgDMb9XWFvP WTwDswU83Gw3nHa1Hlwx6EC5JJ4ajCKk4QtXvKFkWSWAVOoMPsmu9S2m/NVNdAdo50op 5tnw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature:dkim-signature; bh=ub5Z0aktNzj/lepH4mfdcwqtpHFTthYuGcmg+70XkjI=; b=tMxqOaC9lfoBMStE+vw5gh3Hx6KD1CGgo+BW7x5fNCYnGg8j2GqrlgwOIeC+bHwVRF z2y5G17WcBDdtBR5ThwNIRzcYRH2UigLN2H1bLLnQhY+FqhIFBqHpusyyDddoglc1IVi lHCe6Jm2K/wAgZwbMw+ZeYie8KCIOZn/Q59B8VNMMlZs00cvaGlFh1cRTdjOqi584S7v YJC7a/NGWFRxzSQ8o2Yv7yziGYpPFbB9RrLr5g4nLG8GWu+n6HnlbXfkWyW6XEdq5H5w D22gdj/+4e0/Qkc291h2yxvWmL8N8ujm1V/0N73q40kk6IKyZH1vl8HoOxx1iDFLjpWV ma0w== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=jLF34zZy; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b="jeJ0Ih/8"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alu.unizg.hr Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id lr17-20020a17090b4b9100b0023b481b8dcesi20488865pjb.102.2023.04.26.06.21.16; Wed, 26 Apr 2023 06:21:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=jLF34zZy; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b="jeJ0Ih/8"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alu.unizg.hr Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241109AbjDZNQ3 (ORCPT + 99 others); Wed, 26 Apr 2023 09:16:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49270 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241123AbjDZNQR (ORCPT ); Wed, 26 Apr 2023 09:16:17 -0400 Received: from domac.alu.hr (domac.alu.unizg.hr [161.53.235.3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6F904659F; Wed, 26 Apr 2023 06:16:16 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by domac.alu.hr (Postfix) with ESMTP id 35C846017C; Wed, 26 Apr 2023 15:16:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682514974; bh=DplkcwFmSKllgiewQPCLoUNazOClR1ZbTBiZSdEqy0U=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=jLF34zZySbrMIOar5hbB9l+LuyXzqgbjgZcvmCeF6wi62cxiSBVLdnN33YdtamlH7 cxz82IsYoaZRnYV0sAcYqYPs1h2BS4xFbwHv/lb2vTonHxSu9DqH25Xgz8rtlCKLTI tEEmgSqjGTS/zSGn8o2mMkarG1eHI3oxjCTTSntsDb7cib6Yp0EEQsreaud/UKc+Su 6yRps4q37snvK5dKB0mjdVu0eP4lDtE9J8Z6zbw1uj7LyHB66qCgBh6XYkk4OqaaOK J2qpvxyNE/CDHmFDdWbokqReuiI/Xvi0AR0M2bA/lPRVT45bJ44IDzw8aGrGdM7CRp oMTYHbrvrkGpA== X-Virus-Scanned: Debian amavisd-new at domac.alu.hr Received: from domac.alu.hr ([127.0.0.1]) by localhost (domac.alu.hr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1v-Us3qcu3JB; Wed, 26 Apr 2023 15:16:11 +0200 (CEST) Received: by domac.alu.hr (Postfix, from userid 1014) id BF8626017E; Wed, 26 Apr 2023 15:16:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682514971; bh=DplkcwFmSKllgiewQPCLoUNazOClR1ZbTBiZSdEqy0U=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=jeJ0Ih/8vIHpF7+4lPcHrAOhBKIylrZon4YZvFtptil7dkKd30YiZ7juYT+ui9lC1 VVvemuF2C0PxVzqLOwBP38jRFtCOLUAsENzDleEvBBMlIesu8UEjswrCK7Wiwptuh4 S2afR3HJ7AYsPq32LgNYJ1U3xFYEkjtUzIv7BDlHJ0j15j4W2jLqzdHalf2tW28X8s olPSgMrSSoG7F20x9Up56Syg3p+JkSxoA1xNLbHSywcZLImZBrJ6vQoQnA+Tp5hIvh T1C4zHtx+IpbcC1qZaO851r7C4rnNjvqnGiBN9rSjCnOQKulBznA+hMCBLwN9OPQU8 uObYf4PwasanQ== From: Mirsad Goran Todorovac To: Mirsad Goran Todorovac , linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , Dan Carpenter , Takashi Iwai , Luis Chamberlain , Russ Weight , Tianfei zhang , Christophe JAILLET , Zhengchao Shao , Colin Ian King , Kees Cook , Scott Branden , linux-kselftest@vger.kernel.org, stable@vger.kernel.org Subject: [PATCH v5 3/3] test_firmware: fix the memory leak of the allocated firmware buffer Date: Wed, 26 Apr 2023 15:14:43 +0200 Message-Id: <20230426131438.16936-3-mirsad.todorovac@alu.unizg.hr> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230426131438.16936-1-mirsad.todorovac@alu.unizg.hr> References: <20230426131438.16936-1-mirsad.todorovac@alu.unizg.hr> MIME-Version: 1.0 X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764245153978261207?= X-GMAIL-MSGID: =?utf-8?q?1764245153978261207?= The following kernel memory leak was noticed after running tools/testing/selftests/firmware/fw_run_tests.sh: [root@pc-mtodorov firmware]# cat /sys/kernel/debug/kmemleak . . . unreferenced object 0xffff955389bc3400 (size 1024): comm "test_firmware-0", pid 5451, jiffies 4294944822 (age 65.652s) hex dump (first 32 bytes): 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 00 00 GH4567.......... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] slab_post_alloc_hook+0x8c/0x3c0 [] __kmem_cache_alloc_node+0x184/0x240 [] kmalloc_trace+0x2e/0xc0 [] test_fw_run_batch_request+0x9d/0x180 [] kthread+0x10b/0x140 [] ret_from_fork+0x29/0x50 unreferenced object 0xffff9553c334b400 (size 1024): comm "test_firmware-1", pid 5452, jiffies 4294944822 (age 65.652s) hex dump (first 32 bytes): 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 00 00 GH4567.......... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] slab_post_alloc_hook+0x8c/0x3c0 [] __kmem_cache_alloc_node+0x184/0x240 [] kmalloc_trace+0x2e/0xc0 [] test_fw_run_batch_request+0x9d/0x180 [] kthread+0x10b/0x140 [] ret_from_fork+0x29/0x50 unreferenced object 0xffff9553c334f000 (size 1024): comm "test_firmware-2", pid 5453, jiffies 4294944822 (age 65.652s) hex dump (first 32 bytes): 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 00 00 GH4567.......... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] slab_post_alloc_hook+0x8c/0x3c0 [] __kmem_cache_alloc_node+0x184/0x240 [] kmalloc_trace+0x2e/0xc0 [] test_fw_run_batch_request+0x9d/0x180 [] kthread+0x10b/0x140 [] ret_from_fork+0x29/0x50 unreferenced object 0xffff9553c3348400 (size 1024): comm "test_firmware-3", pid 5454, jiffies 4294944822 (age 65.652s) hex dump (first 32 bytes): 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 00 00 GH4567.......... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] slab_post_alloc_hook+0x8c/0x3c0 [] __kmem_cache_alloc_node+0x184/0x240 [] kmalloc_trace+0x2e/0xc0 [] test_fw_run_batch_request+0x9d/0x180 [] kthread+0x10b/0x140 [] ret_from_fork+0x29/0x50 [root@pc-mtodorov firmware]# Note that the size 1024 corresponds to the size of the test firmware buffer. The actual number of the buffers leaked is around 70-110, depending on the test run. The cause of the leak is the following: request_partial_firmware_into_buf() and request_firmware_into_buf() provided firmware buffer isn't released on release_firmware(), we have allocated it and we are responsible for deallocating it manually. This is introduced in a number of context where previously only release_firmware() was called, which was insufficient. Reported-by: Mirsad Goran Todorovac Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf") Cc: Greg Kroah-Hartman Cc: Dan Carpenter Cc: Takashi Iwai Cc: Luis Chamberlain Cc: Russ Weight Cc: Tianfei zhang Cc: Christophe JAILLET Cc: Zhengchao Shao Cc: Colin Ian King Cc: linux-kernel@vger.kernel.org Cc: Kees Cook Cc: Scott Branden Cc: Luis R. Rodriguez Cc: linux-kselftest@vger.kernel.org Cc: stable@vger.kernel.org # v5.4 Signed-off-by: Mirsad Goran Todorovac --- lib/test_firmware.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 91b232ed3161..1d7d480b8eeb 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -45,6 +45,7 @@ struct test_batched_req { bool sent; const struct firmware *fw; const char *name; + const char *fw_buf; struct completion completion; struct task_struct *task; struct device *dev; @@ -175,8 +176,14 @@ static void __test_release_all_firmware(void) for (i = 0; i < test_fw_config->num_requests; i++) { req = &test_fw_config->reqs[i]; - if (req->fw) + if (req->fw) { + if (req->fw_buf) { + kfree_const(req->fw_buf); + req->fw_buf = NULL; + } release_firmware(req->fw); + req->fw = NULL; + } } vfree(test_fw_config->reqs); @@ -670,6 +677,8 @@ static ssize_t trigger_request_store(struct device *dev, mutex_lock(&test_fw_mutex); release_firmware(test_firmware); + if (test_fw_config->reqs) + __test_release_all_firmware(); test_firmware = NULL; rc = request_firmware(&test_firmware, name, dev); if (rc) { @@ -770,6 +779,8 @@ static ssize_t trigger_async_request_store(struct device *dev, mutex_lock(&test_fw_mutex); release_firmware(test_firmware); test_firmware = NULL; + if (test_fw_config->reqs) + __test_release_all_firmware(); rc = request_firmware_nowait(THIS_MODULE, 1, name, dev, GFP_KERNEL, NULL, trigger_async_request_cb); if (rc) { @@ -812,6 +823,8 @@ static ssize_t trigger_custom_fallback_store(struct device *dev, mutex_lock(&test_fw_mutex); release_firmware(test_firmware); + if (test_fw_config->reqs) + __test_release_all_firmware(); test_firmware = NULL; rc = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOUEVENT, name, dev, GFP_KERNEL, NULL, @@ -874,6 +887,8 @@ static int test_fw_run_batch_request(void *data) test_fw_config->buf_size); if (!req->fw) kfree(test_buf); + else + req->fw_buf = test_buf; } else { req->rc = test_fw_config->req_firmware(&req->fw, req->name, @@ -934,6 +949,7 @@ static ssize_t trigger_batched_requests_store(struct device *dev, req->fw = NULL; req->idx = i; req->name = test_fw_config->name; + req->fw_buf = NULL; req->dev = dev; init_completion(&req->completion); req->task = kthread_run(test_fw_run_batch_request, req, @@ -1038,6 +1054,7 @@ ssize_t trigger_batched_requests_async_store(struct device *dev, for (i = 0; i < test_fw_config->num_requests; i++) { req = &test_fw_config->reqs[i]; req->name = test_fw_config->name; + req->fw_buf = NULL; req->fw = NULL; req->idx = i; init_completion(&req->completion);