On Fri, Sep 30, 2022 at 08:20:03AM -0700, Tadeusz Struk wrote:
> Since fdt_next_tag() in a public API function all input parameters,
> including the fdt blob should not be trusted. It is possible to forge
> a blob with invalid property length that will cause integer overflow
> during offset calculation. To prevent that, validate the property length
> read from the blob before doing calculations.
So.. yes, there can be an integer overflow here. I think the actual
damage it can cause is strongly mitigated by the fact that we should
only ever use the result of that overflow via fdt_offset_ptr(), which
will safely reject a bad offset.
> Signed-off-by: Tadeusz Struk <tadeusz.struk@xxxxxxxxxx>
> v2:
> * Use len local variable to avoid multiple calls to fdt32_to_cpu(*lenp)
> * Add can_assume(VALID_DTB) to the new checks
> libfdt/fdt.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 90a39e8..b7c202a 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -162,7 +162,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> {
> const fdt32_t *tagp, *lenp;
> - uint32_t tag;
> + uint32_t tag, len;
> int offset = startoffset;
> const char *p;
>
> @@ -188,12 +188,20 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> if (!can_assume(VALID_DTB) && !lenp)
> return FDT_END; /* premature end */
> +
> + len = fdt32_to_cpu(*lenp);
> + if (!can_assume(VALID_DTB) && INT_MAX <= len)
This check is redundant with the one below, isn't it?
> + return FDT_END; /* premature end */
> +
> /* skip-name offset, length and value */
> - offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> - + fdt32_to_cpu(*lenp);
> + offset += sizeof(struct fdt_property) - FDT_TAGSIZE + len;
> +
> + if (!can_assume(VALID_DTB) && offset < 0)
> + return FDT_END; /* premature end */
Hmmm.. so, signed integer overflow is actually undefined behaviour in
C, so checking for offset < 0 after the addition isn't actually a safe
way to check for overflow. To safely check for overflow, I believe
you need to check that the *unsigned* sum of offset and len is greater
than or equal to offset (*unsigned* integer overflow is defined to
wrap as you'd expect for 2s complement arithmetic). Actually given
the constraints we've put on offsets in general, we need to check that
that unsigned sum is both greater than offset and less than INT_MAX.
> if (!can_assume(LATEST) &&
> - fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
> - ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
> + fdt_version(fdt) < 0x10 && len >= 8 &&
> + ((offset - len) % 8) != 0)
> offset += 4;
> break;
--
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:
signature.asc
Description: PGP signature
