Re: [PATCH v2] HID: amd_sfh: Validate PCI BAR size before mapping

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race on `cli_data->cur_hid_dev` in `amd_sfh_set_report()` can lead to wrong completion flags being set, causing legitimate HID requests to time out.
--

commit eb712409bb933989aa1a882596c6ff02ffcdb00c
Author: Slawomir Stepien <sst@poczta.fm>
Subject: HID: amd_sfh: Validate PCI BAR size before mapping

This patch validates that the PCI BAR 2 size is at least 128KB before
attempting to map it. This prevents an out-of-bounds memory access and page
fault during the probe function if the driver is bound to a device with a
smaller BAR size.

Link: https://syzkaller.appspot.com/bug?extid=4eadd4dfe9e66522bae8
Link: https://syzkaller.appspot.com/ai_job?id=3bc1c45c-548f-4ab5-8243-d2c8ec321d6c

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> index eda26a094d3f1..061a63519d441 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
[ ... ]
> @@ -497,6 +497,16 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
>  	if (rc)
>  		return rc;
>  
> +	if (!(pci_resource_flags(pdev, 2) & IORESOURCE_MEM)) {
> +		dev_err(&pdev->dev, "BAR 2 is not IORESOURCE_MEM\n");
> +		return -ENODEV;
> +	}
> +
> +	if (pci_resource_len(pdev, 2) < AMD_SFH_MIN_BAR_SIZE) {
> +		dev_err(&pdev->dev, "BAR 2 is too small\n");
> +		return -EINVAL;
> +	}
> +
>  	rc = pcim_iomap_regions(pdev, BIT(2), DRIVER_NAME);
>  	if (rc)
>  		return rc;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible to have a data
race on cli_data->cur_hid_dev here in the amd_sfh driver?

Looking at drivers/hid/amd-sfh-hid/amd_sfh_client.c:amd_sfh_set_report(),
the shared global state is modified without acquiring the mp2->lock mutex:

amd_sfh_set_report() {
    ...
    cli_data->cur_hid_dev = i;
    break;
    ...
}

Could this lead to wrong completion flags being set in amdtp_hid_wakeup()?

amdtp_hid_wakeup() {
    ...
    cli_data->request_done[cli_data->cur_hid_dev] = true;
    ...
}

If Process A triggers HID_REQ_GET_REPORT on one HID device, it queues work to
amd_sfh_work(). Concurrently, if Process B triggers HID_REQ_SET_REPORT via
ioctl on a different HID device on the same AMD SFH PCI controller,
amd_sfh_set_report() modifies cli_data->cur_hid_dev without lock protection.

Would this cause amdtp_hid_wakeup() to read the racy cur_hid_dev value, write
to the wrong request_done index, and wake up the wrong waitqueue, causing
legitimate HID requests to time out?

This seems reachable by local users capable of sending HID raw requests (e.g.,
via /dev/hidrawX), which in many desktop configurations is accessible to the
logged-in user.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914095007.205287-1-sst@poczta.fm?part=1