[ 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 mcgrof@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Russ Weight russell.h.weight@intel.com Cc: Takashi Iwai tiwai@suse.de Cc: Tianfei Zhang tianfei.zhang@intel.com Cc: Shuah Khan shuah@kernel.org Cc: Colin Ian King colin.i.king@gmail.com Cc: Randy Dunlap rdunlap@infradead.org Cc: linux-kselftest@vger.kernel.org Cc: stable@vger.kernel.org # v5.4, 4.19, 4.14 Suggested-by: Dan Carpenter error27@gmail.com Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.... Signed-off-by: Mirsad Todorovac mirsad.todorovac@alu.unizg.hr
[ 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: minor versioning clarifications for the patchwork. no changes to the commit.
v3: fixed a minor typo. no change to commit.
v2: tested on 5.4 stable build.
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 38553944e967..92d7195d5b5b 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -301,16 +301,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; @@ -340,7 +350,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; @@ -352,14 +362,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; @@ -392,10 +411,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;