From bb2b54f19e202d5781ec6c05b3d584fcd85cddcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre-Cl=C3=A9ment=20Tosi?= Date: Fri, 6 Oct 2023 14:39:12 +0100 Subject: [PATCH 1/3] ANDROID: Revert "Fix integer wrap sanitisation." This reverts commit 0e783e26f75c08e421467ca4a6c21ff2589cd2fa. Revert the patch we've had in Android now that upstream has [1] commit 73590342fc85 ("libfdt: prevent integer overflow in fdt_next_tag") which addresses the same bug. As that patch is less rigorous w.r.t. the final value of 'offset' than the one, the last 'if' is upstreamed by [2], which will be cherry-picked here. [1]: https://android.googlesource.com/platform/external/dtc/+/73590342fc85ca207ca1e6cbc110179873a96962 [2]: https://lore.kernel.org/devicetree-compiler/20231011172427.g4tlsew3wsjtddil@google.com/ Test: N/A Change-Id: I662a599713b4090abd090322bca0a78e58f4c92c --- libfdt/fdt.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/libfdt/fdt.c b/libfdt/fdt.c index c17cad5..9fe7cf4 100644 --- a/libfdt/fdt.c +++ b/libfdt/fdt.c @@ -188,20 +188,12 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset) break; case FDT_PROP: - lenp = fdt_offset_ptr(fdt, offset, sizeof(struct fdt_property) - FDT_TAGSIZE); + lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp)); if (!can_assume(VALID_DTB) && !lenp) return FDT_END; /* premature end */ - - /* skip name offset, length */ - offset += sizeof(struct fdt_property) - FDT_TAGSIZE; - - if (!can_assume(VALID_DTB) - && !fdt_offset_ptr(fdt, offset, fdt32_to_cpu(*lenp))) - return FDT_END; /* premature end */ - - /* skip value */ - offset += fdt32_to_cpu(*lenp); - + /* skip-name offset, length and value */ + offset += sizeof(struct fdt_property) - FDT_TAGSIZE + + fdt32_to_cpu(*lenp); if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 && ((offset - fdt32_to_cpu(*lenp)) % 8) != 0) @@ -217,8 +209,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset) return FDT_END; } - if (!can_assume(VALID_DTB) && (offset <= startoffset - || !fdt_offset_ptr(fdt, startoffset, offset - startoffset))) + if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset)) return FDT_END; /* premature end */ *nextoffset = FDT_TAGALIGN(offset); From cbfd232da37f480bf3abcba01e16e44306149d45 Mon Sep 17 00:00:00 2001 From: Tadeusz Struk Date: Wed, 5 Oct 2022 16:29:30 -0700 Subject: [PATCH 2/3] FROMGIT: libfdt: prevent integer overflow in fdt_next_tag 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. Signed-off-by: Tadeusz Struk Message-Id: <20221005232931.3016047-1-tadeusz.struk@linaro.org> Signed-off-by: David Gibson (cherry-picked from commit 73590342fc85ca207ca1e6cbc110179873a96962 git://git.kernel.org/pub/scm/utils/dtc/dtc.git main) Test: N/A Change-Id: I894051ed101255800717001a71a5a74ac66fd897 --- libfdt/fdt.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/libfdt/fdt.c b/libfdt/fdt.c index 9fe7cf4..13b4b9b 100644 --- a/libfdt/fdt.c +++ b/libfdt/fdt.c @@ -165,7 +165,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, sum; int offset = startoffset; const char *p; @@ -191,12 +191,19 @@ 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); + sum = len + offset; + if (!can_assume(VALID_DTB) && + (INT_MAX <= sum || sum < (uint32_t) offset)) + 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(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; From 0493daab327ef269a7ab6a8ab61fae15f92e7666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre-Cl=C3=A9ment=20Tosi?= Date: Wed, 11 Oct 2023 18:24:27 +0100 Subject: [PATCH 3/3] FROMLIST: libdft: fdt_next_tag: Harden offset overflow check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (am from https://lore.kernel.org/devicetree-compiler/20231011172427.g4tlsew3wsjtddil@google.com/) Test: N/A Change-Id: I0b17b0827ccc0ece0a2d1795b388408fb599aed7 --- libfdt/fdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libfdt/fdt.c b/libfdt/fdt.c index 13b4b9b..b8ffb33 100644 --- a/libfdt/fdt.c +++ b/libfdt/fdt.c @@ -216,7 +216,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);