Re: [PATCH RFC 3/3] selftests/vfio: Add PCIe Device Serial Number test

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Endianness mismatch in PCI configuration space reads leading to test failures on big-endian architectures.
- [Medium] Type punning from a byte array to structs with stricter alignment requirements violates strict aliasing and alignment rules.
- [Low] Capability walker fails to break on unpopulated extended configuration space (`0xffffffff`).

Pre-existing issues:
- [Critical] Missing bounds checking when parsing PCI extended capabilities causes heap buffer overflows.
--

commit 83546a8f398b5cb4fdb341d52a4a665cb8639619
Author: Pranjal Arya <pranjal.arya@oss.qualcomm.com>

selftests/vfio: Add PCIe Device Serial Number test

This patch adds a selftest to verify the vfio-pci DSN handling, ensuring that
the serial number is scrubbed by default and that the VFIO_DEVICE_FEATURE_PCI_DSN
feature correctly probes, sets, and gets the presented serial.

> diff --git a/tools/testing/selftests/vfio/vfio_pci_dsn_test.c b/tools/testing/selftests/vfio/vfio_pci_dsn_test.c
> new file mode 100644
> index 0000000000000..d7652ad725f4e
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/vfio_pci_dsn_test.c
> @@ -0,0 +1,206 @@
[ ... ]
> +/* Walk the extended capability chain and return the DSN cap offset, or 0. */
> +static u16 find_dsn_cap(struct vfio_pci_device *device)
> +{
> +	u16 pos = PCI_CFG_SPACE_SIZE;
> +	int loops = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) /
> +		    PCI_CAP_SIZEOF;
> +
> +	while (pos >= PCI_CFG_SPACE_SIZE && loops--) {
> +		u32 header = vfio_pci_config_readl(device, pos);

[Severity: Medium]
Since vfio_pci_config_readl() reads raw little-endian bytes directly from the
device, will reading them into a host-endian u32 without a conversion like
le32toh() break capability header parsing on big-endian architectures?

> +
> +		if (!header)

[Severity: Low]
When a device implements no further PCIe extended capabilities, the config
header typically reads as 0xffffffff.

Does checking only for 0 miss the standard 0xffffffff value, causing the loop
to continuously re-read offset 0xffc and wastefully execute ioctls until the
safety counter expires?

> +			break;
> +
> +		if (PCI_EXT_CAP_ID(header) == PCI_EXT_CAP_ID_DSN)
> +			return pos;
> +
> +		pos = PCI_EXT_CAP_NEXT(header);
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int dsn_feature(struct vfio_pci_device *device, u32 op, u64 *serial)
> +{
> +	u8 buf[sizeof(struct vfio_device_feature) +
> +	       sizeof(struct vfio_device_feature_pci_dsn)] = {};

[Severity: Medium]
Does this byte array guarantee only 1-byte alignment?

Because tools/ is compiled with standard -fstrict-aliasing, casting this byte
array to structs with stricter alignment requirements could violate strict
aliasing and alignment rules.

> +	struct vfio_device_feature *feature = (void *)buf;
> +	struct vfio_device_feature_pci_dsn *dsn = (void *)feature->data;
> +
> +	feature->argsz = sizeof(buf);
> +	feature->flags = op | VFIO_DEVICE_FEATURE_PCI_DSN;
> +
> +	if ((op & VFIO_DEVICE_FEATURE_SET) && serial)
> +		dsn->serial_number = *serial;

[Severity: Medium]
Will dereferencing the pointer to this 8-byte aligned struct member from the
1-byte aligned buf[] cause unaligned memory access faults on strict
architectures?

> +
> +	if (ioctl(device->fd, VFIO_DEVICE_FEATURE, feature))
[ ... ]
> +/* SET then GET must round-trip, and the guest-visible bytes must match. */
> +TEST_F(vfio_pci_dsn_test, set_get_roundtrip)
> +{
> +	u64 want = 0x0123456789abcdefULL;
> +	u64 got = 0;
> +
> +	if (!self->dsn_pos)
> +		SKIP(return, "Device has no DSN capability\n");
> +
> +	ASSERT_EQ(0, dsn_feature(self->device, VFIO_DEVICE_FEATURE_SET, &want));
> +	ASSERT_EQ(0, dsn_feature(self->device, VFIO_DEVICE_FEATURE_GET, &got));
> +	ASSERT_EQ(want, got);
> +
> +	ASSERT_EQ((u32)want,
> +		  vfio_pci_config_readl(self->device, self->dsn_pos + PCI_DSN_LOW_DW));

[Severity: Medium]
Will comparing a host-endian expected value against raw little-endian bytes
read by vfio_pci_config_readl() cause false test failures on big-endian
architectures?


[Severity: Critical]
This is a pre-existing issue, but I noticed missing bounds checking when
parsing PCI extended capabilities that could cause heap buffer overflows.

Looking at vfio_ecap_init() in drivers/vfio/pci/vfio_pci_config.c:

    memset(map + epos, ecap, len);

This writes ecap bytes into map based on an unverified capability length, which
could lead to an out-of-bounds write onto the kernel heap if a malicious
emulated or physical device places a capability near the end of the extended
config space (so that epos + len exceeds pdev->cfg_size).

Additionally, looking at vfio_pci_core_feature_dsn() in
drivers/vfio/pci/vfio_pci_config.c:

    WRITE_ONCE(vserial[0], cpu_to_le32(lower_32_bits(dsn.serial_number)));

Could this directly write 8 bytes to vdev->vconfig at pos + 4 without verifying
that pos + 12 <= pdev->cfg_size, similarly writing user-controlled bytes out of
bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-pcie_vfio-v1-0-09168188f3f2@oss.qualcomm.com?part=3