From: Greg Kroah-Hartman Date: Mon, 21 Aug 2023 13:22:11 +0000 (+0200) Subject: 4.19-stable patches X-Git-Tag: v6.4.12~42 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=57ae63b6ec1e061040767a22daaaf1e0207b56a7;p=thirdparty%2Fkernel%2Fstable-queue.git 4.19-stable patches added patches: test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch --- diff --git a/queue-4.19/series b/queue-4.19/series index 15823204aee..9eccb9b3c8e 100644 --- a/queue-4.19/series +++ b/queue-4.19/series @@ -76,3 +76,4 @@ serial-8250-fix-oops-for-port-pm-on-uart_change_pm.patch alsa-usb-audio-add-support-for-mythware-xa001au-capture-and-playback-interfaces.patch cifs-release-folio-lock-on-fscache-read-hit.patch mmc-wbsd-fix-double-mmc_free_host-in-wbsd_init.patch +test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch diff --git a/queue-4.19/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch b/queue-4.19/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch new file mode 100644 index 00000000000..adde8ab3378 --- /dev/null +++ b/queue-4.19/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch @@ -0,0 +1,205 @@ +From 4acfe3dfde685a5a9eaec5555351918e2d7266a1 Mon Sep 17 00:00:00 2001 +From: Mirsad Goran Todorovac +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 + +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 +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 +Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr +Signed-off-by: Greg Kroah-Hartman +--- + 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;