linux-kernel - Re: [PATCH] PCI/MSI: Improve MSI alias detection

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: <20170619215214.GB554@bhelgaas-glaptop.roam.corp.google.com>
Date:   Mon, 19 Jun 2017 16:52:14 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Robin Murphy <robin.murphy@....com>
Cc:     bhelgaas@...gle.com, marc.zyngier@....com,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        David Daney <david.daney@...ium.com>,
        Alex Williamson <alex.williamson@...hat.com>
Subject: Re: [PATCH] PCI/MSI: Improve MSI alias detection

[+cc David, Alex]

On Wed, May 31, 2017 at 06:52:28PM +0100, Robin Murphy wrote:
> Currently, we consider all DMA aliases when calculating MSI requester
> IDs. This turns out to be the wrong thing to do in the face of pure DMA
> quirks like those of Marvell SATA cards, where we can end up configuring
> the MSI doorbell to expect the phantom function alias, such that it then
> goes on to happily ignore actual MSI writes coming from the card on the
> real RID.

I guess you're making the assumption that DMA might use phantom
function aliases (aliases on the same bus as the actual device), but
MSI writes will not?

> Improve the alias walk to only account for the topological aliases that
> matter, based on the logic from the Intel IRQ remapping code.

Can you include a specific function reference here?  It sounds like
it'd be useful to compare this with the Intel IRQ remapping code.

> Signed-off-by: Robin Murphy <robin.murphy@....com>
> ---
>  drivers/pci/msi.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index ba44fdfda66b..7b34c434970e 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1468,13 +1468,21 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
>  
> +/*
> + * If the alias device/RID is on a different bus, it's a topological alias
> + * we should care about; otherwise, it's a DMA quirk and we don't.
> + */
>  static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	u32 *pa = data;
> +	u8 bus = PCI_BUS_NUM(*pa);
> +
> +	if (pdev->bus->number != bus || PCI_BUS_NUM(alias) != bus)
> +		*pa = alias;
>  
> -	*pa = alias;

Aliases can arise both because of device defects, e.g., the phantom
function quirks, and because of bridges, e.g., all the stuff in
pci_for_each_dma_alias().

Why is it that the users of get_msi_id_cb() can get away with
collapsing any of these bridge aliases into the last one that
pci_for_each_dma_alias() finds?  I guess this is another question
about exactly why DMA and MSI are different here.

>  	return 0;
>  }
> +
>  /**
>   * pci_msi_domain_get_msi_rid - Get the MSI requester id (RID)
>   * @domain:	The interrupt domain
> @@ -1488,7 +1496,7 @@ static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>  u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>  {
>  	struct device_node *of_node;
> -	u32 rid = 0;
> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>  
>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>  
> @@ -1504,14 +1512,14 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>   * @pdev:	The PCI device
>   *
>   * Use the firmware data to find a device-specific MSI domain
> - * (i.e. not one that is ste as a default).
> + * (i.e. not one that is set as a default).
>   *
> - * Returns: The coresponding MSI domain or NULL if none has been found.
> + * Returns: The corresponding MSI domain or NULL if none has been found.
>   */
>  struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
>  {
>  	struct irq_domain *dom;
> -	u32 rid = 0;
> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>  
>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>  	dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> -- 
> 2.12.2.dirty
>