linux-kernel - Re: [PATCH] ASoC: 88pm860x: refactor deprecated strncpy

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] [day] [month] [year] [list]

Message-ID: <202307281205.175FD2FC@keescook>
Date:   Fri, 28 Jul 2023 14:05:02 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Justin Stitt <justinstitt@...gle.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ASoC: 88pm860x: refactor deprecated strncpy

On Thu, Jul 27, 2023 at 10:46:13PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on its destination buffer argument which is
> _not_ always the case for `strncpy`!
> 
> In this case, though, there was care taken to ensure that the
> destination buffer would be NUL-terminated. The destination buffer is
> zero-initialized and each `pm860x->name[i]` has a size of
> `MAX_NAME_LENGTH + 1`. This means that there is unlikely to be a bug
> here.
> 
> However, in an attempt to eliminate the usage of the `strncpy` API as
> well as disambiguate implementations, replacements such as: `strscpy`,
> `strscpy_pad`, `strtomem` and `strtomem_pad` should be preferred.
> 
> We are able to eliminate the need for `len + 1` since `strscpy`
> guarantees NUL-termination for its destination buffer as per its
> implementation [3]:
> 
> |       /* Hit buffer length without finding a NUL; force NUL-termination. */
> |       if (res)
> | 	        dest[res-1] = '\0';
> 
> [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [3]: https://elixir.bootlin.com/linux/v6.3/source/lib/string.c#L183
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Justin Stitt <justinstitt@...gle.com>
> ---
>  sound/soc/codecs/88pm860x-codec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/88pm860x-codec.c b/sound/soc/codecs/88pm860x-codec.c
> index 3574c68e0dda..d99b674d574b 100644
> --- a/sound/soc/codecs/88pm860x-codec.c
> +++ b/sound/soc/codecs/88pm860x-codec.c
> @@ -143,7 +143,7 @@ struct pm860x_priv {
>  	struct pm860x_det	det;
>  
>  	int			irq[4];
> -	unsigned char		name[4][MAX_NAME_LEN+1];
> +	unsigned char		name[4][MAX_NAME_LEN];
>  };
>  
>  /* -9450dB to 0dB in 150dB steps ( mute instead of -9450dB) */
> @@ -1373,7 +1373,7 @@ static int pm860x_codec_probe(struct platform_device *pdev)
>  			return -EINVAL;
>  		}
>  		pm860x->irq[i] = res->start + chip->irq_base;
> -		strncpy(pm860x->name[i], res->name, MAX_NAME_LEN);
> +		strscpy(pm860x->name[i], res->name, MAX_NAME_LEN);

res->name is (perhaps) unbounded in length:

struct resource {
	...
        const char *name;
	...
};

So reducing struct pm860x_priv::name's size _might_ have a user-visible
effect, but probably not.

Reviewed-by: Kees Cook <keescook@...omium.org>

-Kees

-- 
Kees Cook