From patchwork Wed Aug 16 21:24: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: 136222 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b82d:0:b0:3f2:4152:657d with SMTP id z13csp2284662vqi; Sat, 19 Aug 2023 19:36:24 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHtyX5dyl2yXZHh/yrxrD23a9ggcnZ61J0f8BuX2IJfaFDelsRPdmKGpngbJEbiSE+9Rtnb X-Received: by 2002:a54:4487:0:b0:3a6:fb16:c782 with SMTP id v7-20020a544487000000b003a6fb16c782mr4638963oiv.30.1692498984697; Sat, 19 Aug 2023 19:36:24 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1692498984; cv=none; d=google.com; s=arc-20160816; b=E2WtLh4nPjZ2no+KA8DEiMWiwK/unzCklma3x0y2fZXFUvcRtIcuBgMsnSURsyzKT0 lW1S2/lMbBAyjLi53IPI4bAvvAhhkrU0RoVTgQyHZ03r5xkxd3HwJ5evT10VfNiPOSUV E6kqQfbmCk4YBDdCz3vdX4BPxnFQsJFoFnGM27VO+bEZnMoJI/hoJa76yLjbY00F9hx/ 8B/gEmJFUtrUY8KF/SoOS/UNxam3MMhO0wOlaV3k+fHdtDCmPLXVyj6XAb9CtHtK7R7f iKlP4RdQS0lvyaOTdHV42SLyGWH4o7dOiBx9WVDRvfw0H6K0cqGyR20AN2HXoPgalnlt /XnQ== 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=RHsNKWWv43WBOT6+FbZwMZfBnhvBHYSlUli6QfPBoSI=; fh=6mTdv1Zv/FjMMrXq2sdfdqYvCwoNvAs4pswwid2dL+U=; b=VV9p8yn61GYvRtGu/yAK2csutb4JHOPWAmDtno6d3A3cvQxBfwlFKUztu/tnLWMncD E64p7EGJ/SPEd+pfmiSTGDak9Yks4WulpibbGluIzd+FxzIEIfgNYE6Cah7QWVw6JnHu CQ2+DdZn/svGc/76cdbZOQK8PhfZ8tJ60cc0i/NQQ2TTRn2E4bSC1LE2EAqAToDFOM2X vu89CwGXmI4GdeNheA2uE7Plns7bstmtHqIQfoJfaSEPdOLtwRZ1XAkVpMMLDbWyk4Yb iHQU3Xj6YhFKEbWQgXk+6WiBRcio4UgAdV4MnscaR+GMuJrwOWcK3CTHl6yvdEfi5JR2 1TGw== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=RoCP8Q+i; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=qwfJOGv6; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 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 lindbergh.monkeyblade.net (lindbergh.monkeyblade.net. [2620:137:e000::1:18]) by mx.google.com with ESMTPS id f10-20020a056a0022ca00b00687a5701a37si4625978pfj.26.2023.08.19.19.36.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 19 Aug 2023 19:36:24 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18; Authentication-Results: mx.google.com; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=RoCP8Q+i; dkim=fail header.i=@alu.unizg.hr header.s=mail header.b=qwfJOGv6; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id A2FD3263B05; Sat, 19 Aug 2023 11:39:16 -0700 (PDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346449AbjHPV05 (ORCPT + 99 others); Wed, 16 Aug 2023 17:26:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39022 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346501AbjHPV0s (ORCPT ); Wed, 16 Aug 2023 17:26:48 -0400 Received: from domac.alu.hr (domac.alu.unizg.hr [161.53.235.3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DA71D1BE8; Wed, 16 Aug 2023 14:26:46 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by domac.alu.hr (Postfix) with ESMTP id 605FA60174; Wed, 16 Aug 2023 23:26:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1692221204; bh=qKCgEnpgvae4+SWUleyz/Br7qQr7Lwj+vKFlzTtf4f4=; h=From:To:Cc:Subject:Date:From; b=RoCP8Q+imRVKysEr2nOOAqE/pHU4asus1gTj4j4rprDE6gnuCnqXwMhtFwwIKUB9Y vyIbr5/nteC786Ajw0m9ZalOz5tpjXMSgp7hZTeBECHA/OhHovQSF/kfPb0giAWg8o PgmzYSl62s3VN+82PSLG/cclTt3ywj9N5pC9yFmHeHaGYuk6nAUvbAUTYPXxREa7yd J4aWZkkVmvdpPtKwl8ZLd9ps1RDOuouSdvgZ3yQ7rshyIbnQ39tH3nptFseKHchqcq 0WaohEqrQVXfn4QpCAaGyIHfn6xdzsJDkjTd6NCVBq4TDfEleUyZGP3HfHZKo1VMOU CYPM1/bvoEYdg== 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 jmYuHQsEBa5M; Wed, 16 Aug 2023 23:26:40 +0200 (CEST) Received: from defiant.. (unknown [94.250.191.183]) by domac.alu.hr (Postfix) with ESMTPSA id D36C36015E; Wed, 16 Aug 2023 23:26:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=alu.unizg.hr; s=mail; t=1692221200; bh=qKCgEnpgvae4+SWUleyz/Br7qQr7Lwj+vKFlzTtf4f4=; h=From:To:Cc:Subject:Date:From; b=qwfJOGv6KMYz2S2eozDyAoukDK4gnIpeMTBHKXVDomcJpUZuH3lpx4UXFhsEmK1Jh ja80ITU5qvGNwI3BIphSW+vA+IiSC7mwdDl2FGP5O0Px+qQvEDEeew0/GjGjx5HCLi jRjkesxoQEnHpl/6RcNK+iIMJbKL6XeZSm+oLyA01UanWJtrXjO87HiMAjf+F/Ub5T mHSt4kKS+mIea9kfm0eqCklgnVqBcqC2Ai2X3wWywRi6AmSuEs9uKkzUpkZlwD2vGM c+VcZ6oEz66zaZzH/LspY4hnym+0mUT9OghsGyWmOKFTELe1n9FEKCWxK8YwEGmx1h veEamjIlJaAzg== From: Mirsad Goran Todorovac To: Mirsad Goran Todorovac , linux-kernel@vger.kernel.org Cc: "Luis R . Rodriguez" , 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 v4 4.14 1/1] test_firmware: prevent race conditions by a correct implementation of locking Date: Wed, 16 Aug 2023 23:24:06 +0200 Message-Id: <20230816212405.816977-1-mirsad.todorovac@alu.unizg.hr> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham 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: INBOX X-GMAIL-THRID: 1774704340824633308 X-GMAIL-MSGID: 1774713815264547632 [ Upstream commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 ] 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); // NOTE: HERE is the race!!! Function can be preempted! // test_fw_config->reqs can change between the release of // the lock about and acquire of the lock in the // test_dev_config_update_u8() 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. Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests") Cc: Luis R. Rodriguez 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, 4.19, 4.14 Suggested-by: Dan Carpenter Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr Signed-off-by: Mirsad Goran Todorovac [ This is the patch to fix the racing condition in locking for the 5.4, ] [ 4.19 and 4.14 stable branches. Not all the fixes from the upstream ] [ commit apply, but those which do are verbatim equal to those in the ] [ upstream commit. ] --- v4: verbatim the same patch as for the 5.4 stable tree which patchwork didn't apply lib/test_firmware.c | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 34210306ea66..d407e5e670f3 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -283,16 +283,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, - bool *cfg) +static inline int __test_dev_config_update_bool(const char *buf, size_t size, + bool *cfg) { int ret; - mutex_lock(&test_fw_mutex); if (strtobool(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; @@ -322,7 +332,7 @@ static ssize_t test_dev_config_show_int(char *buf, int cfg) return snprintf(buf, PAGE_SIZE, "%d\n", val); } -static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) +static inline int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) { int ret; long new; @@ -334,14 +344,23 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) if (new > U8_MAX) return -EINVAL; - mutex_lock(&test_fw_mutex); *(u8 *)cfg = new; - 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 cfg) { u8 val; @@ -374,10 +393,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;