thermal: Fix NULL pointer dereference issue

PATCH In review Mukesh Ojha 3 participants 5 messages Nov 16, 2020 – Nov 17, 2020 Thermal

Share & tools

Mukesh Ojha

author

2020-11-16 16:30 UTC #0 lore raw

PATCH 1/1 thermal: Fix NULL pointer dereference issue

Cooling stats variable inside thermal_cooling_device_stats_update() can get NULL. We should add a NULL check on stat inside for sanity.

Signed-off-by Mukesh Ojha <[email protected]>

---
 drivers/thermal/thermal_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/thermal/thermal_sysfs.c +3 -0

index a6f371f..f52708f 100644
@@ -754,6 +754,9 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 {
 	struct cooling_dev_stats *stats = cdev->stats;
 
+	if (!stats)
+		return;
+
 	spin_lock(&stats->lock);
 
 	if (stats->state == new_state)

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Zhang Rui

reviewer

2020-11-17 07:19 UTC #1 lore raw

Quoted 14 lines · Mukesh Ojha

Cooling stats variable inside thermal_cooling_device_stats_update() can get NULL. We should add a NULL check on stat inside for sanity.
Signed-off-by Mukesh Ojha <[email protected]>
---
 drivers/thermal/thermal_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/thermal/thermal_sysfs.c +0 -0
index a6f371f..f52708f 100644

drivers/thermal/thermal_sysfs.c void thermal_cooling_device_stats_update(struct · line 760

Quoted 7 lines

drivers/thermal/thermal_sysfs.c +3 -0

@@ -754,6 +754,9 @@ void thermal_cooling_device_stats_update(struct
 thermal_cooling_device *cdev,
 {
 	struct cooling_dev_stats *stats = cdev->stats;
 
+	if (!stats)
+		return;
+

May I know in which case stats can be NULL?
The only possibility I can see is that cdev->ops->get_max_state() fails in cooling_device_stats_setup(), right?

thanks,
rui

Quoted 3 lines

drivers/thermal/thermal_sysfs.c +0 -0
 	spin_lock(&stats->lock);
 
if (stats->state == new_state)

Daniel Lezcano

2020-11-17 08:57 UTC #2 lore raw

Quoted 19 lines · Zhang Rui

Quoted 18 lines · Mukesh Ojha
Cooling stats variable inside thermal_cooling_device_stats_update() can get NULL. We should add a NULL check on stat inside for sanity.
Signed-off-by Mukesh Ojha <[email protected]>
---
 drivers/thermal/thermal_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/thermal/thermal_sysfs.c +0 -0
index a6f371f..f52708f 100644
@@ -754,6 +754,9 @@ void thermal_cooling_device_stats_update(struct
 thermal_cooling_device *cdev,
 {
 	struct cooling_dev_stats *stats = cdev->stats;
 

Quoted 6 lines

drivers/thermal/thermal_sysfs.c void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, · line 759
Quoted 3 lines
drivers/thermal/thermal_sysfs.c +3 -0
+	if (!stats)
+		return;
+
May I know in which case stats can be NULL?
The only possibility I can see is that cdev->ops->get_max_state() fails in cooling_device_stats_setup(), right?

A few lines below, the allocation could fail.

        stats = kzalloc(var, GFP_KERNEL);
        if (!stats)
                return;

Some drivers define themselves as a cooling device state to let the userspace to act on their power. The screen brightness is one example with a cdev with 1024 states, the resulting stats table to be allocated is very big and the kzalloc is prone to fail.

Quoted 7 lines

thanks,
rui
Quoted 3 lines
drivers/thermal/thermal_sysfs.c +0 -0
 	spin_lock(&stats->lock);
 
if (stats->state == new_state)

Signature · 5 lines

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Zhang Rui

reviewer

2020-11-17 11:27 UTC #3 lore raw

Quoted 38 lines · Daniel Lezcano

Quoted 22 lines · Zhang Rui
Quoted 21 lines · Mukesh Ojha
Cooling stats variable inside thermal_cooling_device_stats_update()
can get NULL. We should add a NULL check on stat inside for sanity.
Signed-off-by Mukesh Ojha <[email protected]>
---
 drivers/thermal/thermal_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/thermal/thermal_sysfs.c +0 -0
index a6f371f..f52708f 100644
@@ -754,6 +754,9 @@ void
thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
{
struct cooling_dev_stats *stats = cdev->stats;
Quoted 8 lines
drivers/thermal/thermal_sysfs.c void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, · line 759
Quoted 3 lines
drivers/thermal/thermal_sysfs.c +3 -0
+	if (!stats)
+		return;
+
May I know in which case stats can be NULL?
The only possibility I can see is that cdev->ops->get_max_state() fails
in cooling_device_stats_setup(), right?
A few lines below, the allocation could fail.
        stats = kzalloc(var, GFP_KERNEL);
        if (!stats)
                return;

Quoted 6 lines

Some drivers define themselves as a cooling device state to let the userspace to act on their power. The screen brightness is one example with a cdev with 1024 states, the resulting stats table to be allocated
is very big and the kzalloc is prone to fail.

Oh, right.
As we're not going to fix the cdev, so I think we do need this patch, right?

thanks,
rui

Quoted 8 lines

Quoted 6 lines
thanks,
rui
Quoted 3 lines
drivers/thermal/thermal_sysfs.c +0 -0
 	spin_lock(&stats->lock);
 
 	if (stats->state == new_state)

Daniel Lezcano

2020-11-17 12:09 UTC #4 lore raw

Quoted 45 lines · Zhang Rui

Quoted 38 lines · Daniel Lezcano
Quoted 22 lines · Zhang Rui
Quoted 21 lines · Mukesh Ojha
Cooling stats variable inside thermal_cooling_device_stats_update()
can get NULL. We should add a NULL check on stat inside for sanity.
Signed-off-by Mukesh Ojha <[email protected]>
---
 drivers/thermal/thermal_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/thermal/thermal_sysfs.c +0 -0
index a6f371f..f52708f 100644
@@ -754,6 +754,9 @@ void
thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
{
struct cooling_dev_stats *stats = cdev->stats;
Quoted 8 lines
drivers/thermal/thermal_sysfs.c void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, · line 759
Quoted 3 lines
drivers/thermal/thermal_sysfs.c +3 -0
+	if (!stats)
+		return;
+
May I know in which case stats can be NULL?
The only possibility I can see is that cdev->ops->get_max_state() fails
in cooling_device_stats_setup(), right?
A few lines below, the allocation could fail.
        stats = kzalloc(var, GFP_KERNEL);
        if (!stats)
                return;
Quoted 6 lines
Some drivers define themselves as a cooling device state to let the userspace to act on their power. The screen brightness is one example with a cdev with 1024 states, the resulting stats table to be allocated
is very big and the kzalloc is prone to fail.

Quoted 3 lines

Oh, right.
As we're not going to fix the cdev, so I think we do need this patch, right?

If the allocation fails at this level if initialization there is clearly something wrong. I'm wondering if it would make sense to report back an error and make thermal_cooling_device_register to fail.

Having an allocation failing and silently ignore it sounds like not very robust IMO.

Signature · 5 lines

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog