On Thu, Jan 19, 2023 at 07:06:32PM +0200, Alexander Shishkin wrote:
> A malicious device can change its MSIX table size between the table
> ioremap() and subsequent accesses, resulting in a kernel page fault in
> pci_write_msg_msix().
>
> To avoid this, cache the table size observed at the moment of table
> ioremap() and use the cached value. This, however, does not help drivers
> that peek at the PCIE_MSIX_FLAGS register directly.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> drivers/pci/msi/api.c | 7 ++++++-
> drivers/pci/msi/msi.c | 2 +-
> include/linux/pci.h | 1 +
> 3 files changed, 8 insertions(+), 2 deletions(-)
I'm not security expert here, but not sure that this protects from anything.
1. Kernel relies on working and not-malicious HW. There are gazillion ways
to cause crashes other than changing MSI-X.
2. Device can report large table size, kernel will cache it and
malicious device will reduce it back. It is not handled and will cause
to kernel crash too.
Thanks
>
> diff --git a/drivers/pci/msi/api.c b/drivers/pci/msi/api.c
> index b8009aa11f3c..617ea1256487 100644
> --- a/drivers/pci/msi/api.c
> +++ b/drivers/pci/msi/api.c
> @@ -75,8 +75,13 @@ int pci_msix_vec_count(struct pci_dev *dev)
> if (!dev->msix_cap)
> return -EINVAL;
>
> + if (dev->flags_qsize)
> + return dev->flags_qsize;
> +
> pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
> - return msix_table_size(control);
> + dev->flags_qsize = msix_table_size(control);
> +
> + return dev->flags_qsize;
> }
> EXPORT_SYMBOL(pci_msix_vec_count);
>
> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 1f716624ca56..d50cd45119f1 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -715,7 +715,7 @@ static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries,
>
> pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
> /* Request & Map MSI-X table region */
> - tsize = msix_table_size(control);
> + tsize = pci_msix_vec_count(dev);
> dev->msix_base = msix_map_region(dev, tsize);
> if (!dev->msix_base) {
> ret = -ENOMEM;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index adffd65e84b4..2e1a72a2139d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -352,6 +352,7 @@ struct pci_dev {
> u8 rom_base_reg; /* Config register controlling ROM */
> u8 pin; /* Interrupt pin this device uses */
> u16 pcie_flags_reg; /* Cached PCIe Capabilities Register */
> + u16 flags_qsize; /* Cached MSIX table size */
> unsigned long *dma_alias_mask;/* Mask of enabled devfn aliases */
>
> struct pci_driver *driver; /* Driver bound to this device */
> --
> 2.39.0
>
