On Wed, 09 Sep 2026 16:11:12 +0000
[email protected] wrote:
> 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()?
Oh, I never thought I could shoot myself in the foot like that.
Anyway, since bootconfig file size should be smaller than 32KB,
so we can check the file size before opening it.
>
> 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.
Good catch!
>
> Could we make this size check more robust against integer overflow?
OK, let me fix it too.
Thanks!
--
Masami Hiramatsu (Google) <[email protected]>
