[v3] backlight: pm8941: Add NULL check in wled_configure()

[PATCH v3] backlight: pm8941: Add NULL check in wled_configure()

Posted by Henry Martin 1 week, 2 days ago

The function wled_configure() uses devm_kasprintf() without checking for
allocation failures, which could lead to NULL pointer dereferences.

Add proper error handling when devm_kasprintf() fails by:
- Returning -ENOMEM immediately
- Ensuring no resources are left allocated (none need cleanup in this case)

Fixes: f86b77583d88 ("backlight: pm8941: Convert to using %pOFn instead of device_node.name")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
V2 -> V3: Fix commit message and verify proper error handling with
resource cleanup.
V1 -> V2: Fix commit message to use imperative mood and wrap lines to 75
characters.

 drivers/video/backlight/qcom-wled.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 9afe701b2a1b..a63bb42c8f8b 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1406,9 +1406,11 @@ static int wled_configure(struct wled *wled)
 	wled->ctrl_addr = be32_to_cpu(*prop_addr);
 
 	rc = of_property_read_string(dev->of_node, "label", &wled->name);
-	if (rc)
+	if (rc) {
 		wled->name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn", dev->of_node);
-
+		if (!wled->name)
+			return -ENOMEM;
+	}
 	switch (wled->version) {
 	case 3:
 		u32_opts = wled3_opts;
-- 
2.34.1

Re: [PATCH v3] backlight: pm8941: Add NULL check in wled_configure()

Posted by Daniel Thompson 1 week, 2 days ago

On Tue, Apr 01, 2025 at 04:29:50PM +0800, Henry Martin wrote:
> The function wled_configure() uses devm_kasprintf() without checking for
> allocation failures, which could lead to NULL pointer dereferences.
>
> Add proper error handling when devm_kasprintf() fails by:
> - Returning -ENOMEM immediately
> - Ensuring no resources are left allocated (none need cleanup in this case)

Two things have gone wrong here:

1. This patch has been incorrectly posted with the wrong patch number.
2. You've let Markus bully you into adding some pointless text in the
   patch description ;-). I think the original v3 was better worded.
   IMHO the bulleted list adds nothing useful.

When you get the chance please send a v4 of the patch so we can pick it
up without any confusion (I suspect it might be a week or two before
this gets pulled so clean mail threads help a lot). It's up to you but
I'd recommend keeping the original v3 wording and label it something
like:
V3 -> V4: No functional changes, just correcting the version number


Daniel.



>
> Fixes: f86b77583d88 ("backlight: pm8941: Convert to using %pOFn instead of device_node.name")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> V2 -> V3: Fix commit message and verify proper error handling with
> resource cleanup.
> V1 -> V2: Fix commit message to use imperative mood and wrap lines to 75
> characters.
>
>  drivers/video/backlight/qcom-wled.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 9afe701b2a1b..a63bb42c8f8b 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1406,9 +1406,11 @@ static int wled_configure(struct wled *wled)
>  	wled->ctrl_addr = be32_to_cpu(*prop_addr);
>
>  	rc = of_property_read_string(dev->of_node, "label", &wled->name);
> -	if (rc)
> +	if (rc) {
>  		wled->name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn", dev->of_node);
> -
> +		if (!wled->name)
> +			return -ENOMEM;
> +	}
>  	switch (wled->version) {
>  	case 3:
>  		u32_opts = wled3_opts;
> --
> 2.34.1
>

Re: [v4?] backlight: pm8941: Add NULL check in wled_configure()

Posted by Markus Elfring 1 week, 2 days ago

…
>    patch description ;-). I think the original v3 was better worded.
…

Can you find the mentioning of adjustments helpful for better error handling?

Regards,
Markus

[PATCH v4] backlight: pm8941: Add NULL check in wled_configure()

Posted by Henry Martin 1 week, 2 days ago

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

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

Fixes: f86b77583d88 ("backlight: pm8941: Convert to using %pOFn instead of device_node.name")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
---
V3 -> V4: No functional changes, just correcting the version number
V2 -> V3: Correct commit meessage and confirm this patch has considered
resource cleanup to avoid any subsequent issues, ensuring that errors
are handled properly and no resources are left in an inconsistent
state.
V1 -> V2: Fix commit message to use imperative mood and wrap lines to 75
characters.

 drivers/video/backlight/qcom-wled.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 9afe701b2a1b..a63bb42c8f8b 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1406,9 +1406,11 @@ static int wled_configure(struct wled *wled)
 	wled->ctrl_addr = be32_to_cpu(*prop_addr);
 
 	rc = of_property_read_string(dev->of_node, "label", &wled->name);
-	if (rc)
+	if (rc) {
 		wled->name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn", dev->of_node);
-
+		if (!wled->name)
+			return -ENOMEM;
+	}
 	switch (wled->version) {
 	case 3:
 		u32_opts = wled3_opts;
-- 
2.34.1

Re: (subset) [PATCH v4] backlight: pm8941: Add NULL check in wled_configure()

Posted by Lee Jones 6 days, 3 hours ago

On Tue, 01 Apr 2025 17:16:47 +0800, Henry Martin wrote:
> devm_kasprintf() returns NULL when memory allocation fails. Currently,
> wled_configure() does not check for this case, which results in a NULL
> pointer dereference.
> 
> Add NULL check after devm_kasprintf() to prevent this issue.
> 
> 
> [...]

Applied, thanks!

[1/1] backlight: pm8941: Add NULL check in wled_configure()
      commit: b0fdeb96ead46de57a6226bc3a3ac7f9b50c0ace

--
Lee Jones [李琼斯]

Re: (subset) [PATCH v4] backlight: pm8941: Add NULL check in wled_configure()

Posted by Lee Jones 6 days, 3 hours ago

On Fri, 04 Apr 2025, Lee Jones wrote:

> On Tue, 01 Apr 2025 17:16:47 +0800, Henry Martin wrote:
> > devm_kasprintf() returns NULL when memory allocation fails. Currently,
> > wled_configure() does not check for this case, which results in a NULL
> > pointer dereference.
> > 
> > Add NULL check after devm_kasprintf() to prevent this issue.
> > 
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/1] backlight: pm8941: Add NULL check in wled_configure()
>       commit: b0fdeb96ead46de57a6226bc3a3ac7f9b50c0ace

Next time, please send subsequent patch versions independently.

-- 
Lee Jones [李琼斯]

Re: (subset) [PATCH v4] backlight: pm8941: Add NULL check in wled_configure()

Posted by henry martin 6 days, 3 hours ago

Noted, thanks for the feedback! I’ll send subsequent versions as
independent patches in the future.

Best regards,
Henry


Lee Jones <lee@kernel.org> 于2025年4月4日周五 23:13写道：
>
> On Fri, 04 Apr 2025, Lee Jones wrote:
>
> > On Tue, 01 Apr 2025 17:16:47 +0800, Henry Martin wrote:
> > > devm_kasprintf() returns NULL when memory allocation fails. Currently,
> > > wled_configure() does not check for this case, which results in a NULL
> > > pointer dereference.
> > >
> > > Add NULL check after devm_kasprintf() to prevent this issue.
> > >
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> > [1/1] backlight: pm8941: Add NULL check in wled_configure()
> >       commit: b0fdeb96ead46de57a6226bc3a3ac7f9b50c0ace
>
> Next time, please send subsequent patch versions independently.
>
> --
> Lee Jones [李琼斯]