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) <[email protected]> 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. > 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 [ ... ] > @@ -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
