| Message ID | PH7PR20MB59255EF9DFFB022CB1BBB574BF712@PH7PR20MB5925.namprd20.prod.outlook.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | eeab239d6a2418fc5d2cd7ea76187085a97acde0 |
| Headers | show |
| Series | sound: soc: wcd934x: fix an incorrect use of kstrndup() | expand |
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.
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/
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
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().
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.
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;
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(-)