Re: [PATCH v7 06/24] PCI/sysfs: Add static PCI resource attribute macros

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The `pci_dev_resource_io_attr` macro leaves the `.size` field of the binary attribute uninitialized (0), disabling VFS bounds checking. This allows arbitrary offsets that truncate on 32-bit architectures, enabling read/write access to arbitrary I/O ports.
- [High] The `pci_dev_resource_io_attr` macro conditionally omits the `.llseek` callback on architectures without I/O mmap support (like x86 and arm64), causing `lseek(SEEK_END)` to incorrectly return 0 for I/O BAR sysfs files.
--

commit 2bb0cfc5ea48584962711dffb2f762f594fd8374
Author: Krzysztof Wilczyński <kwilczynski@kernel.org>

PCI/sysfs: Add static PCI resource attribute macros

This patch adds macros for declaring static binary attributes for PCI
resource files, setting specific callbacks based on the resource type.
It conditionally includes mmap support for I/O BAR resources based on
arch_can_pci_mmap_io().

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
[ ... ]
> @@ -1197,6 +1197,47 @@ static ssize_t pci_write_resource_io(struct file *filp, struct kobject *kobj,
>  	return pci_resource_io(filp, kobj, attr, buf, off, count, true);
>  }
>  
> +/*
> + * generic_file_llseek() consults f_mapping->host to determine
> + * the file size. As iomem_inode knows nothing about the
> + * attribute, it's not going to work, so override it as well.
> + */
> +#if arch_can_pci_mmap_io()
> +# define __PCI_RESOURCE_IO_MMAP_ATTRS		\
> +	.f_mapping = iomem_get_mapping,		\
> +	.llseek = pci_llseek_resource,		\
> +	.mmap = pci_mmap_resource_uc,
> +#else
> +# define __PCI_RESOURCE_IO_MMAP_ATTRS
> +#endif

On architectures where arch_can_pci_mmap_io() is false (such as x86 and arm64),
__PCI_RESOURCE_IO_MMAP_ATTRS evaluates to empty, meaning .llseek is left as
NULL.

Since these are static attributes, their .size field defaults to 0. When
.llseek is NULL, sysfs_kf_bin_llseek() falls back to generic_file_llseek().

Because the inode size is 0, will lseek(fd, 0, SEEK_END) incorrectly return
0 instead of the actual resource length, potentially breaking userspace tools
like pciutils?

Should .llseek = pci_llseek_resource be defined unconditionally to ensure
the correct dynamic resource length is computed?

> +
> +#define pci_dev_resource_io_attr(_bar)					\
> +static const struct bin_attribute dev_resource##_bar##_io_attr = {	\
> +	.attr = { .name = "resource" __stringify(_bar), .mode = 0600 },	\
> +	.private = (void *)(unsigned long)(_bar),			\
> +	.read = pci_read_resource_io,					\
> +	.write = pci_write_resource_io,					\
> +	__PCI_RESOURCE_IO_MMAP_ATTRS					\
> +}

Because the .size field of this static bin_attribute is uninitialized (0),
VFS bounds checks in sysfs_kf_bin_read() and sysfs_kf_bin_write() are
disabled.

If an arbitrarily large 64-bit positive file offset is passed via pread(),
does this offset truncate on 32-bit architectures?

Looking at pci_resource_io():

static ssize_t pci_resource_io(struct file *filp, struct kobject *kobj,
			       const struct bin_attribute *attr, char *buf,
			       loff_t off, size_t count, bool write)
{
#ifdef CONFIG_HAS_IOPORT
	...
	unsigned long port = off;
	...
	port += pci_resource_start(pdev, bar);

	if (port > pci_resource_end(pdev, bar))
		return 0;
	...
}

Could a 64-bit offset be crafted such that when truncated to 32 bits, it
becomes a negative value that exactly offsets pci_resource_start(pdev, bar)?

This would cause port to wrap around to a targeted port value.

Since the upper-bound check against pci_resource_end() would then pass,
and there is no lower-bound check, does this allow reading or writing any
physical I/O port between 0 and pci_resource_end?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508043543.217179-1-kwilczynski@kernel.org?part=6