Re: [PATCH 1/4] libfdt: Check for invalid memreserve block

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

On Fri, Jun 11, 2021 at 12:58:20PM +0100, Andre Przywara wrote:
> fdt_num_mem_rsv() can return a negative error value, when the memreserve
> block is not properly terminated or is exceeding its advertised size,
> This can happen on malformed DTBs, and FDT_RO_PROBE() does not cover this.
> 
> Check the return value of fdt_num_mem_rsv() first for an error
> condition, and also check if the total size of the memreserve block
> would provoke a signed integer overflow.
> 
> This fixes issues with malformed DT blobs, where we would end up with
> negative offsets into buffers.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  libfdt/fdt_rw.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 3621d36..a0f03d0 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -427,8 +427,13 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  
>  	FDT_RO_PROBE(fdt);
>  
> -	mem_rsv_size = (fdt_num_mem_rsv(fdt)+1)
> -		* sizeof(struct fdt_reserve_entry);
> +	mem_rsv_size = fdt_num_mem_rsv(fdt);
> +	if (mem_rsv_size < 0)

You want a can_assume(VALID_DTB) on this test as well.  The people
doing fdt on miniscule first stage roms really hate every extra
instruction.

> +		return mem_rsv_size;
> +
> +	mem_rsv_size = (mem_rsv_size + 1) * sizeof(struct fdt_reserve_entry);
> +	if (!can_assume(VALID_DTB) && mem_rsv_size < 0)
> +		return -FDT_ERR_NOSPACE;

This second test is not necessary.  fdt_num_mem_rsv() implicitly
checks for a minimally sane mem reserve block, which implies its size
lies within totalsize, which in turn is <= INT_MAX.

If it were necessary, I don't think it's quite safe, because signed
overflow is UB, IIUC.

>  	if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
>  		struct_size = fdt_size_dt_struct(fdt);
> @@ -490,8 +495,14 @@ int fdt_pack(void *fdt)
>  
>  	FDT_RW_PROBE(fdt);
>  
> -	mem_rsv_size = (fdt_num_mem_rsv(fdt)+1)
> -		* sizeof(struct fdt_reserve_entry);
> +	mem_rsv_size = fdt_num_mem_rsv(fdt);
> +	if (mem_rsv_size < 0)
> +		return mem_rsv_size;
> +
> +	mem_rsv_size = (mem_rsv_size + 1) * sizeof(struct fdt_reserve_entry);
> +	if (!can_assume(VALID_DTB) && mem_rsv_size < 0)
> +		return -FDT_ERR_NOSPACE;

Same comments here.

>  	fdt_packblocks_(fdt, fdt, mem_rsv_size, fdt_size_dt_struct(fdt),
>  			fdt_size_dt_strings(fdt));
>  	fdt_set_totalsize(fdt, fdt_data_size_(fdt));

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]