On 2022-03-16 18:34, Limonciello, Mario wrote:
> [Public]
>
>>> 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.
>>
>
> Ah thanks for explaining it, that was immediately obvious to me.
>
>>> 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.
>
> How about in iommu_dma_protection_show to just check that all the device
> links to the NHI are marked as untrusted?
>
> Then if there are device links missing we solve that separately (discrete USB4
> DVSEC case we just need to make those device links).
The feeling I'm getting from all this is that if we've got as far as
iommu_dma_protection_show() then it's really too late to meaningfully
mitigate bad firmware. We should be able to detect missing
untrusted/external-facing properties as early as nhi_probe(), and if we
could go into "continue at your own risk" mode right then *before*
anything else happens, it all becomes a lot easier to reason about. If
there's a strong enough impetus from Microsoft for system vendors to get
their firmware right, hopefully we can get away with not trying too hard
to cope with systems that haven't.
I'm inclined to send v2 of this patch effectively going back to my
original (even simpler) cleanup, just now with much more reasoning about
why it isn't doing more :)
Cheers,
Robin.
[Public]
> -----Original Message-----
> From: Robin Murphy <[email protected]>
> Sent: Wednesday, March 16, 2022 14:18
> To: Limonciello, Mario <[email protected]>; Mika Westerberg
> <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected]
> foundation.org; [email protected]; [email protected]
> Subject: Re: [PATCH] thunderbolt: Stop using iommu_present()
>
> On 2022-03-16 18:34, Limonciello, Mario wrote:
> > [Public]
> >
> >>> 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.
> >>
> >
> > Ah thanks for explaining it, that was immediately obvious to me.
> >
> >>> 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.
> >
> > How about in iommu_dma_protection_show to just check that all the
> device
> > links to the NHI are marked as untrusted?
> >
> > Then if there are device links missing we solve that separately (discrete
> USB4
> > DVSEC case we just need to make those device links).
>
> The feeling I'm getting from all this is that if we've got as far as
> iommu_dma_protection_show() then it's really too late to meaningfully
> mitigate bad firmware. We should be able to detect missing
> untrusted/external-facing properties as early as nhi_probe(), and if we
> could go into "continue at your own risk" mode right then *before*
> anything else happens, it all becomes a lot easier to reason about. If
> there's a strong enough impetus from Microsoft for system vendors to get
> their firmware right, hopefully we can get away with not trying too hard
> to cope with systems that haven't.
>
> I'm inclined to send v2 of this patch effectively going back to my
> original (even simpler) cleanup, just now with much more reasoning about
> why it isn't doing more :)
>
Yeah I'm fine with your patch code as it stands right now.
In that case how about a second patch in the series to dev_warn in drivers/thunderbolt/acpi.c
right when the link is made if it's not set as trusted? That should happen right during
tb_probe as you suggest then.
Hi Robin,
On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
> The feeling I'm getting from all this is that if we've got as far as
> iommu_dma_protection_show() then it's really too late to meaningfully
> mitigate bad firmware.
Note, these are requirements from Microsoft in order for the system to
use the "Kernel DMA protection". Because of this, likelyhood of "bad
firmware" should be quite low since these systems ship with Windows
installed so they should get at least some soft of validation that this
actually works.
> We should be able to detect missing
> untrusted/external-facing properties as early as nhi_probe(), and if we
> could go into "continue at your own risk" mode right then *before* anything
> else happens, it all becomes a lot easier to reason about.
I think what we want is that the DMAR opt-in bit is set in the ACPI
tables and that we know the full IOMMU translation is happening for the
devices behind "external facing ports". If that's not the case the
iommu_dma_protection_show() should return 0 meaning the userspace can
ask the user whether the connected device is allowed to use DMA (e.g
PCIe is tunneled or not).
We do check for the DMAR bit in the Intel IOMMU code and we also do
check that there actually are PCIe ports marked external facing but we
could issue warning there if that's not the case. Similarly if the user
explicitly disabled the IOMMU translation. This can be done inside a new
IOMMU API that does something like the below pseudo-code:
#if IOMMU_ENABLED
bool iommu_dma_protected(struct device *dev)
{
if (dmar_platform_optin() /* or the AMD equivalent */) {
if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
dev_warn(dev, "IOMMU protection disabled!");
/*
* Look for the external facing ports. Should be at
* least 1 or issue warning.
*/
...
return true;
}
return false;
}
#else
static inline bool iommu_dma_protected(struct device *dev)
{
return false;
}
#endif
Then we can make iommu_dma_protection_show() to call this function.
On 2022-03-17 08:08, Mika Westerberg wrote:
> Hi Robin,
>
> On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
>> The feeling I'm getting from all this is that if we've got as far as
>> iommu_dma_protection_show() then it's really too late to meaningfully
>> mitigate bad firmware.
>
> Note, these are requirements from Microsoft in order for the system to
> use the "Kernel DMA protection". Because of this, likelyhood of "bad
> firmware" should be quite low since these systems ship with Windows
> installed so they should get at least some soft of validation that this
> actually works.
>
>> We should be able to detect missing
>> untrusted/external-facing properties as early as nhi_probe(), and if we
>> could go into "continue at your own risk" mode right then *before* anything
>> else happens, it all becomes a lot easier to reason about.
>
> I think what we want is that the DMAR opt-in bit is set in the ACPI
> tables and that we know the full IOMMU translation is happening for the
> devices behind "external facing ports". If that's not the case the
> iommu_dma_protection_show() should return 0 meaning the userspace can
> ask the user whether the connected device is allowed to use DMA (e.g
> PCIe is tunneled or not).
Ah, if it's safe to just say "no protection" in the case that we don't
know for sure, that's even better. Clearly I hadn't quite grasped that
aspect of the usage model, thanks for the nudge!
> We do check for the DMAR bit in the Intel IOMMU code and we also do
> check that there actually are PCIe ports marked external facing but we
> could issue warning there if that's not the case. Similarly if the user
> explicitly disabled the IOMMU translation. This can be done inside a new
> IOMMU API that does something like the below pseudo-code:
>
> #if IOMMU_ENABLED
> bool iommu_dma_protected(struct device *dev)
> {
> if (dmar_platform_optin() /* or the AMD equivalent */) {
> if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
> dev_warn(dev, "IOMMU protection disabled!");
> /*
> * Look for the external facing ports. Should be at
> * least 1 or issue warning.
> */
> ...
>
> return true;
> }
>
> return false;
> }
> #else
> static inline bool iommu_dma_protected(struct device *dev)
> {
> return false;
> }
> #endif
>
> Then we can make iommu_dma_protection_show() to call this function.
The problem that I've been trying to nail down here is that
dmar_platform_optin() really doesn't mean much for us - I don't know how
Windows' IOMMU drivers work, but there's every chance it's not the
same way as ours. The only material effect that dmar_platform_optin()
has for us is to prevent the user from disabling the IOMMU driver
altogether, and thus ensure that iommu_present() is true. Whether or not
we can actually trust the IOMMU driver to provide reliable protection
depends entirely on whether it knows the PCIe ports are external-facing.
If not, we can only *definitely* know what the IOMMU driver will do for
a given endpoint once that endpoint has appeared behind the port and
iommu_probe_device() has decided what its default domain should be, and
as far as I now understand, that's not an option for Thunderbolt since
it can only happen *after* the tunnel has been authorised and created.
Much as I'm tempted to de-scope back to my IOMMU API cleanup and run
away from the rest of the issue, I think I can crib enough from the
existing code to attempt a reasonable complete fix, so let me give that
a go...
Thanks,
Robin.
Hi Robin,
On Thu, Mar 17, 2022 at 01:42:56PM +0000, Robin Murphy wrote:
> On 2022-03-17 08:08, Mika Westerberg wrote:
> > Hi Robin,
> >
> > On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote:
> > > The feeling I'm getting from all this is that if we've got as far as
> > > iommu_dma_protection_show() then it's really too late to meaningfully
> > > mitigate bad firmware.
> >
> > Note, these are requirements from Microsoft in order for the system to
> > use the "Kernel DMA protection". Because of this, likelyhood of "bad
> > firmware" should be quite low since these systems ship with Windows
> > installed so they should get at least some soft of validation that this
> > actually works.
> >
> > > We should be able to detect missing
> > > untrusted/external-facing properties as early as nhi_probe(), and if we
> > > could go into "continue at your own risk" mode right then *before* anything
> > > else happens, it all becomes a lot easier to reason about.
> >
> > I think what we want is that the DMAR opt-in bit is set in the ACPI
> > tables and that we know the full IOMMU translation is happening for the
> > devices behind "external facing ports". If that's not the case the
> > iommu_dma_protection_show() should return 0 meaning the userspace can
> > ask the user whether the connected device is allowed to use DMA (e.g
> > PCIe is tunneled or not).
>
> Ah, if it's safe to just say "no protection" in the case that we don't know
> for sure, that's even better. Clearly I hadn't quite grasped that aspect of
> the usage model, thanks for the nudge!
There is some documentation here too, hope it is helpful:
https://docs.kernel.org/admin-guide/thunderbolt.html
> > We do check for the DMAR bit in the Intel IOMMU code and we also do
> > check that there actually are PCIe ports marked external facing but we
> > could issue warning there if that's not the case. Similarly if the user
> > explicitly disabled the IOMMU translation. This can be done inside a new
> > IOMMU API that does something like the below pseudo-code:
> >
> > #if IOMMU_ENABLED
> > bool iommu_dma_protected(struct device *dev)
> > {
> > if (dmar_platform_optin() /* or the AMD equivalent */) {
> > if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */
> > dev_warn(dev, "IOMMU protection disabled!");
> > /*
> > * Look for the external facing ports. Should be at
> > * least 1 or issue warning.
> > */
> > ...
> >
> > return true;
> > }
> >
> > return false;
> > }
> > #else
> > static inline bool iommu_dma_protected(struct device *dev)
> > {
> > return false;
> > }
> > #endif
> >
> > Then we can make iommu_dma_protection_show() to call this function.
>
> The problem that I've been trying to nail down here is that
> dmar_platform_optin() really doesn't mean much for us - I don't know how
> Windows' IOMMU drivers work, but there's every chance it's not the same way
> as ours. The only material effect that dmar_platform_optin() has for us is
> to prevent the user from disabling the IOMMU driver altogether, and thus
> ensure that iommu_present() is true. Whether or not we can actually trust
> the IOMMU driver to provide reliable protection depends entirely on whether
> it knows the PCIe ports are external-facing. If not, we can only
> *definitely* know what the IOMMU driver will do for a given endpoint once
> that endpoint has appeared behind the port and iommu_probe_device() has
> decided what its default domain should be, and as far as I now understand,
> that's not an option for Thunderbolt since it can only happen *after* the
> tunnel has been authorised and created.
That's correct. We do know the PCIe root/downstream ports (the external
facing ones) that host the tunneled PCIe topology but rest will appear
dynamically after the connection manager established the protocol
tunnel.
> Much as I'm tempted to de-scope back to my IOMMU API cleanup and run away
> from the rest of the issue, I think I can crib enough from the existing code
> to attempt a reasonable complete fix, so let me give that a go...
Sure ;-)