[PATCH v2] thermal/drivers/rcar_thermal: fail probe when no sensors are probed

[PATCH v2] thermal/drivers/rcar_thermal: fail probe when no sensors are probed

[Li Ximo]

From: Wang Haoran <haoranwangsec@gmail.com>

rcar_thermal_probe() returns success even when zero sensors were probed
(the per-sensor loop simply falls through with i == 0), leaving
common->head empty. rcar_thermal_suspend()/rcar_thermal_resume() then
call list_first_entry(&common->head, ...) unconditionally, and
list_first_entry() on an empty list does not return NULL: it returns
container_of() of the list head, i.e. a wild pointer into
struct rcar_thermal_common, which is dereferenced at
priv->chip->needs_suspend_resume and in the MMIO accesses that follow.
A malformed DT node with no MEM resource triggers this at every
suspend/resume cycle.

As suggested by Niklas Söderlund, fix this at the root instead of
tolerating an empty list in the PM callbacks: fail probe with -ENODEV
when no sensor was probed, since a successfully bound driver with zero
sensors is meaningless. The error path through rcar_thermal_remove()
already tolerates the empty list (it iterates with
rcar_thermal_for_each_priv()).

This issue was found by static analysis; we do not have R-Car hardware,
so this is compile-tested only.

Reported-by: Wang Haoran <haoranwangsec@gmail.com>
Reported-by: Li Ximo <lixicaicai@gmail.com>
Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>
---
v1: https://lore.kernel.org/r/20261026115018.3227372-1-lixicaicai@gmail.com
v2: completely rework the fix per Niklas Söderlund's review: fail probe
with -ENODEV when no sensors are probed instead of guarding
suspend/resume.

 drivers/thermal/renesas/rcar_thermal.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/thermal/renesas/rcar_thermal.c b/drivers/thermal/renesas/rcar_thermal.c
index 9a9038671..6a1e977d8 100644
--- a/drivers/thermal/renesas/rcar_thermal.c
+++ b/drivers/thermal/renesas/rcar_thermal.c
@@ -514,6 +514,12 @@ static int rcar_thermal_probe(struct platform_device *pdev)
 			enr_bits |= 3 << (i * 8);
 	}
 
+	if (!i) {
+		dev_err(dev, "no thermal sensor probed\n");
+		ret = -ENODEV;
+		goto error_unregister;
+	}
+
 	if (common->base && enr_bits)
 		rcar_thermal_common_write(common, ENR, enr_bits);
 
-- 
2.30.2

Re: [PATCH v2] thermal/drivers/rcar_thermal: fail probe when no sensors are probed

[Niklas Söderlund]

Hi Li,

Thanks for your work.

On 2026-11-03 16:46:04 +0800, Li Ximo wrote:
> From: Wang Haoran <haoranwangsec@gmail.com>
> 
> rcar_thermal_probe() returns success even when zero sensors were probed
> (the per-sensor loop simply falls through with i == 0), leaving
> common->head empty. rcar_thermal_suspend()/rcar_thermal_resume() then
> call list_first_entry(&common->head, ...) unconditionally, and
> list_first_entry() on an empty list does not return NULL: it returns
> container_of() of the list head, i.e. a wild pointer into
> struct rcar_thermal_common, which is dereferenced at
> priv->chip->needs_suspend_resume and in the MMIO accesses that follow.
> A malformed DT node with no MEM resource triggers this at every
> suspend/resume cycle.
> 
> As suggested by Niklas Söderlund, fix this at the root instead of
> tolerating an empty list in the PM callbacks: fail probe with -ENODEV
> when no sensor was probed, since a successfully bound driver with zero
> sensors is meaningless. The error path through rcar_thermal_remove()
> already tolerates the empty list (it iterates with
> rcar_thermal_for_each_priv()).
> 
> This issue was found by static analysis; we do not have R-Car hardware,
> so this is compile-tested only.
> 
> Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> Reported-by: Li Ximo <lixicaicai@gmail.com>
> Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>
> ---
> v1: https://lore.kernel.org/r/20261026115018.3227372-1-lixicaicai@gmail.com
> v2: completely rework the fix per Niklas Söderlund's review: fail probe
> with -ENODEV when no sensors are probed instead of guarding
> suspend/resume.
> 
>  drivers/thermal/renesas/rcar_thermal.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/thermal/renesas/rcar_thermal.c b/drivers/thermal/renesas/rcar_thermal.c
> index 9a9038671..6a1e977d8 100644
> --- a/drivers/thermal/renesas/rcar_thermal.c
> +++ b/drivers/thermal/renesas/rcar_thermal.c
> @@ -514,6 +514,12 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>  			enr_bits |= 3 << (i * 8);
>  	}
>  
> +	if (!i) {

While this works I think it's fragile as it depends on the loop variable 
of the last loop. If the loop is changed, or a new one added, this check 
can break. Why not make it robust and check if any sensors where added 
to the list containing sensors (common->head)?

> +		dev_err(dev, "no thermal sensor probed\n");
> +		ret = -ENODEV;
> +		goto error_unregister;
> +	}
> +
>  	if (common->base && enr_bits)
>  		rcar_thermal_common_write(common, ENR, enr_bits);
>  
> -- 
> 2.30.2

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2] thermal/drivers/rcar_thermal: fail probe when no sensors are probed

[Geert Uytterhoeven]

Hi Niklas,

