Hi David,
On 10/4/22 00:06, David Gibson wrote:
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.
Thanks for feedback. I will change the logic to only work on unsigned
integers. I will also update the unit tests according to your suggestions.
--
Thanks,
Tadeusz
