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: <20230721162114.GA1321524-robh@kernel.org>
Date:   Fri, 21 Jul 2023 10:21:14 -0600
From:   Rob Herring <robh@...nel.org>
To:     Binglei Wang <l3b2w1@...il.com>
Cc:     frowand.list@...il.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, iommu@...ts.linux.dev
Subject: Re: [PATCH] cma: check for memory region overlapping

On Fri, Jul 21, 2023 at 12:07:29AM +0800, Binglei Wang wrote:
> From: Binglei Wang <l3b2w1@...il.com>
> 
> Cma memory region editted carelessly in dts may overlap
> with kernel code/data memory region which is reserved by memblock
> during the early phase of system memory initialization.

Is your goal for the kernel to function with this careless editing or 
warn enough to fix the DT?

What if other regions overlap with the kernel? Wouldn't we have the same 
problem? 

> 
> Without checking overlap and cma area setup done,
> this region will be released to buddy system later.
> 
> When memory usage under pressure, memory allocated from
> this region will collide with kernel code which is read-only.
> And the following writing to this region will trigger the panic
> of writing to read-only memory.
> 
> So when rmem_cma_setup returns EBUSY, do not phys-free this region
> to memblock or else we end up with free the kernel code memory
> to buddy system.
> 
> Signed-off-by: Binglei Wang <l3b2w1@...il.com>
> ---
>  drivers/of/of_reserved_mem.c | 3 ---
>  kernel/dma/contiguous.c      | 5 +++++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 7ec94cfcb..d62cc76ef 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -338,9 +338,6 @@ void __init fdt_init_reserved_mem(void)
>  					rmem->name);
>  				if (nomap)
>  					memblock_clear_nomap(rmem->base, rmem->size);
> -				else
> -					memblock_phys_free(rmem->base,
> -							   rmem->size);

I don't understand this change. Seems like perhaps someone would want 
the free here?

>  			} else {
>  				phys_addr_t end = rmem->base + rmem->size - 1;
>  				bool reusable =
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 6ea80ae42..a349f3e97 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -410,6 +410,11 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem)
>  		return -EBUSY;

Here we return EBUSY if we are using cmdline params. In that case, isn't 
freeing the DT CMA area the right thing to do?


>  	}
>  
> +	if (memblock_is_region_reserved(rmem->base, rmem->size)) {
> +		pr_info("Reserved memory: overlap with exsiting one\n");
> +		return -EBUSY;
> +	}
> +
>  	if (!of_get_flat_dt_prop(node, "reusable", NULL) ||
>  	    of_get_flat_dt_prop(node, "no-map", NULL))
>  		return -EINVAL;
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