linux-kernel - Re: [PATCH] sound: soc: wcd934x: fix an incorrect use of kstrndup()

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: <8c92d33d-923b-4805-abdf-752e8943072e@linux.intel.com>
Date: Tue, 30 Jan 2024 10:56:41 +0100
From: Amadeusz Sławiński
 <amadeuszx.slawinski@...ux.intel.com>
To: Fullway Wang <fullwaywang@...look.com>, broonie@...nel.org,
 srinivas.kandagatla@...aro.org, bgoswami@...cinc.com, tiwai@...e.com
Cc: linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
 fullwaywang@...cent.com
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 <fullwaywang@...look.com>
> ---
>   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/7573d8ce-7160-39b1-8901-be9155c451a1@suse.cz/ 
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/20190612133040.5kgtio7gidxx64gh@xylophone.i.decadent.org.uk/