Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Integer overflow in initrd bootconfig size calculation leading to a heap buffer overflow.
--
commit 51beb61f7aa059b732b7d7560c90f8155150ccd7
Author: Masami Hiramatsu (Google) [off-list ref]
tools/bootconfig: Consolidate xbc_init() to error message wrapper
Use init_xbc_with_error() for all bootconfig initialization in the
bootconfig tool instead of showing errors in different way.
This simplifies the code logic and make it easy to maintain.
quoted hunk ↗ jump to hunk
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 6035404733c38..7117aa9b2a839 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -238,52 +278,9 @@ static int load_xbc_from_initrd(int fd, char **buf)
return -EINVAL;
}
- ret = xbc_init(*buf, size, &msg, NULL);
- /* Wrong data */
- if (ret < 0) {
- pr_err("parse error: %s.\n", msg);
- return ret;
- }
+ ret = init_xbc_with_error(*buf, size);
[Severity: High]
This is a pre-existing issue, but is it possible for a 32-bit integer overflow
to bypass the size validation check in load_xbc_from_initrd()?
In load_xbc_from_initrd(), size is read from the file as a uint32_t:
tools/bootconfig/main.c:load_xbc_from_initrd() {
...
if (read(fd, &size, sizeof(uint32_t)) < 0)
return pr_errno("Failed to read size", -errno);
size = le32toh(size);
...
if (stat.st_size < size + BOOTCONFIG_FOOTER_SIZE) {
pr_err("bootconfig size is too big\n");
return -E2BIG;
}
...
}
On 32-bit systems, if an attacker crafts size such that adding
BOOTCONFIG_FOOTER_SIZE (20) wraps around (for instance, if size is 0xFFFFFFFF),
the stat.st_size < size + BOOTCONFIG_FOOTER_SIZE check can be bypassed.
This would lead to load_xbc_fd() being called with size evaluating to -1:
tools/bootconfig/main.c:load_xbc_fd() {
...
*buf = malloc(size + 1);
if (!*buf)
return -ENOMEM;
ret = read(fd, *buf, size);
if (ret < 0)
return -errno;
(*buf)[size] = '\0';
...
}
The malloc(size + 1) would result in a 0-byte allocation, while the
read(fd, *buf, size) would cast the negative size to size_t (0xFFFFFFFF) and
overflow the chunk. Finally, (*buf)[size] = '\0' would write to a negative
index (-1), which corrupts heap metadata.
Could we make this size check more robust against integer overflow?
- return size;
-}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/178896921555.177508.434402948295885560.stgit@devnote2?part=2