Re: [PATCH v2 3/5] Input: zinitix - add support for modes 0 and 1

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

New issues:
- [High] The input device is registered and exposed to userspace before its private `read_point_status` function pointer is initialized, creating a race condition that leads to a NULL pointer dereference in the IRQ handler.

Pre-existing issues:
- [High] The driver passes stack-allocated structs to the I2C subsystem for receiving data, which can cause memory corruption or kernel panics if the I2C controller driver maps the buffers for DMA.
--

--- Patch [3]: [PATCH v2 3/5] Input: zinitix - add support for modes 0 and 1 ---
commit 2caca9eabecb85f6226ee1af1cfdde1aeb2f1bc2
Author: Kaustabh Chakraborty <[email protected]>

Input: zinitix - add support for modes 0 and 1

Zinitix touchscreens have three modes, numbered 0 to 2. The driver
implements mode 2, and leaves out modes 0 and 1. Implement modes 0 and 1
in the driver, along with their event structs.

> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
> index a0a89d25e25bb..aafd0b7d8edbd 100644
> --- a/drivers/input/touchscreen/zinitix.c
> +++ b/drivers/input/touchscreen/zinitix.c

[ ... ]

> @@ -440,6 +471,87 @@ static void zinitix_report_keys(struct bt541_ts_data *bt541, u16 icon_events)
>  				 bt541->keycodes[i], icon_events & BIT(i));
>  }
>  
> +static int zinitix_read_point_status_mode0(struct bt541_ts_data *bt541,
> +					   struct touch_event *event)
> +{
> +	struct touch_event_mode0 event_mode0;
> +	int ret;
> +	int i;
> +
> +	ret = zinitix_read_data(bt541->client, ZINITIX_POINT_STATUS_REG,
> +				&event_mode0, sizeof(struct touch_event_mode0));

[Severity: High]
This is a pre-existing issue, but does passing this stack-allocated
event_mode0 struct to the I2C subsystem cause memory corruption or kernel
panics if the underlying I2C host controller driver maps it for DMA?

The same pattern is also present in the new zinitix_read_point_status_mode1()
and zinitix_read_point_status_mode2() functions.

[ ... ]

> @@ -682,13 +793,19 @@ static int zinitix_ts_probe(struct i2c_client *client)
>  		bt541->zinitix_mode = DEFAULT_TOUCH_POINT_MODE;
>  	}
>  
> -	if (bt541->zinitix_mode != 2) {
> -		/*
> -		 * If there are devices that don't support mode 2, support
> -		 * for other modes (0, 1) will be needed.
> -		 */
> +	switch (bt541->zinitix_mode) {
> +	case 0:
> +		bt541->read_point_status = zinitix_read_point_status_mode0;

[Severity: High]
Can a fast userspace trigger a NULL pointer dereference here?

Since zinitix_init_input_dev() is called earlier in zinitix_ts_probe(),
the device is registered and exposed to userspace before
bt541->read_point_status is initialized:

zinitix_ts_probe()
    zinitix_init_input_dev()
        input_register_device()
    ...
    switch (bt541->zinitix_mode) {
    case 0:
        bt541->read_point_status = zinitix_read_point_status_mode0;

If userspace opens the device and an interrupt triggers before the switch
statement, zinitix_ts_irq_handler() will dereference the uninitialized
read_point_status pointer:

zinitix_ts_irq_handler()
    bt541->read_point_status(bt541, &touch_event);

> +		break;
> +	case 1:
> +		bt541->read_point_status = zinitix_read_point_status_mode1;
> +		break;
> +	case 2:
> +		bt541->read_point_status = zinitix_read_point_status_mode2;
> +		break;

[ ... ]

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