Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices
On Thu, Nov 15, 2018 at 7:46 PM Lorenzo Pieralisi wrote: > > On Thu, Nov 15, 2018 at 02:16:27PM +0200, Mika Westerberg wrote: > > On Thu, Nov 15, 2018 at 01:07:36PM +0100, Lukas Wunner wrote: > > > On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote: > > > > On Thu, Nov 15, 2018 at 11:13:56AM +, Lorenzo Pieralisi wrote: > > > > > I have strong objections to the way these bindings have been forced > > > > > upon > > > > > everybody; if that's the way *generic* ACPI bindings are specified I > > > > > wonder why there still exists an ACPI specification and related > > > > > working > > > > > group. > > > > > > > > > > I personally (but that's Bjorn and Rafael choice) think that this is > > > > > not a change that belongs in PCI core, ACPI bindings are ill-defined > > > > > and device tree bindings are non-existing. > > > > > > > > Any idea where should I put it then? These systems are already out there > > > > and we need to support them one way or another. > > > > > > I suppose those are all Thunderbolt, so could be handled by the > > > existing ->is_thunderbolt bit? > > > > > > It was said in this thread that ->is_external is more generic in > > > that it could also be used on PCIe slots, however that use case > > > doesn't appear to lend itself to the "plug in while laptop owner > > > is getting coffee" attack. To access PCIe slots on a server you > > > normally need access to a data center. On a desktop, you usually > > > have to open the case, by which time the coffee may already have > > > been fetched. So frankly the binding seems a bit over-engineered > > > to me and yet another thing that BIOS writers may get wrong. > > > > I would not say it should include PCIe slots but there are other cables > > that carry PCIe and I was thinking we could make it to support those as > > well. > > > > I have no problem using is_thunderbolt here, though if we don't want to > > support non-Thunderbolt external devices this way. > > > > However, the question here is more that where I should put the _DSD > > parsing code if it is not suitable to be placed inside PCI/ACPI core as > > I've done in this patch? ;-) > > Do you really need to parse it if the dev->is_thunderbolt check is enough ? >From what I know, there are more devices that suffer from similar security issues like Thunderbolt, e.g. FireWire [1]. My assumption is that the same protection may be applied to such devices too, even if currently it sounds like vendors care mostly about Thunderbolt (probably because it removes the need for user approval for device connection; it becames a simple plug-and-play experience). Thus, I don't think binding it with dev->is_thunderbolt is the correct thing to do. [1] https://github.com/carmaa/inception ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace
On Tue, Nov 13, 2018 at 5:20 PM Mika Westerberg wrote: > > On Tue, Nov 13, 2018 at 04:42:58PM +0200, Yehezkel Bernat wrote: > > On Tue, Nov 13, 2018 at 1:40 PM Mika Westerberg > > wrote: > > > > > > On Tue, Nov 13, 2018 at 01:13:31PM +0200, Yehezkel Bernat wrote: > > > > On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg > > > > wrote: > > > > > > > > > > > Just one point: > > > > > > Have you considered the option to add this property per (TBT?) > > > > > > device? > > > > > > > > > > No. ;-) > > > > > > > > > > You mean that one device uses security levels and another IOMMU? I > > > > > don't > > > > > think it is possible without having some sort of table in the IOMMU > > > > > driver telling which devices it needs identity map and which not. Also > > > > > not sure what would be the benefit? > > > > > > > > For performance, of course. If some devices are considered safe (maybe > > > > a list > > > > communicated by platform firmware), the kernel may decide to configure > > > > them to > > > > passthrough the IOMMU (I think I remember there is such an option, but > > > > maybe I'm > > > > wrong.) > > > > > > At least I'm not aware of such an option. Windows for example enables > > > IOMMU for everything and I think macOS does the same. In Linux (with > > > these patches) we put all internal devices already passthrough mode so > > > things like internal graphics should not be affected. eGPUs are > > > different thing, though. > > > > So your point here is "currently we do the IOMMU decisions system-wide; we > > can > > always add a per-device attribute if needed"? > > Well, let me elaborate a bit :) I think it is possible to put certain > devices into IOMMU passthrough mode even if they are "external" but I'm > not that familiar with the guts of Intel IOMMU (Baolu or Ashok are the > experts). Assuming it is possible we could have a table or similar in > the kernel that can be used to identify devices that are allowed to be > in passthrough mode. > > I'm not sure if it can be per-device sysfs attribute because at the time > the PCIe device is enumerated it is already put to its IOMMU group. Good point. But I thought about per-TBT-device decision. If the platform is configured for IOMMU+"user" security level, while approving the device the user may want to set also in which IOMMU group to put all the PCIe devices connected to it. The same goes if kernel is supposed to auto-approve such devices based on an internal table. The point is that we can think on a configuration where the devices aren't tunneled yet and the decision about IOMMU can still be changed. As you mentioned this isn't the common configuration anyway, so it probably doesn't worth all this hassle. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace
On Tue, Nov 13, 2018 at 1:40 PM Mika Westerberg wrote: > > On Tue, Nov 13, 2018 at 01:13:31PM +0200, Yehezkel Bernat wrote: > > On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg > > wrote: > > > > > > > Just one point: > > > > Have you considered the option to add this property per (TBT?) device? > > > > > > No. ;-) > > > > > > You mean that one device uses security levels and another IOMMU? I don't > > > think it is possible without having some sort of table in the IOMMU > > > driver telling which devices it needs identity map and which not. Also > > > not sure what would be the benefit? > > > > For performance, of course. If some devices are considered safe (maybe a > > list > > communicated by platform firmware), the kernel may decide to configure them > > to > > passthrough the IOMMU (I think I remember there is such an option, but > > maybe I'm > > wrong.) > > At least I'm not aware of such an option. Windows for example enables > IOMMU for everything and I think macOS does the same. In Linux (with > these patches) we put all internal devices already passthrough mode so > things like internal graphics should not be affected. eGPUs are > different thing, though. So your point here is "currently we do the IOMMU decisions system-wide; we can always add a per-device attribute if needed"? Fair enough. So for this patch, Reviewed-by: Yehezkel Bernat ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace
On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg wrote: > > > Just one point: > > Have you considered the option to add this property per (TBT?) device? > > No. ;-) > > You mean that one device uses security levels and another IOMMU? I don't > think it is possible without having some sort of table in the IOMMU > driver telling which devices it needs identity map and which not. Also > not sure what would be the benefit? For performance, of course. If some devices are considered safe (maybe a list communicated by platform firmware), the kernel may decide to configure them to passthrough the IOMMU (I think I remember there is such an option, but maybe I'm wrong.) > > If the kernel may decide to enable/disable the IOMMU or AST per device, > > maybe > > it should be on this level. > > Or maybe the IOMMU decision isn't going to change (it's system-wide) and > > the AST > > decision will be communicated per device by a new sysfs attribute anyway, if > > needed? > > Not sure what you mean by "AST"? The IOMMU decision is pretty much > system-wide. Sorry, I meant ATS, Address Translation Service, mentioned in patch 3 in this series, and possibly be enabled for some devices for performance, as mentioned there. So if needed, this will be another attribute, and definitely per-device, isn't it? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/4] PCI / iommu / thunderbolt: IOMMU based DMA protection
On Mon, Nov 12, 2018 at 8:12 PM Lukas Wunner wrote: > > On Mon, Nov 12, 2018 at 07:06:24PM +0300, Mika Westerberg wrote: > > Recent systems shipping with Windows 10 version 1803 or newer may be > > utilizing IOMMU to prevent DMA attacks via Thunderbolt ports. This is > > different from the previous security level based scheme because the > > connected device cannot access system memory outside of the regions > > allocated for it by the driver. > > > > When enabled the BIOS makes sure no device can do DMA outside of RMRR > > (Reserved Memory Region Record) regions. This means that during OS boot, > > before it enables IOMMU, none of the connected devices can bypass DMA > > protection for instance by overwriting the data structures used by the > > IOMMU. The BIOS communicates support for this to the OS by setting a new > > bit in ACPI DMAR table [1]. > > > > Because these systems utilize an IOMMU to block possible DMA attacks, > > typically (but not always) the Thunderbolt security level is set to "none" > > which means that all PCIe devices are immediately usable. This also means > > that Linux needs to follow Windows 10 and enable IOMMU automatically when > > running on such system otherwise connected devices can read/write system > > memory pretty much without any restrictions. > > What if the system is booted from a Thunderbolt-attached disk? > Won't this suddenly break with these patches? That would seem like a > pretty significant regression. My assumption is that either it isn't supported on such platforms (at least with this security configuration active) so this doesn't break anything, it never worked there, or the BIOS configures IOMMU in a way that allows the disk to work until the OS will take control and configure the IOMMU according to OS decisions. In the latter case, the kernel+initrd will be loaded before IOMMU configuration will be changed, and then the kernel should be able to config it correctly to work. (Unless I really don't understand the mechanism and workflow of using IOMMU, which is possible.) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace
On Mon, Nov 12, 2018 at 6:06 PM Mika Westerberg wrote: > > Recent systems shipping with Windows 10 version 1803 or later may > support a feature called Kernel DMA protection [1]. In practice this > means that Thunderbolt connected devices are placed behind an IOMMU > during the whole time it is connected (including during boot) making > Thunderbolt security levels redundant. Some of these systems still have > Thunderbolt security level set to "user" in order to support OS > downgrade (the older version of the OS might not support IOMMU based DMA > protection so connecting a device still relies on user approval then). > > Export this information to userspace by introducing a new sysfs > attribute (iommu_dma_protection). Based on it userspace tools can make > more accurate decision whether or not authorize the connected device. > > In addition update Thunderbolt documentation regarding IOMMU based DMA > protection. > > [1] > https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt > > Signed-off-by: Mika Westerberg > --- I can't comment on the IOMMU side, but the Thunderbolt side looks good to me. Just one point: Have you considered the option to add this property per (TBT?) device? If the kernel may decide to enable/disable the IOMMU or AST per device, maybe it should be on this level. Or maybe the IOMMU decision isn't going to change (it's system-wide) and the AST decision will be communicated per device by a new sysfs attribute anyway, if needed? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
