From patchwork Fri Apr 21 18:52:06 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mirsad Todorovac X-Patchwork-Id: 86473 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1289417vqo; Fri, 21 Apr 2023 12:16:13 -0700 (PDT) X-Google-Smtp-Source: AKy350YSfA9e6qaPpKajS5r4COPxSk4ihvE5f5QGRYuwb05L0/mqS6r1D8mtqx2sqbdrO79boMq+ X-Received: by 2002:a05:6a21:3a85:b0:e7:7844:9029 with SMTP id zv5-20020a056a213a8500b000e778449029mr7136127pzb.53.1682104573046; Fri, 21 Apr 2023 12:16:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682104573; cv=none; d=google.com; s=arc-20160816; b=vM4Jclu6Jr3FaQXEntV01c0bZGnOpNStYbJjsP+8rEhQuUkm3lbYVY8qMrE84137sF BgHazS6no7ZXdnSvh1CA0ob5PwFzDlbeNKcNHR9KKTskqKfbXClWooilmjgmKWj+bJ6J RKjSS1i7k3zYvX5nOM2HYJiAq66dWr7tAFJ2YYWy+j1GbAATxpIoveKSZjC6EAEI7Y7x 816Kw5clA+BYwaeH4aqgyr6xtpzZoWBaqukxXlScsgsH9X/A0Aj6kAxr9+MrYaCHV8SF ineVCIs+MScMK7Ps66CKj51pehz5ron0tnIJBeNuaMcu6TBDb52/zQCFC5uSDT1BO6wo cyQw== 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=6ca3Qo/6ZGwdXi/gcFd5SR9efJtsZZU3VaPjrWo5rzM=; b=iCe7vFCy9eO1xcX4uPd9GBSdyMo+YCsOL19mh+cFFVy4LLJlyv9i/AFvIffXL8/ZtM Xn3AwikYXn/HIJEtGf8JX6CU/HrnVN2BowqqiAmg6hFoKVANW2oelNOhn7v0pZ+q2YjT THbs+V0DGgXK763TpfOo5h5CCe8TSjEB0MtC82SRKvViuT3muHv/WI8f+WnSoeQ45bh1 XVshKZ9JDz3BeKIQoDWABTPdUZwoQt7yUotxHqJ7KME5nc/344XTO5wswz2j4pStUSNN GR0+eCDUA1OLjYZFShIN0uMwify+o41BLPnhFisdHqJu0AvF8FcojeWgGeF87QeMLnyd mPwA== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=TpJBRDU3; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b="Mo2qI/yH"; 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 c20-20020a6566d4000000b00518d97e15ebsi4989698pgw.431.2023.04.21.12.16.00; Fri, 21 Apr 2023 12:16:13 -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=TpJBRDU3; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b="Mo2qI/yH"; 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 S232312AbjDUTC4 (ORCPT + 99 others); Fri, 21 Apr 2023 15:02:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59942 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229666AbjDUTCy (ORCPT ); Fri, 21 Apr 2023 15:02:54 -0400 Received: from domac.alu.hr (domac.alu.unizg.hr [161.53.235.3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E51F81B7 for ; Fri, 21 Apr 2023 12:02:51 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by domac.alu.hr (Postfix) with ESMTP id BF9356012C; Fri, 21 Apr 2023 21:02:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682103769; bh=FdPLFo2RCEc+i7kfmV4WzCU9lEz+rq4G5vyLOVPZVMA=; h=From:To:Cc:Subject:Date:From; b=TpJBRDU3qD+JJll+nc9tmd8Hw+AZ2iCdd9Og5fCCvajbtQJrDKHvJg2b/gsgaUvYm CdoZmGvyGfG2fu8c3qxBLLvoJWBKz4S+ERNBrVXbkOxLFFMbHZfnM6yop4jQHGTWhf Sv64qiby+JtfH/v8Hy1YF1ilqbp4G/gBrgB7l50RarlI00aA2APnI4YNfKDdBU3Pxv qRJ5DP4E1ZXLBtZXTOsDQOZD8Ghj3HgAAnOrJbyRVJzvReP3K8U1ChXSvUmeRpt4PF vlGGXBQXkDVOuSDYVaZdk0jGkNIDAYOSNLzOrMYCzjOuU1DNy7zGfOo2G4vXLNuwns OPpjPrB8JXyeQ== 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 NWTIAfSD5oSf; Fri, 21 Apr 2023 21:02:46 +0200 (CEST) Received: by domac.alu.hr (Postfix, from userid 1014) id 07CF06012D; Fri, 21 Apr 2023 21:02:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1682103766; bh=FdPLFo2RCEc+i7kfmV4WzCU9lEz+rq4G5vyLOVPZVMA=; h=From:To:Cc:Subject:Date:From; b=Mo2qI/yHsmckJiAeUa08ZtRCHAK43wYe/vmVKGsmz0NBD8PYeRR0OD8akGP8+dV5r utD+eHfnLTeHFLsk+qXA2ULh5oIQfTv99rzLmfSA3m2o9AJzWdytM9c/weGdCs4/7/ hcYkHj728SD1WnPcyee83j5OqquZmcdYAVjnGCzT3bD6a0pERYeI5B8OMrpksKLqQT IyJeC3AGkB1QiSWPHQyoZS60U4SWay50Cp/MjlydKdBEUTwzUXmRWroCdGzOr8rqNm uqwmtAFyhvUrDpen9P4HJ88s5EJqaqGPpprxOxPJKIzNNhSqhYZ8gi9suyfy06E0rq eCiizFhgHTGQg== From: Mirsad Goran Todorovac To: Greg Kroah-Hartman , Luis Chamberlain , Russ Weight , linux-kernel@vger.kernel.org Cc: Mirsad Goran Todorovac , Tianfei zhang , Christophe JAILLET , Zhengchao Shao , Colin Ian King , Takashi Iwai , Kees Cook , Scott Branden , Dan Carpenter Subject: [PATCH RESEND v4 1/1] test_firmware: fix some memory leaks and racing conditions Date: Fri, 21 Apr 2023 20:52:06 +0200 Message-Id: <20230421185205.28743-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?1763814484971987688?= X-GMAIL-MSGID: =?utf-8?q?1763814484971987688?= Some functions were called both from locked and unlocked context, so the lock was dropped prematurely, introducing a race condition when deadlock was avoided. Having two locks wouldn't assure a race-proof mutual exclusion. __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 causing a race condition. This should guarantee mutual exclusion and prevent any race conditions. Locked versions simply allow for mutual exclusion and call the unlocked counterparts, to avoid duplication of code. trigger_batched_requests_store() and trigger_batched_requests_async_store() now return -EBUSY if called with test_fw_config->reqs already allocated, so the memory leak is prevented. The same functions now keep track of the allocated buf for firmware in req->fw_buf as release_firmware() will not deallocate this storage for us. Additionally, in __test_release_all_firmware(), req->fw_buf is released before calling release_firmware(req->fw), foreach test_fw_config->reqs[i], i = 0 .. test_fw_config->num_requests-1 Cc: Greg Kroah-Hartman 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: Takashi Iwai Cc: Kees Cook Cc: Scott Branden Cc: Luis R. Rodriguez Suggested-by: Dan Carpenter Signed-off-by: Mirsad Goran Todorovac --- v3 -> v4 - fix additional memory leaks of the allocated firmware buffers - fix noticed racing conditions in conformance with the existing code - make it a single patch lib/test_firmware.c | 81 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 05ed84c2fc4c..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); @@ -353,16 +360,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 +390,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 +402,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 +418,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 +427,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 +496,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 +543,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 +573,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; @@ -652,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) { @@ -752,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) { @@ -794,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, @@ -856,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, @@ -895,6 +928,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)); @@ -911,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, @@ -993,6 +1032,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)); @@ -1010,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);