netdev - Re: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu

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:   Fri, 29 Mar 2019 14:50:34 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     laurentiu.tudor@....com, netdev@...r.kernel.org,
        madalin.bucur@....com, roy.pledge@....com, camelia.groza@....com,
        leoyang.li@....com
Cc:     linux-kernel@...r.kernel.org, iommu@...ts.linux-foundation.org,
        linuxppc-dev@...ts.ozlabs.org, davem@...emloft.net,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 02/13] soc/fsl/bman: map FBPR area in the iommu

On 29/03/2019 14:00, laurentiu.tudor@....com wrote:
> From: Laurentiu Tudor <laurentiu.tudor@....com>
> 
> Add a one-to-one iommu mapping for bman private data memory (FBPR).
> This is required for BMAN to work without faults behind an iommu.
> 
> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@....com>
> ---
>   drivers/soc/fsl/qbman/bman_ccsr.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/soc/fsl/qbman/bman_ccsr.c b/drivers/soc/fsl/qbman/bman_ccsr.c
> index 7c3cc968053c..b209c79511bb 100644
> --- a/drivers/soc/fsl/qbman/bman_ccsr.c
> +++ b/drivers/soc/fsl/qbman/bman_ccsr.c
> @@ -29,6 +29,7 @@
>    */
>   
>   #include "bman_priv.h"
> +#include <linux/iommu.h>
>   
>   u16 bman_ip_rev;
>   EXPORT_SYMBOL(bman_ip_rev);
> @@ -178,6 +179,7 @@ static int fsl_bman_probe(struct platform_device *pdev)
>   	int ret, err_irq;
>   	struct device *dev = &pdev->dev;
>   	struct device_node *node = dev->of_node;
> +	struct iommu_domain *domain;
>   	struct resource *res;
>   	u16 id, bm_pool_cnt;
>   	u8 major, minor;
> @@ -225,6 +227,15 @@ static int fsl_bman_probe(struct platform_device *pdev)
>   
>   	dev_dbg(dev, "Allocated FBPR 0x%llx 0x%zx\n", fbpr_a, fbpr_sz);
>   
> +	/* Create an 1-to-1 iommu mapping for FBPR area */
> +	domain = iommu_get_domain_for_dev(dev);

If that's expected to be the default domain that you're grabbing, then 
this is *incredibly* fragile. There's nothing to stop the IOVA that you 
forcibly map from being automatically allocated later and causing some 
other DMA mapping to fail noisily and unexpectedly. Furthermore, have 
you tried this with "iommu.passthrough=1"?

That said, I really don't understand what's going on here anyway :/

As far as I can tell from qbman_init_private_mem(), fbpr_a comes from 
dma_alloc_coherent() and thus would already be a mapped IOVA - isn't 
this the stuff that Roy converted to nicely use shared-dma-pool regions 
a while ago?

Robin.

> +	if (domain) {
> +		ret = iommu_map(domain, fbpr_a, fbpr_a, fbpr_sz,
> +				IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
> +		if (ret)
> +			dev_warn(dev, "failed to iommu_map() %d\n", ret);
> +	}
> +
>   	bm_set_memory(fbpr_a, fbpr_sz);
>   
>   	err_irq = platform_get_irq(pdev, 0);
>