lkml.org 
[lkml]   [2023]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 1/2] test_firmware: Fix some racing conditions in test_fw_config locking.
From
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

\
 
 \ /
  Last update: 2023-04-07 10:25    [W:0.081 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site