Re: [PATCH 1/2] Input: synaptics-rmi4 - f12: read only the object data on attention

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Input: rmi4: unadjusted valid_bytes in rmi_f12_attention() reads past data1
--

commit c4b3a9b641a3a2e611195ada64fab67096a67d47
Author: Dmitry Mikushin <dmitry@kernelgen.org>

Input: synaptics-rmi4 - f12: read only the object data on attention

This commit optimizes F12 attention handling when attention data is not
supplied by the transport by computing the size of the leading registers
up to and including Data1 at probe time and reading only that prefix
instead of the full F12 data packet.

> diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> index 841884d967a3d..ad210464b2f34 100644
> --- a/drivers/input/rmi4/rmi_f12.c
> +++ b/drivers/input/rmi4/rmi_f12.c
> @@ -58,6 +58,8 @@ struct f12_data {
>  	const struct rmi_register_desc_item *data15;
>  	u16 data15_offset;
>  
> +	u32 read_size;
> +
>  	unsigned long irq_mask[];
>  };
>  
> @@ -269,8 +271,9 @@ static irqreturn_t rmi_f12_attention(int irq, void *ctx)
>  		drvdata->attn_data.data += valid_bytes;
>  		drvdata->attn_data.size -= valid_bytes;
>  	} else {
> +		valid_bytes = f12->read_size;
>  		retval = rmi_read_block(rmi_dev, f12->data_addr,
> -					sensor->data_pkt, sensor->pkt_size);
> +					sensor->data_pkt, valid_bytes);
>  		if (retval < 0) {
>  			dev_err(&fn->dev, "Failed to read object data. Code: %d.\n",
>  				retval);
> @@ -529,6 +532,17 @@ static int rmi_f12_probe(struct rmi_function *fn)
>  		data_offset += item->reg_size;
>  	}
>  
> +	/*
> +	 * Only Data1 (object reports) is consumed when the data registers
> +	 * are read from the device, so do not read the remaining registers
> +	 * on every attention. On SMBus this transfer dominates the per-frame
> +	 * cost and therefore limits the report rate.
> +	 */
> +	f12->read_size = sensor->pkt_size;
> +	if (f12->data1)
> +		f12->read_size = min_t(u32, sensor->pkt_size,
> +				       f12->data1_offset + f12->data1->reg_size);

[Severity: High]
Can this read past the end of Data1 into uninitialized memory in
sensor->data_pkt[] or out-of-bounds past sensor->data_pkt[] when Data0 is
present in the F12 data register descriptor (so f12->data1_offset > 0)?

In rmi_f12_probe(), sensor->data_pkt is allocated with devm_kmalloc()
without zero-initialization:

drivers/input/rmi4/rmi_f12.c:rmi_f12_probe() {
	...
	sensor->data_pkt = devm_kmalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
	...
}

In rmi_f12_attention(), rmi_read_block() now populates only the first
valid_bytes (f12->read_size = f12->data1_offset + f12->data1->reg_size)
bytes of sensor->data_pkt, leaving any trailing bytes of sensor->data_pkt
uninitialized, and then passes &sensor->data_pkt[f12->data1_offset] with
the unadjusted valid_bytes to rmi_f12_process_objects():

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);
	...
}

Inside rmi_f12_process_objects(), size is treated as the number of valid
bytes starting at data1 (&sensor->data_pkt[f12->data1_offset]):

drivers/input/rmi4/rmi_f12.c:rmi_f12_process_objects() {
	struct rmi_2d_sensor *sensor = &f12->sensor;
	u32 objects = min(f12->data1->num_subpackets, size / F12_DATA1_BYTES_PER_OBJ);
	...
}

Because rmi_parse_register_desc_item() parses item->reg_size and
item->num_subpackets independently, if f12->data1->reg_size is smaller
than f12->data1->num_subpackets * F12_DATA1_BYTES_PER_OBJ (or if
valid_bytes on the attn_data path is smaller than f12->data1_offset +
f12->data1->num_subpackets * F12_DATA1_BYTES_PER_OBJ),
rmi_f12_process_objects() can read up to f12->data1_offset bytes past
sensor->data_pkt[valid_bytes - 1], reporting uninitialized kernel heap
memory to userspace via input events or reading past the end of
sensor->data_pkt[].

Should rmi_f12_attention() check that valid_bytes > f12->data1_offset and
pass valid_bytes - f12->data1_offset to rmi_f12_process_objects()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026093118.2312302-1-dmitry@kernelgen.org?part=1