[Public] > -----Original Message----- > From: Limonciello, Mario > Sent: Tuesday, March 15, 2022 13:36 > To: Robin Murphy <robin.murphy@xxxxxxx>; Christoph Hellwig > <hch@xxxxxxxxxxxxx>; christian@xxxxxxxxxx; Mika Westerberg > <mika.westerberg@xxxxxxxxxxxxxxx> > Cc: Michael Jamet <michael.jamet@xxxxxxxxx>; open list:THUNDERBOLT > DRIVER <linux-usb@xxxxxxxxxxxxxxx>; open list <linux- > kernel@xxxxxxxxxxxxxxx>; Yehezkel Bernat <YehezkelShB@xxxxxxxxx>; > open list:AMD IOMMU (AMD-VI) <iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx>; > Andreas Noever <andreas.noever@xxxxxxxxx>; Will Deacon > <will@xxxxxxxxxx> > 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@xxxxxxx/T/#u
