2023-01-19 17:29:24

by Alexander Shishkin

[permalink] [raw]
Subject: [PATCH 1/2] PCI/MSI: Cache the MSIX table size

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 <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Cc: [email protected]
---
drivers/pci/msi/api.c | 7 ++++++-
drivers/pci/msi/msi.c | 2 +-
include/linux/pci.h | 1 +
3 files changed, 8 insertions(+), 2 deletions(-)

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


2023-01-22 09:15:01

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH 1/2] PCI/MSI: Cache the MSIX table size

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 <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Cc: [email protected]
> ---
> 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
>

2023-01-22 11:10:34

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH 1/2] PCI/MSI: Cache the MSIX table size

On Sun, 22 Jan 2023 09:00:04 +0000,
Leon Romanovsky <[email protected]> wrote:
>
> 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 <[email protected]>
> > Reviewed-by: Mika Westerberg <[email protected]>
> > Cc: [email protected]
> > ---
> > 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.
>

Indeed, this was my exact reaction reading this patch. This only makes
sure the same (potentially wrong) value is used at all times. So while
this results in a consistent use, this doesn't give much guarantee.

The only way to deal with this is to actually handle the resulting
fault, similar to what the kernel does when accessing userspace. Not
sure how possible this is with something like PCIe.

M.

--
Without deviation from the norm, progress is not possible.

2023-01-22 11:12:39

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 1/2] PCI/MSI: Cache the MSIX table size

On Sun, Jan 22, 2023 at 11:00:04AM +0200, Leon Romanovsky wrote:
> 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 <[email protected]>
> > Reviewed-by: Mika Westerberg <[email protected]>
> > Cc: [email protected]
> > ---
> > 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.

Linux does NOT protect from malicious PCIe devices at this point in
time, you are correct. If we wish to change that model, then we can
work on that with the explict understanding that most all drivers will
need to change as will the bus logic for the busses involved.

To do piece-meal patches like this for no good reason is not a good idea
as it achieves nothing in the end :(

thanks,

greg k-h