linux-kernel - Re: [PATCH 1/1] iommu/vt-d: Add opt-in for ATS support on discrete devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f0ddf129-ec38-05d2-07dc-be5f97d37c78@linux.intel.com>
Date:   Thu, 2 Mar 2023 09:56:39 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     baolu.lu@...ux.intel.com, iommu@...ts.linux.dev,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Kevin Tian <kevin.tian@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] iommu/vt-d: Add opt-in for ATS support on discrete
 devices

On 3/1/23 10:04 PM, Jason Gunthorpe wrote:
>> Here we are talking about soc-integrated devices vs. discrete PCI
>> devices (connected to the system through internal PCI slot). The problem
>> is that the DMAR ACPI table only defines ATS attribute for Soc-
>> integrated devices, which causes ATS (and its ancillary features) on the
>> discrete PCI devices not to work.
> So, IMHO, it is a bug to set what Linux calls as untrusted for
> discrete slots. We also definately don't want internal slots forced to
> non-identity mode either, for example.

Linux doesn't set untrusted for PCI discrete slots. It reads port's
ExternalFacingPort property and set pdev->untrusted for all devices
underneath it. For ACPI compatible platforms, this property is only set
for thunderbolt ports as far as I have seen.

drivers/pci/pci-acpi.c:
1373 static void pci_acpi_set_external_facing(struct pci_dev *dev)
1374 {
1375         u8 val;
1376
1377         if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
1378                 return;
1379         if (device_property_read_u8(&dev->dev, 
"ExternalFacingPort", &val))
1380                 return;
1381
1382         /*
1383          * These root ports expose PCIe (including DMA) outside of the
1384          * system.  Everything downstream from them is external.
1385          */
1386         if (val)
1387                 dev->external_facing = 1;
1388 }

and

drivers/pci/probe.c:
1597 static void set_pcie_untrusted(struct pci_dev *dev)
1598 {
1599         struct pci_dev *parent;
1600
1601         /*
1602          * If the upstream bridge is untrusted we treat this device
1603          * untrusted as well.
1604          */
1605         parent = pci_upstream_bridge(dev);
1606         if (parent && (parent->untrusted || parent->external_facing))
1607                 dev->untrusted = true;
1608 }

> 
> This should be addressed at the PCI layer. Untrusted should always
> mean that the iommu layer should fully secure the device. It means
> identity translation should be blocked, it means the HW should reject
> translated requests, and ATS thus is not supported.
> 
> Every single iommu driver should implement this consistently across
> the whole subsystem.
> 
> If you don't want devices to be secured then that is a PCI/bios layer
> problem to get the correct data into the untrusted flag.
> 
> Not an iommu problem to ignore the flag.

The problem here is that, even for PCI *trusted* devices, the IOMMU
drivers (Intel and SMMUv3) still have a allowed list for ATS. Only
devices in this list are allowed to use ATS. This cause some PCI
discrete devices not able to use ATS even its pdev->untrust is *not*
set.

The purpose of this patch is to allow privileged users to choose to skip
the allowed list according to their wishes. So that, as long as the PCI
layer treats the device as trusted, it can use ATS in the IOMMU layer.

At present, this patch is only for VT-d driver, but I have no objection
to bring it up to the core as a common mechanism.

Best regards,
baolu