Re: [PATCH v5 3/5] thermal/drivers/sun8i: get calibration data from two nvmem cells

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing or empty 'nvmem-cell-names' property causes an unconditional NULL pointer dereference in the calibration callback.
- [Medium] The newly introduced A523 calibration function accesses fixed indices of the dynamically sized caldata array without validating callen.
--

commit 385d056a66304901aadb5675fbf878e2aae0cfc7
Author: Mikhail Kalashnikov <iuncuim@gmail.com>

thermal/drivers/sun8i: get calibration data from two nvmem cells

This commit updates the sun8i thermal driver to support reading calibration
data from multiple nvmem cells by iterating over nvmem-cell-names. It appends
the data from each cell into a single dynamically allocated buffer which is
passed to the calibration callbacks. This provides the ability to support the
A523 processor which has calibration data split across two cells.

> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index df0c269708ea7..3bdd62aa8bdf1 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -301,43 +301,65 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
>  
>  static int sun8i_ths_calibrate(struct ths_device *tmdev)
>  {
> -	struct nvmem_cell *calcell;
> +	struct nvmem_cell *calcell = NULL;
>  	struct device *dev = tmdev->dev;
> -	u16 *caldata;
> -	size_t callen;
> +	struct device_node *np = dev_of_node(dev);
> +	struct property *prop;
> +	const char *cellname;
> +	u8 *caldata = NULL;
> +	size_t callen = 0;
>  	int ret = 0;
>  
> -	calcell = nvmem_cell_get(dev, "calibration");
> -	if (IS_ERR(calcell)) {

[ ... ]

> +		memcpy(caldata + callen, caldatapart, len);
> +		callen += len;
> +		kfree(caldatapart);
>  	}
>  
> -	tmdev->chip->calibrate(tmdev, caldata, callen);
> +	tmdev->chip->calibrate(tmdev, (u16 *)caldata, callen);

[Severity: High]
If the nvmem-cell-names property is missing from the device tree, the
of_property_for_each_string() loop is bypassed entirely and caldata remains
NULL.

Does this unconditionally pass a NULL pointer to the backend calibration
callback?

Backend callbacks like sun8i_h3_ths_calibrate() dereference the array directly:

	if (!caldata[0] || callen < 2 * tmdev->chip->sensor_num)
		return -EINVAL;

If the device tree does not contain calibration nvmem cells, this will cause a
kernel panic on boot.


[Severity: Medium]
For the A523 processor, does sun55i_a523_ths_calibrate() validate that callen
is large enough before accessing the array?

Looking at sun55i_a523_ths_calibrate() which was recently introduced:

		if (tmdev->chip->sensor_num == 1) {
			sensor_reg = ((caldata[5] >> 8) | (caldata[6] << 8)) & TEMP_CALIB_MASK;
		} else {

It accesses elements up to caldata[6] without verifying that callen covers it.
If a device tree defines nvmem cells that yield less than 14 bytes of data,
will these lookups read out of bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704171411.1413349-1-iuncuim@gmail.com?part=3