From: Greg Kroah-Hartman Date: Mon, 7 Aug 2023 09:17:27 +0000 (+0200) Subject: 5.4-stable patches X-Git-Tag: v4.14.321~16 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=53717b2563b373b018ad8be7bd5b248b569ab7d7;p=thirdparty%2Fkernel%2Fstable-queue.git 5.4-stable patches added patches: test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch test_firmware-return-enomem-instead-of-enospc-on-failed-memory-allocation.patch --- diff --git a/queue-5.4/series b/queue-5.4/series index 7c19a016811..f2b024bf726 100644 --- a/queue-5.4/series +++ b/queue-5.4/series @@ -131,3 +131,5 @@ bluetooth-l2cap-fix-use-after-free-in-l2cap_sock_ready_cb.patch net-usbnet-fix-warning-in-usbnet_start_xmit-usb_submit_urb.patch fs-protect-reconfiguration-of-sb-read-write-from-racing-writes.patch ext2-drop-fragment-support.patch +test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch +test_firmware-return-enomem-instead-of-enospc-on-failed-memory-allocation.patch diff --git a/queue-5.4/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch b/queue-5.4/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch new file mode 100644 index 00000000000..0f478bae6a5 --- /dev/null +++ b/queue-5.4/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 +@@ -301,16 +301,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; +@@ -340,7 +350,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; +@@ -352,14 +362,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; +@@ -392,10 +411,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; diff --git a/queue-5.4/test_firmware-return-enomem-instead-of-enospc-on-failed-memory-allocation.patch b/queue-5.4/test_firmware-return-enomem-instead-of-enospc-on-failed-memory-allocation.patch new file mode 100644 index 00000000000..052879baefc --- /dev/null +++ b/queue-5.4/test_firmware-return-enomem-instead-of-enospc-on-failed-memory-allocation.patch @@ -0,0 +1,95 @@ +From 7dae593cd226a0bca61201cf85ceb9335cf63682 Mon Sep 17 00:00:00 2001 +From: Mirsad Goran Todorovac +Date: Tue, 6 Jun 2023 09:08:10 +0200 +Subject: test_firmware: return ENOMEM instead of ENOSPC on failed memory allocation + +From: Mirsad Goran Todorovac + +commit 7dae593cd226a0bca61201cf85ceb9335cf63682 upstream. + +In a couple of situations like + + name = kstrndup(buf, count, GFP_KERNEL); + if (!name) + return -ENOSPC; + +the error is not actually "No space left on device", but "Out of memory". + +It is semantically correct to return -ENOMEM in all failed kstrndup() +and kzalloc() cases in this driver, as it is not a problem with disk +space, but with kernel memory allocator failing allocation. + +The semantically correct should be: + + name = kstrndup(buf, count, GFP_KERNEL); + if (!name) + return -ENOMEM; + +Cc: Dan Carpenter +Cc: Takashi Iwai +Cc: Kees Cook +Cc: "Luis R. Rodriguez" +Cc: Scott Branden +Cc: Hans de Goede +Cc: Brian Norris +Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests") +Fixes: 0a8adf584759c ("test: add firmware_class loader test") +Fixes: 548193cba2a7d ("test_firmware: add support for firmware_request_platform") +Fixes: eb910947c82f9 ("test: firmware_class: add asynchronous request trigger") +Fixes: 061132d2b9c95 ("test_firmware: add test custom fallback trigger") +Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf") +Signed-off-by: Mirsad Goran Todorovac +Reviewed-by: Dan Carpenter +Message-ID: <20230606070808.9300-1-mirsad.todorovac@alu.unizg.hr> +Signed-off-by: Greg Kroah-Hartman +--- + lib/test_firmware.c | 10 +++++----- + 1 file changed, 5 insertions(+), 5 deletions(-) + +--- a/lib/test_firmware.c ++++ b/lib/test_firmware.c +@@ -173,7 +173,7 @@ static int __kstrncpy(char **dst, const + { + *dst = kstrndup(name, count, gfp); + if (!*dst) +- return -ENOSPC; ++ return -ENOMEM; + return count; + } + +@@ -509,7 +509,7 @@ static ssize_t trigger_request_store(str + + name = kstrndup(buf, count, GFP_KERNEL); + if (!name) +- return -ENOSPC; ++ return -ENOMEM; + + pr_info("loading '%s'\n", name); + +@@ -552,7 +552,7 @@ static ssize_t trigger_async_request_sto + + name = kstrndup(buf, count, GFP_KERNEL); + if (!name) +- return -ENOSPC; ++ return -ENOMEM; + + pr_info("loading '%s'\n", name); + +@@ -597,7 +597,7 @@ static ssize_t trigger_custom_fallback_s + + name = kstrndup(buf, count, GFP_KERNEL); + if (!name) +- return -ENOSPC; ++ return -ENOMEM; + + pr_info("loading '%s' using custom fallback mechanism\n", name); + +@@ -648,7 +648,7 @@ static int test_fw_run_batch_request(voi + + test_buf = kzalloc(TEST_FIRMWARE_BUF_SIZE, GFP_KERNEL); + if (!test_buf) +- return -ENOSPC; ++ return -ENOMEM; + + req->rc = request_firmware_into_buf(&req->fw, + req->name,