Re: [PATCH v2 1/5] Input: zinitix - check all available fingers for every touch event

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

New issues:
- [High] Unconditionally iterating over all slots reverts a previous bugfix and reintroduces phantom touches on BT404 controllers.

Pre-existing issues:
- [High] I2C message buffers are allocated on the stack, violating DMA API rules.
- [High] Input device is registered before all driver private data is fully initialized, allowing a Use-After-Free if probe fails.
--

--- Patch [1]: [PATCH v2 1/5] Input: zinitix - check all available fingers for every touch event ---
commit 035227fe3b93a4e9baec0dee6f309a653f790ded
Author: Kaustabh Chakraborty <[email protected]>

Input: zinitix - check all available fingers for every touch event

This commit modifies the Zinitix touchscreen driver to unconditionally
iterate over all possible fingers during a touch event, removing the
use of the finger_mask field.

Link: https://android.googlesource.com/kernel/bcm/+/23d376ef33aa4c500a5ea24a290f029d5f8e2de3/drivers/input/touchscreen/zinitix_touch.c#1942 [1]

> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
> index 0c36765bd79f8..3421b8ffb19b2 100644
> --- a/drivers/input/touchscreen/zinitix.c
> +++ b/drivers/input/touchscreen/zinitix.c
> @@ -445,7 +445,6 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
>  	struct bt541_ts_data *bt541 = bt541_handler;
>  	struct i2c_client *client = bt541->client;
>  	struct touch_event touch_event;

[Severity: High]
This is a pre-existing issue, but does passing this stack-allocated variable
to the I2C read functions violate DMA API rules?

During a touch event, zinitix_ts_irq_handler() passes the stack-allocated
touch_event down to zinitix_read_data(), which eventually reaches
i2c_master_recv(). If the I2C master driver uses DMA, mapping this stack
memory could lead to memory corruption or a kernel panic on architectures
with VMAP_STACK and DMA API debugging enabled.

[ ... ]
> @@ -470,8 +469,7 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
>  		zinitix_report_keys(bt541, le16_to_cpu(icon_events));
>  	}
>  
> -	finger_mask = touch_event.finger_mask;
> -	for_each_set_bit(i, &finger_mask, MAX_SUPPORTED_FINGER_NUM) {
> +	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
>  		const struct point_coord *p = &touch_event.point_coord[i];
>  
>  		/* Only process contacts that are actually reported */

[Severity: High]
Could unconditionally iterating over all slots revert a previous bugfix for
BT404 controllers?

When a touch occurs on a BT404 controller, the firmware incorrectly leaves
SUB_BIT_EXIST set for shadow fingers. By dropping the finger_mask filter
here, it looks like zinitix_report_finger() will be called for those
shadow fingers, potentially causing multiple phantom inputs for every
physical touch.


[Severity: High]
This is a pre-existing issue, but looking at zinitix_ts_probe(), is there a
use-after-free window if probe fails?

The zinitix,mode property is parsed and validated after the input device is
already registered by zinitix_init_input_dev():

zinitix_ts_probe() {
        ...
        error = zinitix_init_input_dev(bt541);
        if (error) { ... }

        error = device_property_read_u32(&client->dev, "zinitix,mode",
                                         &bt541->zinitix_mode);
        if (error < 0) { ... }
        ...
}

If parsing fails, probe returns an error and devres frees the bt541
structure. If userspace opens the device during this window, the input_dev
survives the probe failure. When userspace subsequently closes it, wouldn't
zinitix_input_close() dereference the freed bt541 pointer?

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