Message-ID: <BL1PR12MB5157244DC83C6A776D0CDB87E2119@BL1PR12MB5157.namprd12.prod.outlook.com>
Date:   Wed, 16 Mar 2022 19:25:09 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@....com>
To:     Robin Murphy <robin.murphy@....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()

[Public]



> -----Original Message-----
> From: Robin Murphy <robin.murphy@....com>
> Sent: Wednesday, March 16, 2022 14:18
> To: Limonciello, Mario <Mario.Limonciello@....com>; Mika Westerberg
> <mika.westerberg@...ux.intel.com>
> Cc: michael.jamet@...el.com; linux-usb@...r.kernel.org; linux-
> kernel@...r.kernel.org; YehezkelShB@...il.com; iommu@...ts.linux-
> foundation.org; andreas.noever@...il.com; hch@....de
> 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.

Powered by blists - more mailing lists