On Sun, 24 Nov 2024 16:27:39 +0200
Avihai Horon <avihaih@xxxxxxxxxx> wrote:
> There are cases where a PCIe extended capability should be hidden from
> the user. For example, an unknown capability (i.e., capability with ID
> greater than PCI_EXT_CAP_ID_MAX) or a capability that is intentionally
> chosen to be hidden from the user.
>
> Hiding a capability is done by virtualizing and modifying the 'Next
> Capability Offset' field of the previous capability so it points to the
> capability after the one that should be hidden.
>
> The special case where the first capability in the list should be hidden
> is handled differently because there is no previous capability that can
> be modified. In this case, the capability ID and version are zeroed
> while leaving the next pointer intact. This hides the capability and
> leaves an anchor for the rest of the capability list.
>
> However, today, hiding the first capability in the list is not done
> properly if the capability is unknown, as struct
> vfio_pci_core_device->pci_config_map is set to the capability ID during
> initialization but the capability ID is not properly checked later when
> used in vfio_config_do_rw(). This leads to the following warning [1] and
> to an out-of-bounds access to ecap_perms array.
>
> Fix it by checking cap_id in vfio_config_do_rw(), and if it is greater
> than PCI_EXT_CAP_ID_MAX, use an alternative struct perm_bits for direct
> read only access instead of the ecap_perms array.
>
> Note that this is safe since the above is the only case where cap_id can
> exceed PCI_EXT_CAP_ID_MAX (except for the special capabilities, which
> are already checked before).
>
> [1]
>
> WARNING: CPU: 118 PID: 5329 at drivers/vfio/pci/vfio_pci_config.c:1900 vfio_pci_config_rw+0x395/0x430 [vfio_pci_core]
> CPU: 118 UID: 0 PID: 5329 Comm: simx-qemu-syste Not tainted 6.12.0+ #1
> (snip)
> Call Trace:
> <TASK>
> ? show_regs+0x69/0x80
> ? __warn+0x8d/0x140
> ? vfio_pci_config_rw+0x395/0x430 [vfio_pci_core]
> ? report_bug+0x18f/0x1a0
> ? handle_bug+0x63/0xa0
> ? exc_invalid_op+0x19/0x70
> ? asm_exc_invalid_op+0x1b/0x20
> ? vfio_pci_config_rw+0x395/0x430 [vfio_pci_core]
> ? vfio_pci_config_rw+0x244/0x430 [vfio_pci_core]
> vfio_pci_rw+0x101/0x1b0 [vfio_pci_core]
> vfio_pci_core_read+0x1d/0x30 [vfio_pci_core]
> vfio_device_fops_read+0x27/0x40 [vfio]
> vfs_read+0xbd/0x340
> ? vfio_device_fops_unl_ioctl+0xbb/0x740 [vfio]
> ? __rseq_handle_notify_resume+0xa4/0x4b0
> __x64_sys_pread64+0x96/0xc0
> x64_sys_call+0x1c3d/0x20d0
> do_syscall_64+0x4d/0x120
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver")
> Signed-off-by: Avihai Horon <avihaih@xxxxxxxxxx>
> Reviewed-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> Tested-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
> Changes from v2:
> * Fix clang compilation error reported by kernel test robot.
> * Drop const qualifier of direct_ro_perms to avoid casting in
> vfio_config_do_rw and to be aligned with other perms declaration.
> * Add Yi's R-b/T-b tags.
Applied to vfio next branch for v6.13. Thanks,
Alex
> Changes from v1:
> * Use Alex's suggestion to fix the bug and adapt the commit message.
> ---
> drivers/vfio/pci/vfio_pci_config.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 97422aafaa7b..ea2745c1ac5e 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -313,6 +313,10 @@ static int vfio_virt_config_read(struct vfio_pci_core_device *vdev, int pos,
> return count;
> }
>
> +static struct perm_bits direct_ro_perms = {
> + .readfn = vfio_direct_config_read,
> +};
> +
> /* Default capability regions to read-only, no-virtualization */
> static struct perm_bits cap_perms[PCI_CAP_ID_MAX + 1] = {
> [0 ... PCI_CAP_ID_MAX] = { .readfn = vfio_direct_config_read }
> @@ -1897,9 +1901,17 @@ static ssize_t vfio_config_do_rw(struct vfio_pci_core_device *vdev, char __user
> cap_start = *ppos;
> } else {
> if (*ppos >= PCI_CFG_SPACE_SIZE) {
> - WARN_ON(cap_id > PCI_EXT_CAP_ID_MAX);
> + /*
> + * We can get a cap_id that exceeds PCI_EXT_CAP_ID_MAX
> + * if we're hiding an unknown capability at the start
> + * of the extended capability list. Use default, ro
> + * access, which will virtualize the id and next values.
> + */
> + if (cap_id > PCI_EXT_CAP_ID_MAX)
> + perm = &direct_ro_perms;
> + else
> + perm = &ecap_perms[cap_id];
>
> - perm = &ecap_perms[cap_id];
> cap_start = vfio_find_cap_start(vdev, *ppos);
> } else {
> WARN_ON(cap_id > PCI_CAP_ID_MAX);
