Messages in this thread | | | Date | Fri, 7 Apr 2023 10:24:24 +0200 | Subject | Re: [PATCH v3 1/2] test_firmware: Fix some racing conditions in test_fw_config locking. | From | Mirsad Goran Todorovac <> |
| |
On 6.4.2023. 16:04, Dan Carpenter wrote: > On Thu, Apr 06, 2023 at 03:53:17AM +0200, Mirsad Goran Todorovac wrote: >> Some functions were called both from locked and unlocked context, so the lock >> was dropped prematurely, introducing a race condition when deadlock was avoided. >> >> Having two locks wouldn't assure a race-proof mutual exclusion. >> >> test_dev_config_update_bool_unlocked(), test_dev_config_update_u8_unlocked() >> 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 causing a race condition, much like putc() >> and putc_unlocked() in stdio glibc library. >> >> This should guarantee mutual exclusion and prevent any race conditions. >> > > Thanks for figuring this out! It seems like a good approach to me. > However, I feel like PATCH 1/1 needs some style changes. > > The question you seem to be dealing with is how consistent to be and how > much infrastructure to create. Don't think about that. Just fix the > bug in the most minimal way possible and don't worry about being > consistent. > > (Probably the best way to make this consistent is to change the > test_dev_config_update_XXX functions into a single macro that calls the > correct kstroXXX function. Then create a second macro that takes the > lock and calls the first macro. But that is a clean up patch and > unrelated to this bug.) > >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Luis Chamberlain <mcgrof@kernel.org> >> Cc: Russ Weight <russell.h.weight@intel.com> >> Cc: Tianfei zhang <tianfei.zhang@intel.com> >> Cc: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> >> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> Cc: Zhengchao Shao <shaozhengchao@huawei.com> >> Cc: Colin Ian King <colin.i.king@gmail.com> >> Cc: linux-kernel@vger.kernel.org >> Cc: Takashi Iwai <tiwai@suse.de> >> Suggested-by: Dan Carpenter <error27@gmail.com> >> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> >> --- >> lib/test_firmware.c | 52 +++++++++++++++++++++++++++++++++------------ >> 1 file changed, 38 insertions(+), 14 deletions(-) >> >> diff --git a/lib/test_firmware.c b/lib/test_firmware.c >> index 05ed84c2fc4c..272af8dc54b0 100644 >> --- a/lib/test_firmware.c >> +++ b/lib/test_firmware.c >> @@ -353,16 +353,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, >> +static inline int test_dev_config_update_bool_unlocked(const char *buf, size_t size, >> bool *cfg) >> { >> int ret; >> >> - mutex_lock(&test_fw_mutex); >> if (kstrtobool(buf, cfg) < 0) >> ret = -EINVAL; >> else >> ret = size; >> + >> + return ret; >> +} > > This change can be left out completely. > >> + >> +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_unlocked(buf, size, cfg); >> mutex_unlock(&test_fw_mutex); >> >> return ret; >> @@ -373,7 +383,8 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val) >> return snprintf(buf, PAGE_SIZE, "%d\n", val); >> } >> >> -static int test_dev_config_update_size_t(const char *buf, >> +static int test_dev_config_update_size_t_unlocked( >> + const char *buf, >> size_t size, >> size_t *cfg) >> { > > Do not rename this function. Just add a comment that the mutext must be > held. Or a WARN_ONCE(). > > WARN_ON_ONCE(!mutex_is_locked(&test_fw_mutex)); > > >> @@ -384,9 +395,7 @@ static int test_dev_config_update_size_t(const char *buf, >> if (ret) >> return ret; >> >> - mutex_lock(&test_fw_mutex); >> *(size_t *)cfg = new; >> - mutex_unlock(&test_fw_mutex); >> >> /* Always return full write size even if we didn't consume all */ >> return size; >> @@ -402,6 +411,21 @@ static ssize_t test_dev_config_show_int(char *buf, int val) >> return snprintf(buf, PAGE_SIZE, "%d\n", val); >> } >> >> +static int test_dev_config_update_u8_unlocked(const char *buf, size_t size, u8 *cfg) >> +{ >> + u8 val; >> + int ret; >> + >> + ret = kstrtou8(buf, 10, &val); >> + if (ret) >> + return ret; >> + >> + *(u8 *)cfg = val; >> + >> + /* Always return full write size even if we didn't consume all */ >> + return size; >> +} >> + > > Just change the test_dev_config_update_u8() to not take the lock. > Add the comment that the lock must be held. Change both callers to take > the lock. > > > Otherwise we end up creating too much duplicate code. > >> static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) >> { >> u8 val; > > regards, > dan carpenter
Hi Mr. Carpenter,
Thank you for your review.
I will proceed according to your guidelines and issue the next version of the patch set.
But I cannot promise it will be before the holidays - I do not want to make the gods angry either ;-)
I cannot promise to try smart macros or inline functions with smart function parameters just yet.
I would consider the real success if I hunt down the remaining leak and races in this driver. Despite being considered a less important one.
As you have previously asserted, it is not a real security issue with a CVE, however, for completeness sake I would like to see these problems fixed.
Best regards, Mirsad
-- Mirsad Todorovac System engineer Faculty of Graphic Arts | Academy of Fine Arts University of Zagreb Republic of Croatia, the European Union
Sistem inženjer Grafički fakultet | Akademija likovnih umjetnosti Sveučilište u Zagrebu
| |