From: Binarly Vulnerability Research <[email protected]>
fdt_get_name() in scripts/dtc/libfdt/fdt_ro.c can return a null
pointer. This happens when the fdt_ro_probe_() or
fdt_check_node_offset_() check fails, as well as when the
'!can_assume(LATEST) && fdt_version(fdt) < 0x10' condition is true
and the FDT node name doesn't contain the '/' character:
const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
{
const struct fdt_node_header *nh = fdt_offset_ptr_(fdt,
nodeoffset);
const char *nameptr;
int err;
if (((err = fdt_ro_probe_(fdt)) < 0)
|| ((err = fdt_check_node_offset_(fdt, nodeoffset)) <
0))
goto fail;
nameptr = nh->name;
if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
/*
* For old FDT versions, match the naming conventions of
V16:
* give only the leaf name (after all /). The actual tree
* contents are loosely checked.
*/
const char *leaf;
leaf = strrchr(nameptr, '/');
if (leaf == NULL) {
err = -FDT_ERR_BADSTRUCTURE;
goto fail;
}
nameptr = leaf+1;
}
if (len)
*len = strlen(nameptr);
return nameptr;
fail:
if (len)
*len = err;
return NULL;
}
The fdt_get_name() function is used in fdt_find_regions() in
boot/fdt_region.c:
int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,
char * const exc_prop[], int exc_prop_count,
struct fdt_region region[], int max_regions,
char *path, int path_len, int add_string_tab)
{
int stack[FDT_MAX_DEPTH] = { 0 };
char *end;
int nextoffset = 0;
uint32_t tag;
int count = 0;
int start = -1;
int depth = -1;
int want = 0;
int base = fdt_off_dt_struct(fdt);
bool expect_end = false;
end = path;
*end = '\0';
do {
const struct fdt_property *prop;
const char *name;
const char *str;
int include = 0;
int stop_at = 0;
int offset;
int len;
offset = nextoffset;
tag = fdt_next_tag(fdt, offset, &nextoffset);
stop_at = nextoffset;
/* If we see two root nodes, something is wrong */
if (expect_end && tag != FDT_END)
return -FDT_ERR_BADLAYOUT;
switch (tag) {
case FDT_PROP:
include = want >= 2;
stop_at = offset;
prop = fdt_get_property_by_offset(fdt, offset,
NULL);
str = fdt_string(fdt,
fdt32_to_cpu(prop->nameoff));
if (!str)
return -FDT_ERR_BADSTRUCTURE;
if (str_in_list(str, exc_prop, exc_prop_count))
include = 0;
break;
case FDT_NOP:
include = want >= 2;
stop_at = offset;
break;
case FDT_BEGIN_NODE:
depth++;
if (depth == FDT_MAX_DEPTH)
return -FDT_ERR_BADSTRUCTURE;
name = fdt_get_name(fdt, offset, &len);
/* The root node must have an empty name */
if (!depth && *name)
return -FDT_ERR_BADLAYOUT;
if (end - path + 2 + len >= path_len)
return -FDT_ERR_NOSPACE;
if (end != path + 1)
*end++ = '/';
strcpy(end, name);
end += len;
stack[depth] = want;
if (want == 1)
stop_at = offset;
if (str_in_list(path, inc, inc_count))
want = 2;
else if (want)
want--;
else
stop_at = offset;
include = want;
break;
...
There is no check that the value returned by fdt_get_name() is not
null, which leads to a null pointer dereference (at *name and
strcpy(end, name)) and a potential stack overflow of the 'end'
buffer. However, if the strcpy call succeeds, the subsequent
'end += len' decreases the 'end' pointer, since 'len' is set to the
negative error code returned by fdt_get_name(). As a result, the next
iteration of the loop writes data to a stack location before the
start of the 'path' buffer, which can be used to overwrite the
return address of fdt_find_regions() and achieve code execution in
the context of the bootloader. This attack is possible when the page
at address 0x0 is mapped, which is common for embedded devices, so
the strcpy() call doesn't fail.
The same lack of validation exists in fdt_next_region() in
boot/fdt_region.c, fdt_check_full() in scripts/dtc/libfdt/fdt_ro.c,
and display_fdt_by_regions() in tools/fdtgrep.c.
Fix: Validate both the returned pointer and the length before use.
Signed-off-by: Binarly Vulnerability Research <[email protected]>
---
boot/fdt_region.c | 4 ++++
scripts/dtc/libfdt/fdt_ro.c | 2 ++
tools/fdtgrep.c | 2 ++
3 files changed, 8 insertions(+)
diff --git a/boot/fdt_region.c b/boot/fdt_region.c
index 295ea08ac91..95706b872f4 100644
--- a/boot/fdt_region.c
+++ b/boot/fdt_region.c
@@ -86,6 +86,8 @@ int fdt_find_regions(const void *fdt, char * const inc[], int
inc_count,
if (depth == FDT_MAX_DEPTH)
return -FDT_ERR_BADSTRUCTURE;
name = fdt_get_name(fdt, offset, &len);
+ if (!name || len < 0)
+ return -FDT_ERR_BADSTRUCTURE;
/* The root node must have an empty name */
if (!depth && *name)
@@ -553,6 +555,8 @@ int fdt_next_region(const void *fdt,
if (p.depth == FDT_MAX_DEPTH)
return -FDT_ERR_BADSTRUCTURE;
name = fdt_get_name(fdt, offset, &len);
+ if (!name || len < 0)
+ return -FDT_ERR_BADSTRUCTURE;
if (p.end - path + 2 + len >= path_len)
return -FDT_ERR_NOSPACE;
diff --git a/scripts/dtc/libfdt/fdt_ro.c b/scripts/dtc/libfdt/fdt_ro.c
index 3e7e26b4398..82f90eb46f6 100644
--- a/scripts/dtc/libfdt/fdt_ro.c
+++ b/scripts/dtc/libfdt/fdt_ro.c
@@ -940,6 +940,8 @@ int fdt_check_full(const void *fdt, size_t bufsize)
int len;
name = fdt_get_name(fdt, offset, &len);
+ if (!name || len < 0)
+ return -FDT_ERR_BADSTRUCTURE;
if (*name || len)
return -FDT_ERR_BADLAYOUT;
}
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
index b4c041070f5..ab8ee939e97 100644
--- a/tools/fdtgrep.c
+++ b/tools/fdtgrep.c
@@ -355,6 +355,8 @@ static int display_fdt_by_regions(struct display_info
*disp, const void *blob,
case FDT_BEGIN_NODE:
name = fdt_get_name(blob, offset, &len);
+ if (!name || len < 0)
+ return -FDT_ERR_BADSTRUCTURE;
fprintf(f, "%*s%s {", depth++ * shift, "",
*name ? name : "/");
break;
--
2.53.0
