| Message ID | 20240930011521.26283-1-zichenxie0106@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe' | expand |
> 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
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 > > > > >
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.
>> 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
> > 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
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.
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
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);
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(+)