Re: [PATCH RESEND] Input: rmi4 - Move sensor initialization into own function for F12

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

Pre-existing issues:
- [High] Heap Out-of-Bounds Read in F12 attention handler
- [High] Division by Zero kernel panic triggered by malicious device
- [High] Uninitialized Kernel Stack Memory Leak
--

--- Patch [1]: [PATCH RESEND] Input: rmi4 - Move sensor initialization into own function for F12 ---
commit ca79b6f1c1761554daeb36e4304a62bdc7fa5f10
Author: David Heidelberg <[email protected]>

Input: rmi4 - Move sensor initialization into own function for F12

This commit refactors the F12 probe logic by extracting the sensor
initialization sequence into a separate helper function,
rmi_f12_sensor_init(). This reduces the size of rmi_f12_probe() and makes
the code more structured.

> diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> index 88c28089de993..333477bf0663e 100644
> --- a/drivers/input/rmi4/rmi_f12.c
> +++ b/drivers/input/rmi4/rmi_f12.c
[ ... ]
> @@ -426,29 +457,10 @@ static int rmi_f12_probe(struct rmi_function *fn)
>  		return ret;
>  
>  	sensor = &f12->sensor;
> -	sensor->fn = fn;
> -	f12->data_addr = fn->fd.data_base_addr;
> -	pkt_size = rmi_register_desc_calc_size(&f12->data_reg_desc);
> -	if (pkt_size > SZ_1M) {
> -		dev_err(&fn->dev, "Invalid data packet size: %zu\n", pkt_size);
> -		return -EINVAL;
> -	}
> -	sensor->pkt_size = pkt_size;
> -
> -	sensor->axis_align = f12->sensor_pdata.axis_align;
>  
> -	sensor->x_mm = f12->sensor_pdata.x_mm;
> -	sensor->y_mm = f12->sensor_pdata.y_mm;
> -	sensor->dribble = f12->sensor_pdata.dribble;
> -
> -	if (sensor->sensor_type == rmi_sensor_default)
> -		sensor->sensor_type = f12->sensor_pdata.sensor_type;
> -
> -	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: data packet size: %u\n", __func__,
> -		sensor->pkt_size);
> -	sensor->data_pkt = devm_kmalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
> -	if (!sensor->data_pkt)
> -		return -ENOMEM;
> +	ret = rmi_f12_sensor_init(fn, f12);
> +	if (ret)
> +		return ret;
>  
>  	dev_set_drvdata(&fn->dev, f12);
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds heap
read when f12 processes objects.

In rmi_f12_attention(), we advance the pointer by data1_offset but pass the
full valid_bytes without adjusting it:

drivers/input/rmi4/rmi_f12.c:rmi_f12_attention() {
    ...
    if (f12->data1)
        rmi_f12_process_objects(f12,
                                &sensor->data_pkt[f12->data1_offset],
                                valid_bytes);
    ...
}

Then in rmi_f12_process_objects(), we read object coordinates:

drivers/input/rmi4/rmi_f12.c:rmi_f12_process_objects() {
    ...
    obj->x = get_unaligned_le16(&data1[1]);
    ...
}

Could a malicious device over I2C/SPI specify a non-zero Data0 register size
(offsetting Data1) and a large number of fingers, causing the loop to read
data1_offset bytes past the end of the sensor->data_pkt heap allocation?

[Severity: High]
This isn't a bug introduced by this patch, but could a malicious hardware
device trigger a kernel panic via division by zero?

In rmi_f12_read_sensor_tuning(), if the device returns 0 for the DPM
resolution:

drivers/input/rmi4/rmi_f12.c:rmi_f12_read_sensor_tuning() {
    ...
    sensor->x_mm = sensor->max_x / dpm_resolution;
    sensor->y_mm = sensor->max_y / dpm_resolution;
    ...
}

Does the driver need to check if dpm_resolution is zero before performing this
division?

[Severity: High]
This is a pre-existing issue, but could this code leak uninitialized kernel
stack memory to userspace?

In rmi_f12_read_sensor_tuning(), we read item->reg_size bytes into the buf
stack array:

drivers/input/rmi4/rmi_f12.c:rmi_f12_read_sensor_tuning() {
    ...
    ret = rmi_read_block(rmi_dev, fn->fd.control_base_addr + offset,
                         buf, item->reg_size);
    ...
    if (rmi_register_desc_has_subpacket(item, 0)) {
        sensor->max_x = get_unaligned_le16(&buf[offset]);
        sensor->max_y = get_unaligned_le16(&buf[offset + 2]);
        offset += 4;
    }
    ...
}

If a malicious device specifies a small reg_size (e.g., 1 byte) but indicates
via the subpacket_map that subpacket 0 exists, could the unconditional reads
for max_x and max_y read past the initialized portion of buf?

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