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@xxxxxxxxxxxx> wrote:
> On 2026-11-03 16:46:04 +0800, Li Ximo wrote:
> > From: Wang Haoran <haoranwangsec@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Reported-by: Li Ximo <lixicaicai@xxxxxxxxx>
> > Signed-off-by: Wang Haoran <haoranwangsec@xxxxxxxxx>
> > ---
> > v1: https://lore.kernel.org/r/20261026115018.3227372-1-lixicaicai@xxxxxxxxx
> > 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@xxxxxxxxxxxxxx

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