linux-kernel - RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton

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]

Date:   Wed, 6 Apr 2022 10:44:54 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>,
        Jason Gunthorpe <jgg@...dia.com>
CC:     "Alex Williamson (alex.williamson@...hat.com)" 
        <alex.williamson@...hat.com>, "Raj, Ashok" <ashok.raj@...el.com>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Christoph Hellwig" <hch@...radead.org>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "Pan, Jacob jun" <jacob.jun.pan@...el.com>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>
Subject: RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()

> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Wednesday, April 6, 2022 6:02 PM
> 
> Hi Kevin,
> 
> On 2022/4/2 15:12, Tian, Kevin wrote:
> >>>> Add a flag to the group that positively indicates the group can never
> >>>> have more than one member, even after hot plug. eg because it is
> >>>> impossible due to ACS, or lack of bridges, and so on.
> >>> OK, I see your point. It essentially refers to a singleton group which
> >>> is immutable to hotplug.
> >> Yes, known at creation time, not retroactively enforced because
> >> someone used SVA
> >>
> > We may check following conditions to set the immutable flag when
> > a new group is created for a device in pci_device_group():
> >
> > 1) ACS is enabled in the upstream path of the device;
> > 2) the device is single function or ACS is enabled on a multi-function device;
> > 3) the device type is PCI_EXP_TYPE_ENDPOINT (thus no hotplug);
> > 4) no 'dma aliasing' on this device;
> >
> > The last one is a bit conservative as it also precludes a device which aliasing
> > dma due to quirks from being treated as a singleton group. But doing so
> > saves the effort on trying to separate different aliasing scenarios as defined
> > in pci_for_each_dma_alias(). Probably we can go this way as the first step.
> >
> > Once the flag is set on a group no other event can change it. If a new
> > identified device hits an existing singleton group in pci_device_group()
> > then it's a bug.
> 
> How about below implementation?
> 
> /* callback for pci_for_each_dma_alias() */
> static int has_pci_alias(struct pci_dev *pdev, u16 alias, void *opaque)
> {
> 	return -EEXIST;
> }
> 
> static bool pci_dev_is_immutably_isolated(struct pci_dev *pdev)
> {
> 	/* Skip bridges. */
> 	if (pci_is_bridge(pdev))
> 		return false;
> 
> 	/* Either connect to root bridge or the ACS-enabled bridge. */
> 	if (!pci_is_root_bus(pdev->bus) &&
> 	    !pci_acs_enabled(pdev->bus->self, REQ_ACS_FLAGS))
> 		return false;

it's not sufficient to just check the non-root bridge itself. This needs to
cover the entire path from the bridge to the root port, as pci_device_group()
does.

> 
> 	/* ACS is required for MFD. */
> 	if (pdev->multifunction && !pci_acs_enabled(pdev, REQ_ACS_FLAGS))
> 		return false;

Above two checks be replaced by a simple check as below:

	if (!pci_acs_path_enabled(pdev, NULL, REQ_ACS_FLAGS))
		return false;

> 
> 	/* Make sure no PCI alias. */
> 	if (pci_for_each_dma_alias(pdev, has_pci_alias, NULL))
> 		return false;
> 
> 	return true;
> }
> 
> I didn't get why do we need to check the PCI_EXP_TYPE_ENDPOINT device
> type. Can you please elaborate a bit more?
> 

I didn't know there is a pci_is_bridge() facility thus be conservative
to restrict it to only endpoint device. If checking pci_is_bridge() alone
excludes any hotplug possibility, then it's definitely better.

Thanks
Kevin