[PATCH] sound: soc: wcd934x: fix an incorrect use of kstrndup()

[PATCH] sound: soc: wcd934x: fix an incorrect use of kstrndup()

[Fullway Wang]

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@outlook.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;
 
-- 
2.39.3 (Apple Git-145)

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On Thu, Jan 18, 2024 at 03:52:49PM +0800, 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.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Amadeusz Sławiński]

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@outlook.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/

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

[Mark Brown]

On Thu, 18 Jan 2024 15:52:49 +0800, 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.
> 
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] sound: soc: wcd934x: fix an incorrect use of kstrndup()
      commit: eeab239d6a2418fc5d2cd7ea76187085a97acde0

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Amadeusz Sławiński]

On 1/30/2024 4:43 PM, Mark Brown wrote:
> On Thu, 18 Jan 2024 15:52:49 +0800, 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.
>>
>>
>> [...]
> 
> Applied to
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
> 
> Thanks!
> 

Hi,

Mark, my other comment was meant to stop this patch from being applied 
;), perhaps I could have been more clear? kmemdup_nul() in this case 
will copy bytes behind the end of widget name when copying. Widgets to 
which it applies are named: "ADX MUX0", "ADC MUX1" and so on, until "ADC 
MUX 8", which is 10 bytes including '\0', and kmemdup_nul() will copy 15 
using memcpy().

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Thu, Feb 01, 2024 at 10:04:23AM +0100, Amadeusz Sławiński wrote:

> Mark, my other comment was meant to stop this patch from being applied ;),
> perhaps I could have been more clear? kmemdup_nul() in this case will copy

Your comment appeared to be a complaint about the existing code being
bad which sure but not a blocker to a minor fix.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]