[PATCH] libdft: fdt_next_tag: Harden offset overflow check

[PATCH] libdft: fdt_next_tag: Harden offset overflow check

[Pierre-Clément Tosi]

As 'offset' is obtained through various paths within the function by
adding user-provided values to 'startoffset' and as we validate its
final value by substracting it from the initial one, there is a risk
that one of the paths might have lead to an overflow, making the check
validate a "negative" (wrong) length, potentially causing fdt_next_tag()
to report an invalid offset as valid through 'nextoffset'.

For example, when parsing an FDT_PROP, we currently validate that

    offset = startoffset + len + FDT_TAGSIZE

doesn't overflow but then assign

    offset = startoffset + len + sizeof(struct fdt_property)

so harden all paths by validating the offset in the very last check.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 libfdt/fdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 20c6415..dda342d 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -213,7 +213,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		return FDT_END;
 	}
 
-	if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
+	if (!can_assume(VALID_DTB) && (offset <= startoffset
+	    || !fdt_offset_ptr(fdt, startoffset, offset - startoffset)))
 		return FDT_END; /* premature end */
 
 	*nextoffset = FDT_TAGALIGN(offset);
-- 
2.42.0.609.gbb76f46606-goog


-- 
Pierre

Re: [PATCH] libdft: fdt_next_tag: Harden offset overflow check

[David Gibson]

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

On Wed, Oct 11, 2023 at 06:24:27PM +0100, Pierre-Clément Tosi wrote:
> As 'offset' is obtained through various paths within the function by
> adding user-provided values to 'startoffset' and as we validate its
> final value by substracting it from the initial one, there is a risk
> that one of the paths might have lead to an overflow, making the check
> validate a "negative" (wrong) length, potentially causing fdt_next_tag()
> to report an invalid offset as valid through 'nextoffset'.
> 
> For example, when parsing an FDT_PROP, we currently validate that
> 
>     offset = startoffset + len + FDT_TAGSIZE
> 
> doesn't overflow but then assign
> 
>     offset = startoffset + len + sizeof(struct fdt_property)
> 
> so harden all paths by validating the offset in the very last check.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>

The intended purpose of fdt_offset_ptr() is that you can give it
otherwise unvetted values, and it will only return non-NULL if those
correspond to a sane range of bytes within the dtb.  If the test
you're adding here is necessary, then fdt_offset_ptr() isn't
accomplishing that job.

So, it seems like it's fdt_offset_ptr() that should be hardened
instead, which may fix other places as well as this one.

> ---
>  libfdt/fdt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 20c6415..dda342d 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -213,7 +213,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		return FDT_END;
>  	}
>  
> -	if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> +	if (!can_assume(VALID_DTB) && (offset <= startoffset
> +	    || !fdt_offset_ptr(fdt, startoffset, offset - startoffset)))
>  		return FDT_END; /* premature end */
>  
>  	*nextoffset = FDT_TAGALIGN(offset);

-- 
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 --]