linux-kernel - Re: [PATCH v3 1/2] test_firmware: Fix some racing conditions in test_fw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <eb117a50-79ad-4f5a-8ad9-73247107469e@kili.mountain>
Date:   Thu, 6 Apr 2023 17:04:19 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
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 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