Re: [PATCH -next] thermal/drivers/thermal_hwmon: Fix a kernel NULL pointer dereference

[Rafael J. Wysocki]

On Wednesday, March 29, 2023 6:18:31 PM CEST Daniel Lezcano wrote:
> On 29/03/2023 18:03, Rafael J. Wysocki wrote:
> > On Wed, Mar 29, 2023 at 5:59 PM Daniel Lezcano
> > <daniel.lezcano@xxxxxxxxxx> wrote:
> >>
> >> On 29/03/2023 16:38, Rafael J. Wysocki wrote:
> >>> On Wed, Mar 29, 2023 at 4:16 PM Daniel Lezcano
> >>> <daniel.lezcano@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 29/03/2023 14:06, Rafael J. Wysocki wrote:
> >>>>> On Wed, Mar 29, 2023 at 11:57 AM Daniel Lezcano
> >>>>> <daniel.lezcano@xxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On 29/03/2023 11:00, Zhang Rui wrote:
> >>>>>>> When the hwmon device node of a thermal zone device is not found,
> >>>>>>> using hwmon->device causes a kernel NULL pointer dereference.
> >>>>>>>
> >>>>>>> Reported-by: Preble Adam C <adam.c.preble@xxxxxxxxx>
> >>>>>>> Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> >>>>>>> ---
> >>>>>>> Fixes: dec07d399cc8 ("thermal: Don't use 'device' internal thermal zone structure field")
> >>>>>>> dec07d399cc8 is a commit in the linux-next branch of linux-pm repo.
> >>>>>>> I'm not sure if the Fix tag applies to such commit or not.
> >>>>>>
> >>>>>> Actually it reverts the work done to encapsulate the thermal zone device
> >>>>>> structure.
> >>>>>
> >>>>> So maybe instead of the wholesale switch to using "driver-specific"
> >>>>> device pointers for printing messages, something like
> >>>>> thermal_zone_debug/info/warn/error() taking a thermal zone pointer as
> >>>>> the first argument can be defined?
> >>>>>
> >>>>> At least this particular bug could be avoided this way.
> >>>>
> >>>> Actually we previously said the thermal_hwmon can be considered as part
> >>>> of the thermal core code, so we can keep using tz->device.
> >>>>
> >>>> I'll drop this change from the series.
> >>>
> >>> But it's there in my thermal branch already.
> >>>
> >>> Do you want to revert the thermal_hwmon.c part of commit dec07d399cc8?
> >>
> >> Oh, right. Fair enough.
> >>
> >> I think Rui's patch is fine then.
> > 
> > I guess you mean the $subject one, that is:
> > 
> > https://patchwork.kernel.org/project/linux-pm/patch/20230329090055.7537-1-rui.zhang@xxxxxxxxx
> 
> Correct
> 
> > What about the message printed when temp is NULL.  Should the original
> > form of it be restored too?
> 
> Yes, you are right, for the sake of consistency we should restore also 
> this one.

So I'm going to apply the appended patch.

Please let me know if there are any concerns regarding it.

---
From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
Subject: [PATCH] thermal: thermal_hwmon: Revert recent message adjustment

For the sake of consistency, revert the second part of the
thermal_hwmon.c hunk from commit dec07d399cc8 ("thermal: Don't use
'device' internal thermal zone structure field") after the first
part of it has been reverted.

Link: https://lore.kernel.org/linux-pm/5b084360-898b-aad0-0b8e-33acc585d71d@xxxxxxxxxx
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
---
 drivers/thermal/thermal_hwmon.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/thermal/thermal_hwmon.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_hwmon.c
+++ linux-pm/drivers/thermal/thermal_hwmon.c
@@ -236,7 +236,7 @@ void thermal_remove_hwmon_sysfs(struct t
 	temp = thermal_hwmon_lookup_temp(hwmon, tz);
 	if (unlikely(!temp)) {
 		/* Should never happen... */
-		dev_dbg(hwmon->device, "temperature input lookup failed!\n");
+		dev_dbg(&tz->device, "temperature input lookup failed!\n");
 		return;
 	}