[PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Gax-c]

A 'devm_kzalloc' in 'asoc_qcom_lpass_cpu_platform_probe' could possibly return NULL pointer.
NULL Pointer Dereference may be triggerred in 'asoc_qcom_lpass_cpu_platform_probe' without addtional check.
Add a null check for the returned pointer.

Fixes: b5022a36d28f ("ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers")
Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
Reported-by: Zichen Xie <zichenxie0106@gmail.com>
Reported-by: Zijie Zhao <zzjas98@gmail.com>
Reported-by: Chenyuan Yang <chenyuan0y@gmail.com>
---
 sound/soc/qcom/lpass-cpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
index 5a47f661e0c6..a8e56f47f237 100644
--- a/sound/soc/qcom/lpass-cpu.c
+++ b/sound/soc/qcom/lpass-cpu.c
@@ -1243,6 +1243,9 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
 	drvdata->i2sctl = devm_kzalloc(&pdev->dev, sizeof(struct lpaif_i2sctl),
 					GFP_KERNEL);
 
+	if (!drvdata->i2sctl)
+		return -ENOMEM;
+
 	/* Initialize bitfields for dai I2SCTL register */
 	ret = lpass_cpu_init_i2sctl_bitfields(dev, drvdata->i2sctl,
 						drvdata->lpaif_map);
-- 
2.25.1

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Markus Elfring]

> A 'devm_kzalloc' in 'asoc_qcom_lpass_cpu_platform_probe' could possibly return NULL pointer.
> NULL Pointer Dereference may be triggerred in 'asoc_qcom_lpass_cpu_platform_probe' without addtional check.
> Add a null check for the returned pointer.

How do you think about a wording variant like the following?

  The result from a call of the function “devm_kzalloc” was passed to
  a subsequent function call without checking for a null pointer before
  (according to a memory allocation failure).
  Thus return directly after a failed devm_kzalloc() call.


…
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> Reported-by: Zichen Xie <zichenxie0106@gmail.com>
…

How good does such a tag combination fit together for the same person?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc1#n525


Can a subject like “[PATCH] ASoC: qcom: lpass-cpu: Return directly after a failed devm_kzalloc() call
in asoc_qcom_lpass_cpu_platform_probe()” be more appropriate?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc1#n613


…
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -1243,6 +1243,9 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  	drvdata->i2sctl = devm_kzalloc(&pdev->dev, sizeof(struct lpaif_i2sctl),
>  					GFP_KERNEL);
>
> +	if (!drvdata->i2sctl)
> +		return -ENOMEM;
…

I suggest to omit a blank line here.


By the way:
Would you become interested to omit the label “err” from this function implementation finally?

Regards,
Markus

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Bjorn Andersson]

On Sun, Sep 29, 2024 at 08:15:22PM -0500, Gax-c wrote:
> A 'devm_kzalloc' in 'asoc_qcom_lpass_cpu_platform_probe' could possibly return NULL pointer.
> NULL Pointer Dereference may be triggerred in 'asoc_qcom_lpass_cpu_platform_probe' without addtional check.
> Add a null check for the returned pointer.
> 

Your description and patch looks good to me.


But please run git log on the changed file and add a prefix to your
subject to match other changes to this file, and please wrap your commit
message at 75 characters.

A good resource to read about this is:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

Please also use the form devm_kzalloc() instead of 'devm_kzalloc' when
referring to kernel functions.

It seems reasonable to mark this for backporting to stable, so I'd also
suggest adding the following tag:

Cc: stable@vger.kernel.org
> Fixes: b5022a36d28f ("ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> Reported-by: Zichen Xie <zichenxie0106@gmail.com>
> Reported-by: Zijie Zhao <zzjas98@gmail.com>
> Reported-by: Chenyuan Yang <chenyuan0y@gmail.com>
> ---
>  sound/soc/qcom/lpass-cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index 5a47f661e0c6..a8e56f47f237 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -1243,6 +1243,9 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  	drvdata->i2sctl = devm_kzalloc(&pdev->dev, sizeof(struct lpaif_i2sctl),
>  					GFP_KERNEL);
>  

Please drop this empty line.

Regards,
Bjorn

> +	if (!drvdata->i2sctl)
> +		return -ENOMEM;
> +
>  	/* Initialize bitfields for dai I2SCTL register */
>  	ret = lpass_cpu_init_i2sctl_bitfields(dev, drvdata->i2sctl,
>  						drvdata->lpaif_map);
> -- 
> 2.25.1
> 
> 
> 
> 
> 

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Mark Brown]

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

On Mon, Sep 30, 2024 at 06:33:49PM +0200, Markus Elfring wrote:

> How do you think about a wording variant like the following?

>   The result from a call of the function “devm_kzalloc” was passed to
>   a subsequent function call without checking for a null pointer before
>   (according to a memory allocation failure).
>   Thus return directly after a failed devm_kzalloc() call.

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

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

Re: Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Markus Elfring]

>> How do you think about a wording variant like the following?
>
>>   The result from a call of the function “devm_kzalloc” was passed to
>>   a subsequent function call without checking for a null pointer before
>>   (according to a memory allocation failure).
>>   Thus return directly after a failed devm_kzalloc() call.
>
> Feel free to ignore Markus, he has a long history of sending
> unhelpful review comments and continues to ignore repeated requests
> to stop.

30 non-merge commits were published with related subjects so far.
Can you get a more constructive view from parts of public software development history?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=v6.12-rc1&qt=grep&q=Return+directly+after+a+failed

Regards,
Markus

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Markus Elfring]

> > A 'devm_kzalloc' in 'asoc_qcom_lpass_cpu_platform_probe' could possibly return NULL pointer.
> > NULL Pointer Dereference may be triggerred in 'asoc_qcom_lpass_cpu_platform_probe' without addtional check.
> > Add a null check for the returned pointer.
>
> Your description and patch looks good to me.

Interesting “view” …


> But please run git log on the changed file and add a prefix to your
> subject to match other changes to this file, and please wrap your commit
> message at 75 characters.

* How does your initial information fit to this patch review advice?

* Would you like to “tolerate” any typos (at the first glance)?


Regards,
Markus

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Mark Brown]

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

On Tue, Oct 01, 2024 at 02:48:54PM +0200, Markus Elfring wrote:

> > Your description and patch looks good to me.

> Interesting “view” …

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

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

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Krzysztof Kozlowski]

On 30/09/2024 03:15, Gax-c wrote:
> A 'devm_kzalloc' in 'asoc_qcom_lpass_cpu_platform_probe' could possibly return NULL pointer.
> NULL Pointer Dereference may be triggerred in 'asoc_qcom_lpass_cpu_platform_probe' without addtional check.
> Add a null check for the returned pointer.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Fixes: b5022a36d28f ("ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> Reported-by: Zichen Xie <zichenxie0106@gmail.com>

Drop the unnecessary tag. You are the author.

> Reported-by: Zijie Zhao <zzjas98@gmail.com>
> Reported-by: Chenyuan Yang <chenyuan0y@gmail.com>

Why two people reported it? And where?

> ---
>  sound/soc/qcom/lpass-cpu.c | 3 +++
>  1 file changed, 3 insertions(+)


Best regards,
Krzysztof