On Thu, 14 Feb 2019 19:27:15 +0100
Auger Eric <eric.auger@xxxxxxxxxx> wrote:
> Hi Alex,
>
> On 2/13/19 6:52 PM, Alex Williamson wrote:
> > On Wed, 13 Feb 2019 12:06:10 +0100
> > Eric Auger <eric.auger@xxxxxxxxxx> 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).
> >
> > As implied in response to Konrad, the likeliness really depends on the
> > whole platform, not just the CPU architecture. It's a class of
> > problems that depends on OS control or error handling, which we simply
> > don't have on many systems. But we can fix this instance of it.
>
> Agreed, I just hit this issue on one specific aarch64 machine
> >
> >> 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@xxxxxxxxxx>
> >
> > This has been around since the beginning, but maybe a Fixes tag would
> > be useful:
> >
> > Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver")
> OK
> >
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - also re-enable in case of error
> >> ---
> >> drivers/vfio/pci/vfio_pci.c | 17 ++++++++++++++++-
> >> 1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >> index ff60bd1ea587..721aa55424a4 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,15 +725,28 @@ 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) {
> >> info.size = 0;
> >> - break;
> >> + goto rom_info_out;
> >> }
> >> pci_unmap_rom(pdev, io);
> >>
> >> info.flags = VFIO_REGION_INFO_FLAG_READ;
> >> +rom_info_out:
> >> + if (mem_access_disabled) {
> >> + cmd &= ~PCI_COMMAND_MEMORY;
> >> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> >> + }
> >> +
> >> break;
> >> }
> >> case VFIO_PCI_VGA_REGION_INDEX:
> >
> > I don't think we need to be so timid about the command register and we
> > can also avoid the goto by modifying the test (testing io and size in
> > the original is probably overly paranoid), perhaps simply:
> Yes looks fine.
>
> Do you want to respin or do you prefer I do?
Please take it, test it, and repost it, I haven't tested it at all.
Thanks,
Alex
