ASoC: Intel: avs: Add NULL check in avs_component_probe()

[PATCH v1] ASoC: Intel: avs: Add NULL check in avs_component_probe()

Posted by Henry Martin 10 months, 1 week ago

devm_kasprintf() returns NULL when memory allocation fails. Currently,
avs_component_probe() does not check for this case, which results in a
NULL pointer dereference.

Add NULL check after devm_kasprintf() to prevent this issue.

Fixes: 739c031110da ("ASoC: Intel: avs: Provide support for fallback topology")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 sound/soc/intel/avs/pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
index dac463390da1..7072bcf4e56f 100644
--- a/sound/soc/intel/avs/pcm.c
+++ b/sound/soc/intel/avs/pcm.c
@@ -927,7 +927,8 @@ static int avs_component_probe(struct snd_soc_component *component)
 		else
 			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
 							     "hda-generic-tplg.bin");
-
+		if (!mach->tplg_filename)
+			return -ENOMEM;
 		filename = kasprintf(GFP_KERNEL, "%s/%s", component->driver->topology_name_prefix,
 				     mach->tplg_filename);
 		if (!filename)
-- 
2.34.1

Re: [PATCH v1] ASoC: Intel: avs: Add NULL check in avs_component_probe()

Posted by Cezary Rojewski 10 months, 1 week ago

On 2025-04-01 4:32 PM, Henry Martin wrote:
> devm_kasprintf() returns NULL when memory allocation fails. Currently,
> avs_component_probe() does not check for this case, which results in a
> NULL pointer dereference.
> 
> Add NULL check after devm_kasprintf() to prevent this issue.

This basically repeats the title, I'd suggest to drop this very line. In 
regard to the title, I believe:

	ASoC: Intel: avs: Fix null-ptr-deref in avs_component_probe()

is more straightforward. That's what you're doing here afterall, fixing 
stuff.

> 
> Fixes: 739c031110da ("ASoC: Intel: avs: Provide support for fallback topology")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
Good catch, thanks for sending the fix, Martin. I'm rather strict when 
it comes to wording title/message but nothing more than a nitpick in 
this case.

Whether you would be willing to provide v2 or not, feel free to add my tag:

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>

> ---
>   sound/soc/intel/avs/pcm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
> index dac463390da1..7072bcf4e56f 100644
> --- a/sound/soc/intel/avs/pcm.c
> +++ b/sound/soc/intel/avs/pcm.c
> @@ -927,7 +927,8 @@ static int avs_component_probe(struct snd_soc_component *component)
>   		else
>   			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
>   							     "hda-generic-tplg.bin");
> -
> +		if (!mach->tplg_filename)
> +			return -ENOMEM;
>   		filename = kasprintf(GFP_KERNEL, "%s/%s", component->driver->topology_name_prefix,
>   				     mach->tplg_filename);
>   		if (!filename)

[PATCH v2] ASoC: Intel: avs: Fix null-ptr-deref in avs_component_probe()

Posted by Henry Martin 10 months, 1 week ago

devm_kasprintf() returns NULL when memory allocation fails. Currently,
avs_component_probe() does not check for this case, which results in a
NULL pointer dereference.

Fixes: 739c031110da ("ASoC: Intel: avs: Provide support for fallback topology")
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Ethan Carter Edwards <ethan@ethancedwards.com>
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
V1 -> V2: No functional changes, just correct title and redundant commit
message.

 sound/soc/intel/avs/pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
index dac463390da1..7072bcf4e56f 100644
--- a/sound/soc/intel/avs/pcm.c
+++ b/sound/soc/intel/avs/pcm.c
@@ -927,7 +927,8 @@ static int avs_component_probe(struct snd_soc_component *component)
 		else
 			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
 							     "hda-generic-tplg.bin");
-
+		if (!mach->tplg_filename)
+			return -ENOMEM;
 		filename = kasprintf(GFP_KERNEL, "%s/%s", component->driver->topology_name_prefix,
 				     mach->tplg_filename);
 		if (!filename)
-- 
2.34.1

Re: [PATCH v2] ASoC: Intel: avs: Fix null-ptr-deref in avs_component_probe()

Posted by Mark Brown 10 months ago

On Wed, 02 Apr 2025 22:14:11 +0800, Henry Martin wrote:
> devm_kasprintf() returns NULL when memory allocation fails. Currently,
> avs_component_probe() does not check for this case, which results in a
> NULL pointer dereference.
> 
> 

Applied to

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

Thanks!

[1/1] ASoC: Intel: avs: Fix null-ptr-deref in avs_component_probe()
      commit: 95f723cf141b95e3b3a5b92cf2ea98a863fe7275

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 v2] ASoC: Intel: avs: Fix null-ptr-deref in avs_component_probe()

Posted by Markus Elfring 10 months, 1 week ago

> devm_kasprintf() returns NULL when memory allocation fails. Currently,
…
                                                       failed?

Can a summary phrase like “Prevent null pointer dereference
in avs_component_probe()” a bit nicer?


…
> +++ b/sound/soc/intel/avs/pcm.c
> @@ -927,7 +927,8 @@ static int avs_component_probe(struct snd_soc_component *component)
>  		else
>  			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
>  							     "hda-generic-tplg.bin");
> -
> +		if (!mach->tplg_filename)
> +			return -ENOMEM;

