[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1f07fd79-b7db-9bd5-f281-8ba1ca71e195@alu.unizg.hr>
Date: Fri, 7 Apr 2023 10:24:24 +0200
From: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
To: Dan Carpenter <error27@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Russ Weight <russell.h.weight@...el.com>,
linux-kernel@...r.kernel.org, Luis Chamberlain <mcgrof@...nel.org>,
Tianfei zhang <tianfei.zhang@...el.com>,
Christophe JAILLET <christophe.jaillet@...adoo.fr>,
Zhengchao Shao <shaozhengchao@...wei.com>,
Colin Ian King <colin.i.king@...il.com>,
Takashi Iwai <tiwai@...e.de>
Subject: Re: [PATCH v3 1/2] test_firmware: Fix some racing conditions in
test_fw_config locking.
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@...uxfoundation.org>
>> Cc: Luis Chamberlain <mcgrof@...nel.org>
>> Cc: Russ Weight <russell.h.weight@...el.com>
>> Cc: Tianfei zhang <tianfei.zhang@...el.com>
>> Cc: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
>> Cc: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> Cc: Zhengchao Shao <shaozhengchao@...wei.com>
>> Cc: Colin Ian King <colin.i.king@...il.com>
>> Cc: linux-kernel@...r.kernel.org
>> Cc: Takashi Iwai <tiwai@...e.de>
>> Suggested-by: Dan Carpenter <error27@...il.com>
>> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@....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
Powered by blists - more mailing lists