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@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)?

> +		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