On Wed, 4 Nov 2026 at 13:08, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2026-11-03 16:46:04 +0800, Li Ximo wrote:
> > From: Wang Haoran <haoranwangsec@gmail.com>
> >
> > rcar_thermal_probe() returns success even when zero sensors were probed
> > (the per-sensor loop simply falls through with i == 0), leaving
> > common->head empty. rcar_thermal_suspend()/rcar_thermal_resume() then
> > call list_first_entry(&common->head, ...) unconditionally, and
> > list_first_entry() on an empty list does not return NULL: it returns
> > container_of() of the list head, i.e. a wild pointer into
> > struct rcar_thermal_common, which is dereferenced at
> > priv->chip->needs_suspend_resume and in the MMIO accesses that follow.
> > A malformed DT node with no MEM resource triggers this at every
> > suspend/resume cycle.
> >
> > As suggested by Niklas Söderlund, fix this at the root instead of
> > tolerating an empty list in the PM callbacks: fail probe with -ENODEV
> > when no sensor was probed, since a successfully bound driver with zero
> > sensors is meaningless. The error path through rcar_thermal_remove()
> > already tolerates the empty list (it iterates with
> > rcar_thermal_for_each_priv()).
> >
> > This issue was found by static analysis; we do not have R-Car hardware,
> > so this is compile-tested only.
> >
> > Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> > Reported-by: Li Ximo <lixicaicai@gmail.com>
> > Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>
> > ---
> > v1: https://lore.kernel.org/r/20261026115018.3227372-1-lixicaicai@gmail.com
> > v2: completely rework the fix per Niklas Söderlund's review: fail probe
> > with -ENODEV when no sensors are probed instead of guarding
> > suspend/resume.
> >
> >  drivers/thermal/renesas/rcar_thermal.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/thermal/renesas/rcar_thermal.c b/drivers/thermal/renesas/rcar_thermal.c
> > index 9a9038671..6a1e977d8 100644
> > --- a/drivers/thermal/renesas/rcar_thermal.c
> > +++ b/drivers/thermal/renesas/rcar_thermal.c
> > @@ -514,6 +514,12 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> >                       enr_bits |= 3 << (i * 8);
> >       }
> >
> > +     if (!i) {
>
> While this works I think it's fragile as it depends on the loop variable
> of the last loop. If the loop is changed, or a new one added, this check
> can break. Why not make it robust and check if any sensors where added
> to the list containing sensors (common->head)?

Yeah, then we could move the loop variable declarations inside the
for-statements...

> > +             dev_err(dev, "no thermal sensor probed\n");
> > +             ret = -ENODEV;
> > +             goto error_unregister;
> > +     }
> > +
> >       if (common->base && enr_bits)
> >               rcar_thermal_common_write(common, ENR, enr_bits);

... if not for the next line:

    dev_info(dev, "%d sensor probed\n", i);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] thermal/drivers/rcar_thermal: fail probe when no sensors are probed

[Niklas Söderlund]

Hi Geert,

On 2026-11-04 14:14:20 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Wed, 4 Nov 2026 at 13:08, Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> > On 2026-11-03 16:46:04 +0800, Li Ximo wrote:
> > > From: Wang Haoran <haoranwangsec@gmail.com>
> > >
> > > rcar_thermal_probe() returns success even when zero sensors were probed
> > > (the per-sensor loop simply falls through with i == 0), leaving
> > > common->head empty. rcar_thermal_suspend()/rcar_thermal_resume() then
> > > call list_first_entry(&common->head, ...) unconditionally, and
> > > list_first_entry() on an empty list does not return NULL: it returns
> > > container_of() of the list head, i.e. a wild pointer into
> > > struct rcar_thermal_common, which is dereferenced at
> > > priv->chip->needs_suspend_resume and in the MMIO accesses that follow.
> > > A malformed DT node with no MEM resource triggers this at every
> > > suspend/resume cycle.
> > >
> > > As suggested by Niklas Söderlund, fix this at the root instead of
> > > tolerating an empty list in the PM callbacks: fail probe with -ENODEV
> > > when no sensor was probed, since a successfully bound driver with zero
> > > sensors is meaningless. The error path through rcar_thermal_remove()
> > > already tolerates the empty list (it iterates with
> > > rcar_thermal_for_each_priv()).
> > >
> > > This issue was found by static analysis; we do not have R-Car hardware,
> > > so this is compile-tested only.
> > >
> > > Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> > > Reported-by: Li Ximo <lixicaicai@gmail.com>
> > > Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>
> > > ---
> > > v1: https://lore.kernel.org/r/20261026115018.3227372-1-lixicaicai@gmail.com
> > > v2: completely rework the fix per Niklas Söderlund's review: fail probe
> > > with -ENODEV when no sensors are probed instead of guarding
> > > suspend/resume.
> > >
> > >  drivers/thermal/renesas/rcar_thermal.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/thermal/renesas/rcar_thermal.c b/drivers/thermal/renesas/rcar_thermal.c
> > > index 9a9038671..6a1e977d8 100644
> > > --- a/drivers/thermal/renesas/rcar_thermal.c
> > > +++ b/drivers/thermal/renesas/rcar_thermal.c
> > > @@ -514,6 +514,12 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> > >                       enr_bits |= 3 << (i * 8);
> > >       }
> > >
> > > +     if (!i) {
> >
> > While this works I think it's fragile as it depends on the loop variable
> > of the last loop. If the loop is changed, or a new one added, this check
> > can break. Why not make it robust and check if any sensors where added
> > to the list containing sensors (common->head)?
> 
> Yeah, then we could move the loop variable declarations inside the
> for-statements...
> 
> > > +             dev_err(dev, "no thermal sensor probed\n");
> > > +             ret = -ENODEV;
> > > +             goto error_unregister;
> > > +     }
> > > +
> > >       if (common->base && enr_bits)
> > >               rcar_thermal_common_write(common, ENR, enr_bits);
> 
> ... if not for the next line:
> 
>     dev_info(dev, "%d sensor probed\n", i);

I like the idea of being able to move the loop variable to the 
declarations. This could be addressed with list_count_nodes(), right?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Kind Regards,
Niklas Söderlund