Re: [PATCH] HID: mcp2200: validate input report length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A raw event arriving immediately after `hid_hw_open` will cause a NULL pointer dereference because driver data and synchronization primitives are not yet initialized.
- [High] Concurrent calls to `mcp_cmd_read_all` can lead to corrupted state, lost wakeups, and data races because `wait_in_report` and shared response fields are not protected by locks during command execution.
- [High] The GPIO chip is managed via devres and unregistered after the HID hardware is stopped, leading to potential use-after-free or invalid hardware accesses.
--

commit d11b6ebfc1ea2a8baf37113e30af821787d261be
Author: Pengpeng <pengpeng@iscas.ac.cn>
Subject: HID: mcp2200: validate input report length

This commit updates mcp2200_raw_event() to validate that the command byte
and response fields are present in the HID input report before accessing
them. It rejects truncated reports instead of reading past the end of the
payload.

> diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
> index dafdd5b4a0797..d2f45892dc22a 100644
> --- a/drivers/hid/hid-mcp2200.c
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -299,8 +299,19 @@ static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	struct mcp2200 *mcp = hid_get_drvdata(hdev);
>  	struct mcp_read_all_resp *all_resp;
>  
> +	if (size < 1) {
> +		mcp->status = -EINVAL;

[Severity: High]
This is a pre-existing issue, but is it possible for an interrupt to fire
immediately after hid_hw_open() in mcp2200_probe() and trigger a NULL
pointer dereference here?

If mcp2200_raw_event() is called before hid_set_drvdata() and lock
initialization complete:

drivers/hid/hid-mcp2200.c:mcp2200_probe() {
        ...
        ret = hid_hw_open(hdev);
        ...
        mutex_init(&mcp->lock);
        init_completion(&mcp->wait_in_report);
        hid_set_drvdata(hdev, mcp);
        ...
}

Could mcp be NULL here, leading to a crash when accessing mcp->status?

> +		complete(&mcp->wait_in_report);
> +		return 0;
> +	}
> +
>  	switch (data[0]) {
>  	case READ_ALL:
> +		if (size < sizeof(*all_resp)) {
> +			mcp->status = -EINVAL;
> +			break;
> +		}
> +
>  		all_resp = (struct mcp_read_all_resp *) data;
>  		mcp->status = 0;
>  		mcp->gpio_inval = all_resp->io_port_val_bmap;

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent calls to
mcp_cmd_read_all() lead to corrupted state or lost wakeups?

In mcp_cmd_read_all(), the mutex is released before waiting for the
response:

drivers/hid/hid-mcp2200.c:mcp_cmd_read_all() {
        ...
        mutex_unlock(&mcp->lock);
        t = wait_for_completion_timeout(&mcp->wait_in_report, ...);
        ...
}

Does modifying mcp->status and mcp->gpio_inval locklessly here in
mcp2200_raw_event() create a data race if multiple threads are reading or
writing GPIO state concurrently?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does using devres for the GPIO chip
create a use-after-free risk during device removal?

In mcp2200_probe(), the chip is registered with devres:

drivers/hid/hid-mcp2200.c:mcp2200_probe() {
        ...
        ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
        ...
}

Then in mcp2200_remove(), the hardware is stopped before the chip is
unregistered:

drivers/hid/hid-mcp2200.c:mcp2200_remove() {
        ...
        hid_hw_close(hdev);
        hid_hw_stop(hdev);
        ...
}

Could a user-space process continue interacting with the GPIO device while
the USB device is physically removed, causing invalid hardware accesses
before devres cleans up the GPIO chip?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706090932.75673-1-pengpeng@iscas.ac.cn?part=1