From 866ba1e619400fa6b2b57bfaab0615c5ddeef5de Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Mon, 21 Aug 2023 15:21:56 +0200 Subject: [PATCH] 4.14-stable patches added patches: binder-fix-memory-leak-in-binder_init.patch test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch --- ...inder-fix-memory-leak-in-binder_init.patch | 60 +++++ queue-4.14/series | 2 + ...-a-correct-implementation-of-locking.patch | 205 ++++++++++++++++++ 3 files changed, 267 insertions(+) create mode 100644 queue-4.14/binder-fix-memory-leak-in-binder_init.patch create mode 100644 queue-4.14/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch diff --git a/queue-4.14/binder-fix-memory-leak-in-binder_init.patch b/queue-4.14/binder-fix-memory-leak-in-binder_init.patch new file mode 100644 index 00000000000..d64f03b10f0 --- /dev/null +++ b/queue-4.14/binder-fix-memory-leak-in-binder_init.patch @@ -0,0 +1,60 @@ +From adb9743d6a08778b78d62d16b4230346d3508986 Mon Sep 17 00:00:00 2001 +From: Qi Zheng +Date: Sun, 25 Jun 2023 15:49:37 +0000 +Subject: binder: fix memory leak in binder_init() + +From: Qi Zheng + +commit adb9743d6a08778b78d62d16b4230346d3508986 upstream. + +In binder_init(), the destruction of binder_alloc_shrinker_init() is not +performed in the wrong path, which will cause memory leaks. So this commit +introduces binder_alloc_shrinker_exit() and calls it in the wrong path to +fix that. + +Signed-off-by: Qi Zheng +Acked-by: Carlos Llamas +Fixes: f2517eb76f1f ("android: binder: Add global lru shrinker to binder") +Cc: stable +Link: https://lore.kernel.org/r/20230625154937.64316-1-qi.zheng@linux.dev +[cmllamas: resolved trivial merge conflicts] +Signed-off-by: Carlos Llamas +Signed-off-by: Greg Kroah-Hartman +--- + drivers/android/binder.c | 1 + + drivers/android/binder_alloc.c | 6 ++++++ + drivers/android/binder_alloc.h | 1 + + 3 files changed, 8 insertions(+) + +--- a/drivers/android/binder.c ++++ b/drivers/android/binder.c +@@ -5658,6 +5658,7 @@ err_init_binder_device_failed: + + err_alloc_device_names_failed: + debugfs_remove_recursive(binder_debugfs_dir_entry_root); ++ binder_alloc_shrinker_exit(); + + return ret; + } +--- a/drivers/android/binder_alloc.c ++++ b/drivers/android/binder_alloc.c +@@ -1033,3 +1033,9 @@ void binder_alloc_shrinker_init(void) + list_lru_init(&binder_alloc_lru); + register_shrinker(&binder_shrinker); + } ++ ++void binder_alloc_shrinker_exit(void) ++{ ++ unregister_shrinker(&binder_shrinker); ++ list_lru_destroy(&binder_alloc_lru); ++} +--- a/drivers/android/binder_alloc.h ++++ b/drivers/android/binder_alloc.h +@@ -128,6 +128,7 @@ extern struct binder_buffer *binder_allo + int is_async); + extern void binder_alloc_init(struct binder_alloc *alloc); + void binder_alloc_shrinker_init(void); ++extern void binder_alloc_shrinker_exit(void); + extern void binder_alloc_vma_close(struct binder_alloc *alloc); + extern struct binder_buffer * + binder_alloc_prepare_to_free(struct binder_alloc *alloc, diff --git a/queue-4.14/series b/queue-4.14/series index 4e2783daf49..1082e03597a 100644 --- a/queue-4.14/series +++ b/queue-4.14/series @@ -30,3 +30,5 @@ alsa-usb-audio-add-support-for-mythware-xa001au-capture-and-playback-interfaces. cifs-release-folio-lock-on-fscache-read-hit.patch mmc-wbsd-fix-double-mmc_free_host-in-wbsd_init.patch serial-8250-fix-oops-for-port-pm-on-uart_change_pm.patch +binder-fix-memory-leak-in-binder_init.patch +test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch diff --git a/queue-4.14/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch b/queue-4.14/test_firmware-prevent-race-conditions-by-a-correct-implementation-of-locking.patch new file mode 100644 index 00000000000..7d56debf6c0 --- /dev/null +++ b/queue-4.14/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 +@@ -283,16 +283,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; +@@ -322,7 +332,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; +@@ -334,14 +344,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; +@@ -374,10 +393,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; -- 2.47.3