linux-kernel - Re: [PATCH v3] drm/msm: gpu: don't abuse dma

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] [day] [month] [year] [list]

Message-ID: <20170726211908.GR20973@minitux>
Date:   Wed, 26 Jul 2017 14:19:08 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Rob Clark <robdclark@...il.com>, David Airlie <airlied@...ux.ie>,
        Jordan Crouse <jcrouse@...eaurora.org>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Sushmita Susheelendra <ssusheel@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] drm/msm: gpu: don't abuse dma_alloc for non-DMA
 allocations

On Wed 26 Jul 12:59 PDT 2017, Arnd Bergmann wrote:

> In zap_shader_load_mdt(), we pass a pointer to a phys_addr_t
> into dmam_alloc_coherent, which the compiler warns about:
> 
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: In function 'zap_shader_load_mdt':
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c:54:50: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 
> The returned DMA address is later passed on to a function that
> takes a phys_addr_t, so it's clearly wrong to use the DMA
> mapping interface here: the memory may be uncached, or the
> address may be completely wrong if there is an IOMMU connected
> to the device. What the code actually wants to do is to get
> the physical address from the reserved-mem node. It goes through
> the dma-mapping interfaces for obscure reasons, and this
> apparently only works by chance, relying on specific bugs
> in the error handling of the arm64 dma-mapping implementation.
> 
> The same problem existed in the "venus" media driver, which was
> now fixed by Stanimir Varbanov after long discussions.
> 
> In order to make some progress here, I have now ported his
> approach over to the adreno driver. The patch is currently
> untested, and should get a good review, but it is now much
> simpler than the original, and it should be obvious what
> goes wrong if I made a mistake in the port.
> 
> See also: a6e2d36bf6b7 ("media: venus: don't abuse dma_alloc for non-DMA allocations")
> Cc: Stanimir Varbanov <stanimir.varbanov@...aro.org>
> Fixes: 7c65817e6d38 ("drm/msm: gpu: Enable zap shader for A5XX")
> Acked-and-Tested-by: Jordan Crouse <jcrouse@...eaurora.org>
> Signed-off-by: Arnd Bergmann <arnd@...db.de>

Acked-by: Bjorn Andersson <bjorn.andersson@...aro.org>

However...

[..]
> @@ -37,6 +39,21 @@ static int zap_shader_load_mdt(struct device *dev, const char *fwname)
>  	if (!IS_ENABLED(CONFIG_ARCH_QCOM))
>  		return -EINVAL;
>  
> +	np = of_get_child_by_name(dev->of_node, "zap-shader");
> +	if (!np)
> +		return -ENODEV;
> +
> +	np = of_parse_phandle(np, "memory-region", 0);
> +	if (!np)
> +		return -EINVAL;
> +
> +	ret = of_address_to_resource(np, 0, &r);
> +	if (ret)
> +		return ret;
> +
> +	mem_phys = r.start;
> +	mem_size = resource_size(&r);
> +

...this means that it's now required that the reserved-memory region has
a "reg", the prior solution supported that we just specify a "size".

I have another driver where I need to figure out a mechanism of getting
hold of the dynamically allocated "base" of struct reserved_mem.


So I think it's appropriate to apply this fix now and loosen the
restrictions on placement later.

Regards,
Bjorn