Re: [PATCH v6 4/5] thermal/drivers/sun8i: Add support for A523 THS0/1 controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect check for unprogrammed eFuse can reject valid calibration data.
- [Medium] Missing bounds check on `callen` in `sun55i_a523_ths_calibrate` allows out-of-bounds memory reads on the dynamically allocated `caldata` array.
--

commit 1a5244bb0d5a03f3ec7f2a8c21b6249015b66057
Author: Mikhail Kalashnikov <iuncuim@gmail.com>

    thermal/drivers/sun8i: Add support for A523 THS0/1 controllers
    
    The A523 processor has two temperature controllers, THS0 and THS1.
    THS0 has only one temperature sensor, which is located in the DRAM
    controller. THS1 does have 4 sensors:
    ths1_0 - "big" cores
    ths1_1 - "little" cores
    ths1_2 - gpu
    ths1_3 - npu

> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 3606548801f02..4a81486f8faec 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c

[ ... ]

> @@ -298,6 +319,105 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
>  	return 0;
>  }
>  
> +/*
> + * The A523 calibration data is stored in the eFuse, split across two
> + * nvmem cells. THS1 has four sensors, including ths1_3 which is used
> + * for the NPU, each with its own calibration value. The fields are
> + * packed as shown in the layouts below.
> + *
> + * efuse layout 0x38-0x3F (caldata[0..3]):
> + *     caldata[0]      caldata[1]      caldata[2]      caldata[3]
> + * 0               16      24      32  36          48          60 64
> + * +---------------+---------------+---------------+---------------+
> + * |               |       |   temp    |  ths1_0   |  ths1_1   |   +
> + * +---------------+---------------+---------------+---------------+
> + *
> + * efuse layout 0x44-0x4B (caldata[4..7]):
> + *     caldata[4]      caldata[5]      caldata[6]      caldata[7]
> + * 0           12  16      24      32  36          48             64
> + * +---------------+---------------+---------------+---------------+
> + * |  ths1_2   |  ths1_3   |    ths0   |           |               +
> + * +---------------+---------------+---------------+---------------+
> + */
> +static int sun55i_a523_ths_calibrate(struct ths_device *tmdev,
> +				     u16 *caldata, int callen)
> +{
> +	struct device *dev = tmdev->dev;
> +	int i, ft_temp;
> +
> +	if (!caldata[1])

[Severity: High]
Does this check inadvertently reject valid calibration data?

If the 12-bit factory calibration temperature happens to be a multiple of
25.6 C (meaning its lowest 8 bits are 0, like 0x100), caldata[1] will be
legitimately 0 since it only holds those lower 8 bits. This would cause the
driver to incorrectly abort calibration on perfectly valid hardware.

[Severity: Medium]
Can this read out-of-bounds memory?

The function receives a dynamically allocated caldata array of size callen
populated from the NVMEM cells, but callen is never verified here. If the
device tree provides a malformed or incomplete NVMEM cell, this access and
subsequent reads will happen past the end of the allocation.

> +		return -EINVAL;
> +
> +	ft_temp = (((caldata[2] << 8) | (caldata[1] >> 8)) & FT_TEMP_MASK) * 100;
> +
> +	for (i = 0; i < tmdev->chip->sensor_num; i++) {
> +		int sensor_reg, sensor_temp, cdata, offset;
> +		/*
> +		 * Chips ths0 and ths1 have common parameters for value
> +		 * calibration. To separate them we can use the number of
> +		 * temperature sensors on each chip.
> +		 * For ths0 this value is 1.
> +		 */
> +		if (tmdev->chip->sensor_num == 1) {
> +			sensor_reg = ((caldata[5] >> 8) |
> +				      (caldata[6] << 8)) & TEMP_CALIB_MASK;

[Severity: Medium]
Is it possible to read out-of-bounds memory here as well?

This is the highest index accessed in sun55i_a523_ths_calibrate. It strictly
requires a minimum buffer size of 14 bytes. If the device tree provides an
8-byte NVMEM cell, accessing caldata[5] and caldata[6] will pull
uninitialized heap data into the calibration calculations or cause a crash.

> +		} else {
> +			switch (i) {
> +			case 0:

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