Re: [PATCH v3 1/2] test_firmware: Fix some racing conditions in test_fw_config locking.

[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@xxxxxxxxxxxxxxxxxxx>
> > Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> > Cc: Russ Weight <russell.h.weight@xxxxxxxxx>
> > Cc: Tianfei zhang <tianfei.zhang@xxxxxxxxx>
> > Cc: Mirsad Goran Todorovac <mirsad.todorovac@xxxxxxxxxxxx>
> > Cc: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> > Cc: Zhengchao Shao <shaozhengchao@xxxxxxxxxx>
> > Cc: Colin Ian King <colin.i.king@xxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: Takashi Iwai <tiwai@xxxxxxx>
> > Suggested-by: Dan Carpenter <error27@xxxxxxxxx>
> > Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@xxxxxxxxxxxx>
> > ---
> >   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