Re: [XEN PATCH] tools/libs/light/libxl_pci.c: explicitly grant access to Intel IGD opregion

[Chuck Zmudzinski <brchuckz@netscape.net>]

On 3/15/2022 9:27 PM, Chuck Zmudzinski wrote:
>
>
> On 3/15/22 7:38 AM, Jan Beulich wrote:
>> On 14.03.2022 04:41, Chuck Zmudzinski wrote:
>>
>>> @@ -610,6 +612,45 @@ out:
>>>       return ret;
>>>   }
>>>   +static uint32_t sysfs_dev_get_igd_opregion(libxl__gc *gc,
>>> +                                           libxl_device_pci *pci)
>>> +{
>>> +    char *pci_device_config_path =
>>> +            GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/config",
>>> +                      pci->domain, pci->bus, pci->dev, pci->func);
>>> +    size_t read_items;
>>> +    uint32_t igd_opregion;
>>> +    uint32_t error = 0xffffffff;
>> I think this constant wants to gain a #define, to be able to correlate
>> the use sites. I'm also not sure of the value - in principle the
>> register can hold this value, but of course then it won't be 3 pages.
>>
>
> I have one more comment to add here. I am not intending
> to define igd_opregion as a data structure 3 pages (12k)

Correction: Actually, the igd_opregion itself would be 2 pages.
The three pages comes from the fact that it is not guaranteed
to be page aligned, so it will take three pages to ensure
that it will be fully mapped to the guest. From the commit
message in hvmloader that increased it from two to three
pages:

From: Keir Fraser <keir@xxxxxxx>
Date: Thu, 10 Jan 2013 17:26:24 +0000 (+0000)
Subject: hvmloader: Allocate 3 pages for Intel GPU OpRegion passthrough.
X-Git-Tag: 4.3.0-rc1~546
X-Git-Url:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff_plain;h=408a9e56343b006c9e58a334f0b97dd2deedf9ac

hvmloader: Allocate 3 pages for Intel GPU OpRegion passthrough.

The 8kB region may not be page aligned, hence requiring 3 pages to
be mapped through.

Signed-off-by: Keir Fraser <keir@xxxxxxx>

In tests on my system, this is true. It was, IIRC,
0x18 (24) bytes offset from a page boundary.

This has an unfortunate side effect of granting
access to one page that the guest does not really
need access to. My well-behaved and trusted Linux
and Windows guests only request the two pages of
the igd_opregion, but it could have accessed the 24
bytes before it or the (4k - 24) bytes after it. I don't
think that greatly increases the security risk of including
this patch, because I think with passthrough of PCI
devices, it must always be to a trusted guest for it to
be secure. I don't think an attacker who gained control
over a guest that has PCI devices passed through to it
would need this exploit to successfully attack the dom0
or control domain from the guest. The damage could
be done whether or not the attacker has access to
that extra page if the attacker gained full control over
a guest with PCI devices passed through to it.

Regards,

Chuck