Can a blank line be desirable after such a statement?


Would another source code transformation become helpful with an additional update step?

-		if (((vendor_id >> 16) & 0xFFFF) == 0x8086)
-			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
-							     "hda-8086-generic-tplg.bin");
-		else
-			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
-							     "hda-generic-tplg.bin");
+		mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
+						     (((vendor_id >> 16) & 0xFFFF) == 0x8086)
+						     ? "hda-8086-generic-tplg.bin"
+						     : "hda-generic-tplg.bin");


Regards,
Markus

Re: [PATCH v2] ASoC: Intel: avs: Fix null-ptr-deref in avs_component_probe()

Posted by Mark Brown 10 months, 1 week ago

On Wed, Apr 02, 2025 at 09:18:27PM +0200, Markus Elfring wrote:
> > devm_kasprintf() returns NULL when memory allocation fails. Currently,
> …
>                                                        failed?
> 
> Can a summary phrase like “Prevent null pointer dereference
> in avs_component_probe()” a bit nicer?

Feel free to ignore Markus, he has a long history of sending
unhelpful review comments and continues to ignore repeated requests
to stop.

Re: [PATCH] ASoC: Intel: avs: Add NULL check in avs_component_probe()

Posted by Markus Elfring 10 months, 1 week ago

> devm_kasprintf() returns NULL when memory allocation fails. Currently,
…
                call?                                  failed?


> Add NULL check after devm_kasprintf() to prevent this issue.

Do you generally propose here to improve the error handling?


…
> +++ b/sound/soc/intel/avs/pcm.c
> @@ -927,7 +927,8 @@ static int avs_component_probe(struct snd_soc_component *component)
>  		else
>  			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
>  							     "hda-generic-tplg.bin");
> -
> +		if (!mach->tplg_filename)
> +			return -ENOMEM;

Can a blank line be desirable after such a statement?


Would another source code transformation become helpful with an additional update step?

-		if (((vendor_id >> 16) & 0xFFFF) == 0x8086)
-			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
-							     "hda-8086-generic-tplg.bin");
-		else
-			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
-							     "hda-generic-tplg.bin");
+		mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
+						     (((vendor_id >> 16) & 0xFFFF) == 0x8086)
+						     ? "hda-8086-generic-tplg.bin"
+						     : "hda-generic-tplg.bin");


Regards,
Markus

Re: [PATCH] ASoC: Intel: avs: Add NULL check in avs_component_probe()

Posted by Mark Brown 10 months, 1 week ago

On Wed, Apr 02, 2025 at 02:00:31PM +0200, Markus Elfring wrote:
> > devm_kasprintf() returns NULL when memory allocation fails. Currently,
> …
>                 call?                                  failed?
> 
> 
> > Add NULL check after devm_kasprintf() to prevent this issue.
> 
> Do you generally propose here to improve the error handling?

Feel free to ignore Markus, he has a long history of sending
unhelpful review comments and continues to ignore repeated requests
to stop.

Re: [PATCH] ASoC: Intel: avs: Add NULL check in avs_component_probe()

Posted by Ethan Carter Edwards 10 months, 1 week ago

On 25/04/02 02:00PM, Markus Elfring wrote:
> > devm_kasprintf() returns NULL when memory allocation fails. Currently,
> …
>                 call?                                  failed?
> 
> 
> > Add NULL check after devm_kasprintf() to prevent this issue.
> 
> Do you generally propose here to improve the error handling?
> 
> 
> …
> > +++ b/sound/soc/intel/avs/pcm.c
> > @@ -927,7 +927,8 @@ static int avs_component_probe(struct snd_soc_component *component)
> >  		else
> >  			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
> >  							     "hda-generic-tplg.bin");
> > -
> > +		if (!mach->tplg_filename)
> > +			return -ENOMEM;
> 
> Can a blank line be desirable after such a statement?
> 
> 
> Would another source code transformation become helpful with an additional update step?
> 
> -		if (((vendor_id >> 16) & 0xFFFF) == 0x8086)
> -			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
> -							     "hda-8086-generic-tplg.bin");
> -		else
> -			mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
> -							     "hda-generic-tplg.bin");
> +		mach->tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL,
> +						     (((vendor_id >> 16) & 0xFFFF) == 0x8086)
> +						     ? "hda-8086-generic-tplg.bin"
> +						     : "hda-generic-tplg.bin");
> 
> 
> Regards,
> Markus


Please feel free to ignore Markus. He has a tendency to provide unclear
and nonsense feedback. He has done this several times with my patch
sets. I almost wonder if he is an internet troll.

In any case, this change makes sense to me.

Reviewed-by: Ethan Carter Edwards <ethan@ethancedwards.com>

Re: ASoC: Intel: avs: Add NULL check in avs_component_probe()

Posted by Markus Elfring 10 months, 1 week ago

> Please feel free to ignore Markus. He has a tendency to provide unclear
> and nonsense feedback. He has done this several times with my patch
> sets. I almost wonder if he is an internet troll.
Can any of the published contributions change your mind?

Regards,
Markus