linux-kernel - Re: [PATCH] dma-mapping: Save base/size instead of pointer to shared DMA pool

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: <6a8ba993-9413-40ad-8c91-0cf268798585@quicinc.com>
Date: Wed, 13 Nov 2024 10:02:49 -0800
From: Oreoluwa Babatunde <quic_obabatun@...cinc.com>
To: Geert Uytterhoeven <geert+renesas@...der.be>,
        Rob Herring
	<robh@...nel.org>, Christoph Hellwig <hch@....de>,
        Marek Szyprowski
	<m.szyprowski@...sung.com>,
        Robin Murphy <robin.murphy@....com>
CC: <iommu@...ts.linux.dev>, <linux-renesas-soc@...ts.infradead.org>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] dma-mapping: Save base/size instead of pointer to shared
 DMA pool


On 11/12/2024 10:39 AM, Geert Uytterhoeven wrote:
> On RZ/Five, which is non-coherent, and uses CONFIG_DMA_GLOBAL_POOL=y:
>
>     Oops - store (or AMO) access fault [#1]
>     CPU: 0 UID: 0 PID: 1 Comm: swapper Not tainted 6.12.0-rc1-00015-g8a6e02d0c00e #201
>     Hardware name: Renesas SMARC EVK based on r9a07g043f01 (DT)
>     epc : __memset+0x60/0x100
>      ra : __dma_alloc_from_coherent+0x150/0x17a
>     epc : ffffffff8062d2bc ra : ffffffff80053a94 sp : ffffffc60000ba20
>      gp : ffffffff812e9938 tp : ffffffd601920000 t0 : ffffffc6000d0000
>      t1 : 0000000000000000 t2 : ffffffffe9600000 s0 : ffffffc60000baa0
>      s1 : ffffffc6000d0000 a0 : ffffffc6000d0000 a1 : 0000000000000000
>      a2 : 0000000000001000 a3 : ffffffc6000d1000 a4 : 0000000000000000
>      a5 : 0000000000000000 a6 : ffffffd601adacc0 a7 : ffffffd601a841a8
>      s2 : ffffffd6018573c0 s3 : 0000000000001000 s4 : ffffffd6019541e0
>      s5 : 0000000200000022 s6 : ffffffd6018f8410 s7 : ffffffd6018573e8
>      s8 : 0000000000000001 s9 : 0000000000000001 s10: 0000000000000010
>      s11: 0000000000000000 t3 : 0000000000000000 t4 : ffffffffdefe62d1
>      t5 : 000000001cd6a3a9 t6 : ffffffd601b2aad6
>     status: 0000000200000120 badaddr: ffffffc6000d0000 cause: 0000000000000007
>     [<ffffffff8062d2bc>] __memset+0x60/0x100
>     [<ffffffff80053e1a>] dma_alloc_from_global_coherent+0x1c/0x28
>     [<ffffffff80053056>] dma_direct_alloc+0x98/0x112
>     [<ffffffff8005238c>] dma_alloc_attrs+0x78/0x86
>     [<ffffffff8035fdb4>] rz_dmac_probe+0x3f6/0x50a
>     [<ffffffff803a0694>] platform_probe+0x4c/0x8a
>
> If CONFIG_DMA_GLOBAL_POOL=y, the reserved_mem structure passed to
> rmem_dma_setup() is saved for later use, by saving the passed pointer.
> However, when dma_init_reserved_memory() is called later, the pointer
> has become stale, causing a crash.
>
> E.g. in the RZ/Five case, the referenced memory now contains the
> reserved_mem structure for the "mmode_resv0@...00" node (with base
> 0x30000 and size 0x10000), instead of the correct "pma_resv0@...00000"
> node (with base 0x58000000 and size 0x8000000).
>
> Fix this by saving the needed reserved_mem structure's contents instead.
>
> Fixes: 8a6e02d0c00e7b62 ("of: reserved_mem: Restructure how the reserved memory regions are processed")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
Reviewed-by: Oreoluwa Babatunde <quic_obabatun@...cinc.com>
> ---
>  kernel/dma/coherent.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
> index ff5683a57f77126d..3b2bdca9f1d4b027 100644
> --- a/kernel/dma/coherent.c
> +++ b/kernel/dma/coherent.c
> @@ -330,7 +330,8 @@ int dma_init_global_coherent(phys_addr_t phys_addr, size_t size)
>  #include <linux/of_reserved_mem.h>
>  
>  #ifdef CONFIG_DMA_GLOBAL_POOL
> -static struct reserved_mem *dma_reserved_default_memory __initdata;
> +static phys_addr_t dma_reserved_default_memory_base __initdata;
> +static phys_addr_t dma_reserved_default_memory_size __initdata;
>  #endif
>  
>  static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev)
> @@ -376,9 +377,10 @@ static int __init rmem_dma_setup(struct reserved_mem *rmem)
>  
>  #ifdef CONFIG_DMA_GLOBAL_POOL
>  	if (of_get_flat_dt_prop(node, "linux,dma-default", NULL)) {
> -		WARN(dma_reserved_default_memory,
> +		WARN(dma_reserved_default_memory_size,
>  		     "Reserved memory: region for default DMA coherent area is redefined\n");
> -		dma_reserved_default_memory = rmem;
> +		dma_reserved_default_memory_base = rmem->base;
> +		dma_reserved_default_memory_size = rmem->size;
>  	}
>  #endif
>  
> @@ -391,10 +393,10 @@ static int __init rmem_dma_setup(struct reserved_mem *rmem)
>  #ifdef CONFIG_DMA_GLOBAL_POOL
>  static int __init dma_init_reserved_memory(void)
>  {
> -	if (!dma_reserved_default_memory)
> +	if (!dma_reserved_default_memory_size)
>  		return -ENOMEM;
> -	return dma_init_global_coherent(dma_reserved_default_memory->base,
> -					dma_reserved_default_memory->size);
> +	return dma_init_global_coherent(dma_reserved_default_memory_base,
> +					dma_reserved_default_memory_size);
>  }
>  core_initcall(dma_init_reserved_memory);
>  #endif /* CONFIG_DMA_GLOBAL_POOL */
Hi Geert,

Thanks for providing a fix!

I also looked around more to see if there are more places that
could store a stale reference to rmem but I didn't see any others,
so I think we are good!

I have given my "reviewed-by" tag as well.

Thank you!
Oreoluwa