Re: [PATCH 2/3] tools/bootconfig: Consolidate xbc_init() to error message wrapper

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 09 Sep 2026 16:11:12 +0000
sashiko-bot@kernel.org 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) <mhiramat@kernel.org>
> 
> 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) <mhiramat@kernel.org>