Re: [PATCH] PCI/sysfs: Check IORESOURCE_DISABLED in resource mmap handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] IORESOURCE_DISABLED is an enumeration/allocation flag, not a dynamic operational state flag.
- [High] The patch fails to check for IORESOURCE_UNSET, leaving a path to map physical address 0.
- [Medium] The check was omitted from the legacy procfs mmap interface and sysfs I/O port read/write handlers.
--

commit b337c28a9b32f2246e39d66b4dc57399f253f527
Author: Ravi Kumar Bandi <ravib@amazon.com>
PCI/sysfs: Check IORESOURCE_DISABLED in resource mmap handler

This commit attempts to block userspace from mapping PCI BAR resources of
disabled or soft-unplugged devices. It does so by adding a check for the
IORESOURCE_DISABLED flag in the sysfs pci_mmap_resource() handler.

> pci_mmap_resource() does not check IORESOURCE_DISABLED before mapping
> a PCI BAR resource into userspace. This allows new mmaps to succeed
> even after a device has been marked disabled or soft-unplugged by the
> driver to prevent further access.

Is IORESOURCE_DISABLED actually set dynamically when a device is disabled
or soft-unplugged by the driver?

Looking at the PCI core, IORESOURCE_DISABLED appears to be an allocation
flag used when a resource cannot be assigned structurally (such as a
64-bit BAR on a 32-bit system). It doesn't seem to be dynamically set
by pci_disable_device() or soft-unplug paths like
pci_stop_and_remove_bus_device().

If the flag is never set during a soft-unplug, will this check successfully
prevent the post-removal mmaps as intended?

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d37860841260c..61ae259f8ca80 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1089,6 +1089,9 @@ static int pci_mmap_resource(struct kobject *kobj, const struct bin_attribute *a
>  	if (ret)
>  		return ret;
>  
> +	if (res->flags & IORESOURCE_DISABLED)
> +		return -ENODEV;
> +

Should this also check for IORESOURCE_UNSET?

When the PCI core fails to assign an address to a resource, it sets
IORESOURCE_UNSET and sets the resource start address to 0. Since the size
remains non-zero, the sysfs files are still created and exposed.

Without checking IORESOURCE_UNSET, could pci_mmap_resource_range() add
pci_resource_start() >> PAGE_SHIFT (which is 0) to vma->vm_pgoff, allowing
userspace to inadvertently map physical address 0 (System RAM/BIOS) instead
of an MMIO region?

Additionally, does this protection need to be extended to other interfaces?

Even with this check in sysfs mmap, userspace might still bypass it using
the legacy procfs mmap interface via proc_bus_pci_mmap().

Furthermore, for I/O port BARs (IORESOURCE_IO), the sysfs read and write
callbacks (pci_read_resource_io and pci_write_resource_io) directly execute
inb/outb instructions without verifying if the resource is disabled. Could
an application holding an open file descriptor continue raw hardware
accesses and bypass this protection?

>  	if (res->flags & IORESOURCE_MEM && iomem_is_exclusive(res->start))
>  		return -EINVAL;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512084315.32564-1-ravib@amazon.com?part=1