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?
Thanks
Eric
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index ff60bd1ea587..659b7c1ea8fb 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -708,6 +708,7 @@ static long vfio_pci_ioctl(void *device_data,
> {
> void __iomem *io;
> size_t size;
> + u16 orig_cmd;
>
> info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
> info.flags = 0;
> @@ -723,15 +724,23 @@ static long vfio_pci_ioctl(void *device_data,
> break;
> }
>
> - /* Is it really there? */
> + /*
> + * Is it really there? Enable memory decode for
> + * implicit access in pci_map_rom().
> + */
> + pci_read_config_word(pdev, PCI_COMMAND, &orig_cmd);
> + pci_write_config_word(pdev, PCI_COMMAND,
> + orig_cmd | PCI_COMMAND_MEMORY);
> +
> io = pci_map_rom(pdev, &size);
> - if (!io || !size) {
> + if (io) {
> + info.flags = VFIO_REGION_INFO_FLAG_READ;
> + pci_unmap_rom(pdev, io);
> + } else
> info.size = 0;
> - break;
> - }
> - pci_unmap_rom(pdev, io);
>
> - info.flags = VFIO_REGION_INFO_FLAG_READ;
> + pci_write_config_word(pdev, PCI_COMMAND, orig_cmd);
> +
> break;
> }
> case VFIO_PCI_VGA_REGION_INDEX:
>
