* [PATCH] vfio_pci: Enable memory accesses before calling pci_map_rom
@ 2019-02-13 10:14 Eric Auger
2019-02-13 10:39 ` Auger Eric
2019-02-13 16:28 ` Konrad Rzeszutek Wilk
0 siblings, 2 replies; 4+ messages in thread
From: Eric Auger @ 2019-02-13 10:14 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, linux-kernel, kvm, alex.williamson
pci_map_rom/pci_get_rom_size() performs memory access in the ROM.
In case the Memory Space accesses were disabled, readw() is likely to
crash the host with a synchronous external abort (aarch64).
In case memory accesses were disabled, re-enable them before the call
and disable them back again just after.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
drivers/vfio/pci/vfio_pci.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index ff60bd1ea587..96b8bbd909d7 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -706,8 +706,10 @@ static long vfio_pci_ioctl(void *device_data,
break;
case VFIO_PCI_ROM_REGION_INDEX:
{
+ bool mem_access_disabled;
void __iomem *io;
size_t size;
+ u16 cmd;
info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
info.flags = 0;
@@ -723,6 +725,13 @@ static long vfio_pci_ioctl(void *device_data,
break;
}
+ pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+ mem_access_disabled = !(cmd & PCI_COMMAND_MEMORY);
+ if (mem_access_disabled) {
+ cmd |= PCI_COMMAND_MEMORY;
+ pci_write_config_word(pdev, PCI_COMMAND, cmd);
+ }
+
/* Is it really there? */
io = pci_map_rom(pdev, &size);
if (!io || !size) {
@@ -731,6 +740,11 @@ static long vfio_pci_ioctl(void *device_data,
}
pci_unmap_rom(pdev, io);
+ if (mem_access_disabled) {
+ cmd &= ~PCI_COMMAND_MEMORY;
+ pci_write_config_word(pdev, PCI_COMMAND, cmd);
+ }
+
info.flags = VFIO_REGION_INFO_FLAG_READ;
break;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] vfio_pci: Enable memory accesses before calling pci_map_rom
2019-02-13 10:14 [PATCH] vfio_pci: Enable memory accesses before calling pci_map_rom Eric Auger
@ 2019-02-13 10:39 ` Auger Eric
2019-02-13 16:28 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 4+ messages in thread
From: Auger Eric @ 2019-02-13 10:39 UTC (permalink / raw)
To: eric.auger.pro, linux-kernel, kvm, alex.williamson
Hi,
On 2/13/19 11:14 AM, Eric Auger wrote:
> pci_map_rom/pci_get_rom_size() performs memory access in the ROM.
> In case the Memory Space accesses were disabled, readw() is likely to
> crash the host with a synchronous external abort (aarch64).
>
> In case memory accesses were disabled, re-enable them before the call
> and disable them back again just after.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
> drivers/vfio/pci/vfio_pci.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index ff60bd1ea587..96b8bbd909d7 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -706,8 +706,10 @@ static long vfio_pci_ioctl(void *device_data,
> break;
> case VFIO_PCI_ROM_REGION_INDEX:
> {
> + bool mem_access_disabled;
> void __iomem *io;
> size_t size;
> + u16 cmd;
>
> info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> info.flags = 0;
> @@ -723,6 +725,13 @@ static long vfio_pci_ioctl(void *device_data,
> break;
> }
>
> + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> + mem_access_disabled = !(cmd & PCI_COMMAND_MEMORY);
> + if (mem_access_disabled) {
> + cmd |= PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> + }
> +
> /* Is it really there? */
> io = pci_map_rom(pdev, &size);
> if (!io || !size) {
> @@ -731,6 +740,11 @@ static long vfio_pci_ioctl(void *device_data,
> }
> pci_unmap_rom(pdev, io);
>
> + if (mem_access_disabled) {
> + cmd &= ~PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> + }
I failed to re-enable in case of error. I will respin.
Thanks
Eric
> +
> info.flags = VFIO_REGION_INFO_FLAG_READ;
> break;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] vfio_pci: Enable memory accesses before calling pci_map_rom
2019-02-13 10:14 [PATCH] vfio_pci: Enable memory accesses before calling pci_map_rom Eric Auger
2019-02-13 10:39 ` Auger Eric
@ 2019-02-13 16:28 ` Konrad Rzeszutek Wilk
2019-02-13 17:11 ` Alex Williamson
1 sibling, 1 reply; 4+ messages in thread
From: Konrad Rzeszutek Wilk @ 2019-02-13 16:28 UTC (permalink / raw)
To: Eric Auger; +Cc: eric.auger.pro, linux-kernel, kvm, alex.williamson
On Wed, Feb 13, 2019 at 11:14:06AM +0100, Eric Auger wrote:
> pci_map_rom/pci_get_rom_size() performs memory access in the ROM.
> In case the Memory Space accesses were disabled, readw() is likely to
> crash the host with a synchronous external abort (aarch64).
Ouch. Is there an CVE for this?
Also I think this can cause x86 machines to blow up.
See https://xenbits.xen.org/xsa/advisory-120.html
>
> In case memory accesses were disabled, re-enable them before the call
> and disable them back again just after.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
> drivers/vfio/pci/vfio_pci.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index ff60bd1ea587..96b8bbd909d7 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -706,8 +706,10 @@ static long vfio_pci_ioctl(void *device_data,
> break;
> case VFIO_PCI_ROM_REGION_INDEX:
> {
> + bool mem_access_disabled;
> void __iomem *io;
> size_t size;
> + u16 cmd;
>
> info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> info.flags = 0;
> @@ -723,6 +725,13 @@ static long vfio_pci_ioctl(void *device_data,
> break;
> }
>
> + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> + mem_access_disabled = !(cmd & PCI_COMMAND_MEMORY);
> + if (mem_access_disabled) {
> + cmd |= PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> + }
> +
> /* Is it really there? */
> io = pci_map_rom(pdev, &size);
> if (!io || !size) {
> @@ -731,6 +740,11 @@ static long vfio_pci_ioctl(void *device_data,
> }
> pci_unmap_rom(pdev, io);
>
> + if (mem_access_disabled) {
> + cmd &= ~PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> + }
> +
> info.flags = VFIO_REGION_INFO_FLAG_READ;
> break;
> }
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] vfio_pci: Enable memory accesses before calling pci_map_rom
2019-02-13 16:28 ` Konrad Rzeszutek Wilk
@ 2019-02-13 17:11 ` Alex Williamson
0 siblings, 0 replies; 4+ messages in thread
From: Alex Williamson @ 2019-02-13 17:11 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: Eric Auger, eric.auger.pro, linux-kernel, kvm
On Wed, 13 Feb 2019 11:28:21 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Wed, Feb 13, 2019 at 11:14:06AM +0100, Eric Auger wrote:
> > pci_map_rom/pci_get_rom_size() performs memory access in the ROM.
> > In case the Memory Space accesses were disabled, readw() is likely to
> > crash the host with a synchronous external abort (aarch64).
>
> Ouch. Is there an CVE for this?
>
> Also I think this can cause x86 machines to blow up.
>
> See https://xenbits.xen.org/xsa/advisory-120.html
The far more common response to a target abort on x86 is simply a -1
return. Device assignment quickly becomes unfeasible in the general
case, as outlined in the above link by restricting only to SR-IOV VFs,
if we try to claim there is no possible way that the device cannot
trigger a fatal error on the host. Systems implementing APEI pretty
much guarantee that by escalating device specific faults to fatal
errors and removing the host OS from the error handling path. Some
platforms will even trigger a fatal error for a DMA outside of the
range mapped for the device by the IOMMU. We can't probe for this
behavior to restrict the devices, we cannot know how DMA is programmed
for every device in order to babysit it, nor can we guarantee that the
PCI config space command register is the only way a device manages
access to I/O resources hosted on the device. Restricting user access
to the command register therefore seems like a false sense of security,
potentially with behavioral issues to the user.
It would be great if we always had a hook into the error handling path
such that we could declare this as a user generated fault, kill the
user process, and keep running, but we're limited by the error handling
capabilities of the hardware and the degree to which the
platform/firmware allows OS control of that error handling. Thanks,
Alex
> >
> > In case memory accesses were disabled, re-enable them before the call
> > and disable them back again just after.
> >
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > ---
> > drivers/vfio/pci/vfio_pci.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index ff60bd1ea587..96b8bbd909d7 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -706,8 +706,10 @@ static long vfio_pci_ioctl(void *device_data,
> > break;
> > case VFIO_PCI_ROM_REGION_INDEX:
> > {
> > + bool mem_access_disabled;
> > void __iomem *io;
> > size_t size;
> > + u16 cmd;
> >
> > info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> > info.flags = 0;
> > @@ -723,6 +725,13 @@ static long vfio_pci_ioctl(void *device_data,
> > break;
> > }
> >
> > + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > + mem_access_disabled = !(cmd & PCI_COMMAND_MEMORY);
> > + if (mem_access_disabled) {
> > + cmd |= PCI_COMMAND_MEMORY;
> > + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> > + }
> > +
> > /* Is it really there? */
> > io = pci_map_rom(pdev, &size);
> > if (!io || !size) {
> > @@ -731,6 +740,11 @@ static long vfio_pci_ioctl(void *device_data,
> > }
> > pci_unmap_rom(pdev, io);
> >
> > + if (mem_access_disabled) {
> > + cmd &= ~PCI_COMMAND_MEMORY;
> > + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> > + }
> > +
> > info.flags = VFIO_REGION_INFO_FLAG_READ;
> > break;
> > }
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-13 17:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-13 10:14 [PATCH] vfio_pci: Enable memory accesses before calling pci_map_rom Eric Auger
2019-02-13 10:39 ` Auger Eric
2019-02-13 16:28 ` Konrad Rzeszutek Wilk
2019-02-13 17:11 ` Alex Williamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).