Re: [PATCH v2 1/2] VT-d: avoid NULL deref on domain_context_mapping_one() error paths

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2022 18:31, Jason Andryuk wrote:
> Hi,
> 
> On Thu, Apr 7, 2022 at 3:50 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 07.04.2022 09:41, Roger Pau Monné wrote:
>>> On Thu, Apr 07, 2022 at 08:11:06AM +0200, Jan Beulich wrote:
>>>> First there's a printk() which actually wrongly uses pdev in the first
>>>> place: We want to log the coordinates of the (perhaps fake) device
>>>> acted upon, which may not be pdev.
>>>>
>>>> Then it was quite pointless for eb19326a328d ("VT-d: prepare for per-
>>>> device quarantine page tables (part I)") to add a domid_t parameter to
>>>> domain_context_unmap_one(): It's only used to pass back here via
>>>> me_wifi_quirk() -> map_me_phantom_function(). Drop the parameter again.
>>>>
>>>> Finally there's the invocation of domain_context_mapping_one(), which
>>>> needs to be passed the correct domain ID. Avoid taking that path when
>>>> pdev is NULL and the quarantine state is what would need restoring to.
>>>> This means we can't security-support PCI devices with RMRRs (if such
>>>> exist in practice) any longer.
>>>
>>> The sentence:
>>>
>>> "This means we can't security-support PCI devices with RMRRs"
>>>
>>> Seems too broad and could lead to confusion. So I would maybe use:
>>> "legacy PCI devices" or "non PCI Express devices".
>>
>> Right. I did actually forget to either drop or edit that sentence. I've
>> now extended this to
>>
>> "This means we can't security-support non-PCI-Express devices with RMRRs
>>  (if such exist in practice) any longer; note that as of trhe 1st of the
>>  two commits referenced below assigning them to DomU-s is unsupported
>>  anyway."
> 
> Mentioning "Express" makes the support statement clearer.  However,
> I'm not clear on what unsupported means in "assigning them to DomU-s
> is unsupported anyway".  They can't be assigned?  I'm testing with
> staging-4.16, so with XSA-400, but not this patch.  I seemingly have a
> legacy PCI device still being assigned to a domU.
> 
> It is an 8th Gen Intel laptop with:
> 00:14.0 USB controller: Intel Corporation Cannon Point-LP USB 3.1 xHCI
> Controller (rev 30) (prog-if 30 [XHCI]).
> 
> lspci output for 00:14.0 does *not* have capability "Express (v2)",
> but it does have an RMRR:
> (XEN) [VT-D]found ACPI_DMAR_RMRR:
> (XEN) [VT-D] endpoint: 0000:00:14.0
> 
> It looks like it is PCI compared to 39:00.0 which does have Express (v2):
> (XEN) [VT-D]d1:PCI: map 0000:00:14.0
> (XEN) [VT-D]d1:PCIe: map 0000:39:00.0
> 
> As I understand it, an RMRR is common with USB controllers for
> implementing legacy mouse & keyboard support.  The Cannon Point PCH is
> fairly modern, so I'd expect it to use PCI Express.  Xen seems to
> identify it as DEV_TYPE_PCI given "PCI" above.  It is an integrated
> PCI device, so it has no upstream bridge.  Maybe that is why it can
> still be assigned?  The "unsupported assignment" is a legacy PCI
> device, behind a bridge, with an RMRR?

Ah yes - the "behind a bridge" aspect does matter (but I can't
adjust the description of an already committed patch). That's both
for the respective part of the XSA-400 series as well as for the
change here.

Jan