Message-ID: <5ef1c30a-1740-00cc-ad16-4b1c1b02fca4@arm.com>
Date:   Wed, 16 Mar 2022 18:22:21 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     "Limonciello, Mario" <Mario.Limonciello@....com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     "michael.jamet@...el.com" <michael.jamet@...el.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "YehezkelShB@...il.com" <YehezkelShB@...il.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "andreas.noever@...il.com" <andreas.noever@...il.com>,
        "hch@....de" <hch@....de>
Subject: Re: [PATCH] thunderbolt: Stop using iommu_present()

On 2022-03-16 17:53, Limonciello, Mario wrote:
> [Public]
> 
>>>>>
>>>>> There is a way to figure out the "tunneled" PCIe ports by looking at
>>>>> certain properties and we do that already actually. The BIOS has the
>>>>> following under these ports:
>>>>>
>>>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
>>>>> .microsoft.com%2Fen-us%2Fwindows-
>> hardware%2Fdrivers%2Fpci%2Fdsd-
>>>>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root-
>>>>>
>> ports&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a
>>>>>
>> 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
>>>>>
>> C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
>>>>>
>> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
>>>>>
>> p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D
>>>>> &amp;reserved=0
>>>>>
>>>>> and the ports will have dev->external_facing set to 1. Perhaps looking
>>>>> at that field helps here?
>>>>
>>>> External facing isn't a guarantee from the firmware though.  It's
>> something we
>>>> all expect in practice, but I think it's better to look at the ones that are
>> from
>>>> the _DSD usb4-host-interface to be safer.
>>>
>>> Right but then we have the discrete ones with the DVSEC that exposes the
>>> tunneled ports :(
>>>
> 
> Can the USB4 CM make the device links in the DVSEC case perhaps too?  I would
> think we want that anyway to control device suspend ordering.
> 
> If I had something discrete to try I'd dust off the DVSEC patch I wrote before to
> try it, but alas all I have is integrated stuff on my hand.
> 
>>>> Mika, you might not have seen it yet, but I sent a follow up diff in this
>> thread
>>>> to Robin's patch.  If that looks good Robin can submit a v2 (or I'm happy to
>> do
>>>> so as well as I confirmed it helps my original intent too).
>>>
>>> I saw it now and I'm thinking are we making this unnecessary complex? I
>>> mean Microsoft solely depends on the DMAR platform opt-in flag:
>>>
>>>
>>
> 
> I think Microsoft doesn't allow you to turn off the IOMMU though or put it in
> passthrough through on the kernel command line.
> 
>>> We also do turn on full IOMMU mappings in that case for devices that are
>>> marked as external facing by the same firmware that provided the DMAR
>>> bit. If the user decides to disable IOMMU from command line for instance
>>> then we expect she knows what she is doing.
>>
>> Yeah, if external_facing is set correctly then we can safely expect the
>> the IOMMU layer to do the right thing, so in that case it probably is OK
>> to infer that if an IOMMU is present for the NHI then it'll be managing
>> that whole bus hierarchy. What I'm really thinking about here is whether
>> we can defend against a case when external_facing *isn't* set, so we
>> treat the tunnelled ports as normal PCI buses, assume it's OK since
>> we've got an IOMMU and everything else is getting translation domains by
>> default, but then a Thunderbolt device shows up masquerading the VID:DID
>> of something that gets a passthrough quirk, and thus tricks its way
>> through the perceived protection.
>>
>> Robin.
> 
> Unless it happened after 5.17-rc8 looking at the code I think that's Intel
> specific behavior though at the moment (has_external_pci).  I don't see it
> in a generic layer.

Ah, it's not necessarily the most obvious thing - 
pci_dev->external_facing gets propagated through to pci_dev->untrusted 
by set_pcie_untrusted(), and it's that that's then checked by 
iommu_get_def_domain_type() to enforce a translation domain regardless 
of default passthrough or quirks. It's then further checked by 
iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data 
leakage in sub-page mappings too.

> In addition to the point Robin said about firmware not setting external facing
> if the IOMMU was disabled on command line then iommu_dma_protection
> would be showing the wrong values meaning userspace may choose to
> authorize the device automatically in a potentially unsafe scenario.
> 
> Even if the user "knew what they were doing", I would expect that we still
> do our best to protect them from themselves and not advertise something
> that will cause automatic authorization.

Might it be reasonable for the Thunderbolt core to check early on if any 
tunnelled ports are not marked as external facing, and if so just tell 
the user that iommu_dma_protection is off the table and anything they 
authorise is at their own risk?

Robin.

Powered by blists - more mailing lists