Re: [PATCH RFC 2/3] vfio/pci: Allow userspace to set a virtual Device Serial Number

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The VFIO_DEVICE_FEATURE_PCI_DSN ioctl can cause an out-of-bounds heap read or write on the `vdev->vconfig` buffer if a device exposes a DSN capability near the end of its PCIe configuration space.
- [Medium] UAPI documentation states `serial_number` is in little-endian wire order, but the implementation processes it as a host-endian `u64`.
- [Medium] The use of `WRITE_ONCE()` on the writer side does not prevent tearing because the concurrent reader side uses `memcpy()`, leading to a data race.
--

commit 519ac218356009252a82c7aa68a221467812de3f
Author: Pranjal Arya <pranjal.arya@oss.qualcomm.com>

vfio/pci: Allow userspace to set a virtual Device Serial Number

This patch adds a VFIO_DEVICE_FEATURE_PCI_DSN device feature that lets the
trusted userspace VMM GET or SET the 64-bit serial number presented in the
device's virtualized config space.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 24dfeb43cb714..60c19129c8145 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -2078,3 +2078,62 @@ bool vfio_pci_core_range_intersect_range(loff_t buf_start, size_t buf_cnt,
[ ... ]
> +int vfio_pci_core_feature_dsn(struct vfio_pci_core_device *vdev, u32 flags,
> +			      void __user *arg, size_t argsz)
> +{
[ ... ]
> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DSN);
> +	if (!pos)
> +		return -ENOTTY;
> +
> +	ret = vfio_check_feature(flags, argsz,
> +				 VFIO_DEVICE_FEATURE_GET |
> +				 VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(dsn));
> +	if (ret != 1)
> +		return ret;
> +
> +	vserial = (__le32 *)&vdev->vconfig[pos + PCI_DSN_LOW_DW];

[Severity: High]
Does this code risk an out-of-bounds heap read or write on vdev->vconfig?

pci_find_ext_capability() only guarantees that the 4-byte capability header
fits within pdev->cfg_size. If a device exposes a DSN capability near the
end of its PCIe configuration space (for example, at offset 4092), pos will
be 4092.

Since vdev->vconfig is allocated with exactly pdev->cfg_size bytes, accessing
pos + PCI_DSN_LOW_DW (offset 4) will calculate a pointer completely out of
bounds, causing the subsequent operations to access adjacent kernel heap memory.

> +
> +	if (flags & VFIO_DEVICE_FEATURE_SET) {
> +		if (copy_from_user(&dsn, arg, sizeof(dsn)))
> +			return -EFAULT;
> +
> +		/*
> +		 * The config-space read path (vfio_default_config_read())
> +		 * does not hold a lock, and a guest reads the DSN as two
> +		 * 32-bit dwords.  Store each dword with WRITE_ONCE() so a
> +		 * concurrent guest read observes a consistent dword; a guest
> +		 * reading the two halves around this update may see an
> +		 * old/new mix, exactly as hardware may tear a 64-bit read of
> +		 * a register pair.  This matches the DSN's read-only,
> +		 * advisory nature.  Serializing concurrent SET callers is the
> +		 * userspace VMM's responsibility.
> +		 */
> +		WRITE_ONCE(vserial[0], cpu_to_le32(lower_32_bits(dsn.serial_number)));
> +		WRITE_ONCE(vserial[1], cpu_to_le32(upper_32_bits(dsn.serial_number)));

[Severity: Medium]
Does using WRITE_ONCE() here actually prevent torn reads by the guest?

The comments state this is to ensure a concurrent guest read observes a
consistent dword. However, the guest config-space read path in
vfio_default_config_read() uses memcpy(), which performs plain, non-atomic
memory accesses.

Because the compiler and processor are free to split the memcpy() load into
byte-sized operations, WRITE_ONCE() on the writer side does not guarantee
that the reader will observe an atomic 32-bit read.

> +		return 0;
> +	}
[ ... ]
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 5de618a3a5eee..e5b8dfd3833f6 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1511,6 +1511,24 @@ struct vfio_device_feature_bus_master {
[ ... ]
> +/**
> + * Upon VFIO_DEVICE_FEATURE_SET, set the PCIe Device Serial Number (DSN)
> + * presented to the user (guest) in the device's virtualized config space.
> + * By default vfio-pci scrubs the physical DSN to zero so the host device's
> + * unique identifier is not leaked; this feature lets the hypervisor present
> + * a chosen, per-VM synthetic serial instead (for example a stable but
> + * non-host-identifying value for migration).
> + *
> + * Upon VFIO_DEVICE_FEATURE_GET, read back the currently presented serial.
> + *
> + * The feature is only available for devices that expose a DSN capability.
> + * serial_number is the 64-bit serial in little-endian wire order.
> + */
> +struct vfio_device_feature_pci_dsn {
> +	__aligned_u64 serial_number;
> +};

[Severity: Medium]
Is there an endianness mismatch between this documentation and the
implementation?

The documentation states serial_number is the 64-bit serial in little-endian
wire order. However, vfio_pci_core_feature_dsn() treats it as a host-endian
value when extracting halves:

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

If a userspace program on a big-endian system follows the documentation and
provides a pre-formatted little-endian value, the kernel will interpret it as
a big-endian integer, incorrectly extract the halves, and store garbled bytes
into the virtual config space.

Should the documentation be updated to indicate a host-endian 64-bit integer,
or should the code be changed to expect an __le64 type?

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