2024-01-30 16:00:32

by Amadeusz Sławiński

[permalink] [raw]
Subject: Re: [PATCH] sound: soc: wcd934x: fix an incorrect use of kstrndup()

On 1/18/2024 8:52 AM, Fullway Wang wrote:
> In wcd934x_codec_enable_dec(), kstrndup() is used to alloc memory.
> However, kmemdup_nul() should be used instead with the size known.
>
> This is similar to CVE-2019-12454 which was fixed in commit
> a549881.
>
> Signed-off-by: Fullway Wang <[email protected]>
> ---
> sound/soc/codecs/wcd934x.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c
> index 1b6e376f3833..4565a0e1877f 100644
> --- a/sound/soc/codecs/wcd934x.c
> +++ b/sound/soc/codecs/wcd934x.c
> @@ -4990,7 +4990,7 @@ static int wcd934x_codec_enable_dec(struct snd_soc_dapm_widget *w,
> char *dec;
> u8 hpf_coff_freq;
>
> - widget_name = kstrndup(w->name, 15, GFP_KERNEL);
> + widget_name = kmemdup_nul(w->name, 15, GFP_KERNEL);
> if (!widget_name)
> return -ENOMEM;
>

This change looks weird to me, and looking at code I'm even more
confused. As far as I can tell code tries to find a number in w->name.
It shouldn't need a duplicate string for this, it can search in existing
one, same applies to referenced commit.

And when it comes to a549881 as already mentioned in
https://lore.kernel.org/alsa-devel/[email protected]/
the size is at most 15 not equal to, so change was misguided. If you
look at actual code widget name is around 8 characters.

Also it seems like consensus was that CVE was bogus and it was a wrong
change:
https://lore.kernel.org/alsa-devel/20190618230527.GE6248@lindsey/
there was a revert send, but it seems like it was not applied:
https://lore.kernel.org/alsa-devel/[email protected]/