--- /dev/null
+From 4acfe3dfde685a5a9eaec5555351918e2d7266a1 Mon Sep 17 00:00:00 2001
+From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
+Date: Tue, 9 May 2023 10:47:45 +0200
+Subject: test_firmware: prevent race conditions by a correct implementation of locking
+
+From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
+
+commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 upstream.
+
+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 <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
+Suggested-by: Dan Carpenter <error27@gmail.com>
+Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
+Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr
+Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+---
+ lib/test_firmware.c | 37 ++++++++++++++++++++++++++++---------
+ 1 file changed, 28 insertions(+), 9 deletions(-)
+
+--- a/lib/test_firmware.c
++++ b/lib/test_firmware.c
+@@ -284,16 +284,26 @@ static ssize_t config_test_show_str(char
+ 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;
+@@ -323,7 +333,7 @@ static ssize_t test_dev_config_show_int(
+ 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;
+@@ -335,14 +345,23 @@ static int test_dev_config_update_u8(con
+ 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;
+@@ -375,10 +394,10 @@ static ssize_t config_num_requests_store
+ 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;
projects / thirdparty / kernel / stable-queue.git / commitdiff
