RE: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD systems

["Limonciello, Mario" <Mario.Limonciello@amd.com>]

[Public]



> -----Original Message-----
> From: Limonciello, Mario
> Sent: Tuesday, March 15, 2022 13:36
> To: Robin Murphy <robin.murphy@arm.com>; Christoph Hellwig
> <hch@infradead.org>; christian@kellner.me; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> Cc: Michael Jamet <michael.jamet@intel.com>; open list:THUNDERBOLT
> DRIVER <linux-usb@vger.kernel.org>; open list <linux-
> kernel@vger.kernel.org>; Yehezkel Bernat <YehezkelShB@gmail.com>;
> open list:AMD IOMMU (AMD-VI) <iommu@lists.linux-foundation.org>;
> Andreas Noever <andreas.noever@gmail.com>; Will Deacon
> <will@kernel.org>
> Subject: Re: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD
> systems
> 
> + Christian Kellner (Bolt userspace maintainer)
> 
> On 3/15/2022 13:07, Robin Murphy wrote:
> > On 2022-03-15 16:54, Limonciello, Mario via iommu wrote:
> >> [Public]
> >>
> >>
> >>> On Tue, Mar 15, 2022 at 11:24:55AM -0500, Mario Limonciello wrote:
> >>>> -     * handled natively using IOMMU. It is enabled when IOMMU is
> >>>> -     * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN
> set.
> >>>> +     * handled natively using IOMMU. It is enabled when the IOMMU is
> >>>> +     * enabled and either:
> >>>> +     * ACPI DMAR table has DMAR_PLATFORM_OPT_IN set
> >>>> +     * or
> >>>> +     * ACPI IVRS table has DMA_REMAP bitset
> >>>>        */
> >>>>       return sprintf(buf, "%d\n",
> >>>> -               iommu_present(&pci_bus_type) &&
> >>> dmar_platform_optin());
> >>>> +               iommu_present(&pci_bus_type) &&
> >>>> +               (dmar_platform_optin() || amd_ivrs_remap_support()));
> >>>
> >>> Yikes.  No, the thunderbot code does not have any business poking into
> >>> either dmar_platform_optin or amd_ivrs_remap_support.  This needs
> >>> a proper abstration from the IOMMU code.
> >>
> >> To make sure I follow your ask - it's to make a new generic iommu
> >> function
> >> That would check dmar/ivrs, and switch out thunderbolt domain.c to use
> >> the
> >> symbol?
> >>
> >> I'm happy to rework that if that is what you want.
> >> Do you have a preferred proposed function name for that?
> >
> > But why? Either IOMMU translation is enabled or it isn't, and if it is,
> > what's to gain from guessing at *why* it might have been? And even if
> > the IOMMU's firmware table did tell the IOMMU driver to enable the
> > IOMMU, why should that be Thunderbolt's business?
> A lot of this comes from baggage from early Thunderbolt 3 implementation
> on systems with ICM (Intel's FW CM). On those systems there was a
> concept of "Security Levels".  This meant that downstream PCIe devices
> were not automatically authorized when a TBT3 device was plugged in.  In
> those cases there was no guarantee that the IOMMU was in use and so the
> security was passed on to the user to make a decision.
> 
> In Linux this was accomplished using the 'authorized' attribute in
> /sys/bus/thunderbolt/devices/$NUM/authorized.  When this was set to 1
> then the TBT3 device and PCIe topology behind it would be enumerated.
> 
> Further documentation explaining how this works is available here:
> https://www.kernel.org/doc/html/latest/admin-
> guide/thunderbolt.html#security-levels-and-how-to-use-them
> 
> (Intel based) Platforms from 2018+ w/ TBT3 started to use the IOMMU
> consistently at runtime but had this existing implementation of security
> levels to worry about.  Furthermore tunnels could be created pre-boot,
> and so the thunderbolt driver may or may not re-create them based on
> policy.
> 
> So a new attribute was created "iommu_dma_protection" that userspace
> could use as part of a policy decision to automatically authorize
> devices.  Exporting this attribute is very similar to what Microsoft
> does to let the user see the security of the system.
> 
> https://docs.microsoft.com/en-us/windows-hardware/design/device-
> experiences/oem-kernel-dma-protection
> 
> In Linux today some userspace software "bolt" has a policy included by
> default that will automatically authorize TBT3 and USB4 (w/ PCIe)
> devices when iommu_dma_protection is set to 1.
> 
> >
> > Furthermore, looking at patch #1 I can only conclude that this is
> > entirely meaningless anyway. AFAICS it's literally reporting whether the
> > firmware flag was set or not. Not whether it's actually been honoured
> > and the IOMMU is enforcing any kind of DMA protection at all. Even on
> > Intel where the flag does at least have some effect on the IOMMU driver,
> > that can still be overridden.
> 
> Take a look at the Microsoft link I shared above.  They also make policy
> decisions based on the information in these tables.
> 
> >
> > I already have a patch refactoring this to get rid of iommu_present(),
> > but at the time I wasn't looking to closely at what it's trying to *do*
> > with the information. If it's supposed to accurately reflect whether the
> > Thunderbolt device is subject to IOMMU translation and not bypassed, I
> > can fix that too (and unexport dmar_platform_optin() in the process...)
> >
> > Robin.
> 
> This patch series stems from that history.  To give the best experience
> to end users you want hotplugged devices to be automatically authorized
> when software says it's safe to do so.
> 
> To summarize the flow:
> * User plugs in device
> * USB4 CM will query supported tunnels
> * USB4 CM will create devices in /sys/bus/thunderbolt/devices for new
> plugged in TBT3/USB4 device
> * "authorized" attribute will default to "0" and PCIe tunnels are not
> created
> * Userspace gets a uevent that the device was added
> * Userspace (bolt) reacts by reading
> /sys/bus/thunderbolt/devices/domainX/iommu_dma_protection
> * If that is set to "1", bolt will write "1" to "authorized"  and USB4
> CM will create PCIe tunnels
> * If that is set to "0", bolt will send an event to GUI to show a popup
> asking to authorize the device
> * After user acks the authorization then it will write "1" to
> "authorized" and USB4 CM will create PCIe tunnels
> 
> 
> Mika,
> 
> I wonder if maybe what we really want is to only use that flow for the
> authorized attribute when using TBT3 + ICM (or IOMMU disabled at
> runtime).  If we're using a USB4 host, check IOMMU translation layer
> active like Robin suggested and then automatically authorize from the CM.
> 
> Thoughts?
> 
>
 
I put an RFC together with what this idea looks like, comments welcome.
https://lore.kernel.org/linux-usb/20220315213008.5357-1-mario.limonciello@amd.com/T/#u