Re: [PATCH 2/2] Input: rmi4 - fallback to platform data for x/y-mm in F12

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Potential kernel panic due to division by zero if the device firmware returns 0 for DPM resolution.
- [High] Potential read of uninitialized stack memory when parsing device subpackets.
- [High] Truncation of physical dimensions `x_mm` and `y_mm` for devices larger than 255 mm.
--

--- Patch [2]: [PATCH 2/2] Input: rmi4 - fallback to platform data for x/y-mm in F12 ---
commit 825204fcd696c68bb8002a2ae57765aed06fc9e4
Author: David Heidelberg <[email protected]>

Input: rmi4 - fallback to platform data for x/y-mm in F12

When detection for touchscreen-x-mm / y-mm returns 0, fallback to the
platform data, so we get meaningful values.

> diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> index 88797556e738c..6f2e98c3a3499 100644
> --- a/drivers/input/rmi4/rmi_f12.c
> +++ b/drivers/input/rmi4/rmi_f12.c
> @@ -199,8 +199,19 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a kernel
panic from division by zero?

Earlier in rmi_f12_read_sensor_tuning(), dpm_resolution is read directly
from the hardware into buf:

        ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
        if (ret) {
                ...
        }
        dpm_resolution = buf[0];

        x_mm = sensor->max_x / dpm_resolution;
        y_mm = sensor->max_y / dpm_resolution;

If a buggy or malicious device returns 0 for the DPM resolution, this will
unconditionally crash the kernel during the probe path. Should there be a 
check for dpm_resolution == 0 before performing this division?


[Severity: High]
This is a pre-existing issue, but does this code read uninitialized stack 
memory when parsing device subpackets?

In rmi_f12_read_sensor_tuning(), a 15-byte stack array is declared without
initialization:

        u8 buf[15];

The driver reads item->reg_size bytes into this buffer:

        ret = rmi_read_block(rmi_dev, fn->fd.control_base_addr + offset,
                             buf, item->reg_size);

The size is controlled by the device descriptor. The driver then uses offsets
to access this buffer based purely on rmi_register_desc_has_subpacket(),
without ensuring item->reg_size actually covers the accessed offsets.

If a device supplies a small reg_size but claims all subpackets are present,
could this read past the initialized portion of buf and leak kernel stack
memory or cause erratic behavior?

>  			 "platform data y_mm (%d) != detected y_mm: (%d)\n",
>  			 sensor->y_mm, y_mm);
>  
> -	sensor->x_mm = x_mm;
> -	sensor->y_mm = y_mm;
> +	if (x_mm)
> +		sensor->x_mm = x_mm;

[Severity: High]
This is a pre-existing issue, but could the physical dimensions overflow on 
larger devices, which this patch might now incorrectly interpret as a failure?

The dimensions are calculated in millimeters and stored in u8 variables:

drivers/input/rmi4/rmi_f12.c:rmi_f12_read_sensor_tuning()
        u8 x_mm, y_mm;

And also stored as u8 in struct rmi_2d_sensor:

drivers/input/rmi4/rmi_2d_sensor.h:
        u8 x_mm;
        u8 y_mm;

For any touchscreen larger than 255 mm (approx. 10 inches), the value will
overflow and truncate. If a dimension is exactly 256 mm, it truncates to 0.

With this patch, a truncated 0 will trigger the x_mm == 0 fallback path and
incorrectly retain platform data instead of the detected value. Should these 
dimensions be stored using u16?

> +	else
> +		dev_warn(&fn->dev,
> +			 "x_mm detection returned 0, keeping %d\n",
> +			 sensor->x_mm);
> +
> +	if (y_mm)
> +		sensor->y_mm = y_mm;
> +	else
> +		dev_warn(&fn->dev,
> +			 "y_mm detection returned 0, keeping %d\n",
> +			 sensor->y_mm);
>  
>  	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: x_mm: %d y_mm: %d\n", __func__,
>  		sensor->x_mm, sensor->y_mm);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2