Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow

[Google]

On Thu, 11 Apr 2024 22:27:23 +0800
Yuntao Wang <ytcoode@xxxxxxxxx> wrote:

> On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> > On Thu, Apr 11, 2024 at 09:23:47AM +0200, Geert Uytterhoeven wrote:
> > > CC Hiramatsu-san
> > > 
> > > On Thu, Apr 11, 2024 at 5:25 AM Yuntao Wang <ytcoode@xxxxxxxxx> wrote:
> > > > We allocate memory of size 'xlen + strlen(boot_command_line) + 1' for
> > > > static_command_line, but the strings copied into static_command_line are
> > > > extra_command_line and command_line, rather than extra_command_line and
> > > > boot_command_line.
> > > >
> > > > When strlen(command_line) > strlen(boot_command_line), static_command_line
> > > > will overflow.
> > 
> > Can this ever happen? 
> > Did you observe the overflow or is this a theoretical bug?
> 
> I didn't observe the overflow, it's just a theoretical bug.
> 
> > > > Fixes: f5c7310ac73e ("init/main: add checks for the return value of memblock_alloc*()")
> > 
> > f5c7310ac73e didn't have the logic for calculating allocation size, we
> > surely don't want to go back that far wiht Fixes.
> 
> Before commit f5c7310ac73e, the memory size allocated for static_command_line
> was 'strlen(command_line) + 1', but commit f5c7310ac73e changed this size
> to 'strlen(boot_command_line) + 1'. I think this should be wrong.

Ah, OK. that sounds reasonable. 

> 
> > > > Signed-off-by: Yuntao Wang <ytcoode@xxxxxxxxx>
> > > > ---
> > > >  init/main.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/init/main.c b/init/main.c
> > > > index 2ca52474d0c3..a7b1f5f3e3b6 100644
> > > > --- a/init/main.c
> > > > +++ b/init/main.c
> > > > @@ -625,11 +625,13 @@ static void __init setup_command_line(char *command_line)
> > > >         if (extra_init_args)
> > > >                 ilen = strlen(extra_init_args) + 4; /* for " -- " */
> > > >
> > > > -       len = xlen + strlen(boot_command_line) + 1;
> > > > +       len = xlen + strlen(boot_command_line) + ilen + 1;
> > > >
> > > > -       saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES);
> > > > +       saved_command_line = memblock_alloc(len, SMP_CACHE_BYTES);
> > > >         if (!saved_command_line)
> > > > -               panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen);
> > > > +               panic("%s: Failed to allocate %zu bytes\n", __func__, len);
> > > > +
> > > > +       len = xlen + strlen(command_line) + 1;

Ah, I missed this line. Sorry. So this looks good to me but you don't need any
other lines, because those are not related to the bug you want to fix.
Please just focus on 1 fix.

Thank you,

> > > >
> > > >         static_command_line = memblock_alloc(len, SMP_CACHE_BYTES);
> > > >         if (!static_command_line)
> > > 
> > > Gr{oetje,eeting}s,
> > > 
> > >                         Geert
> > > 
> > > -- 
> > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> > > 
> > > In personal conversations with technical people, I call myself a hacker. But
> > > when I'm talking to journalists I just say "programmer" or something like that.
> > >                                 -- Linus Torvalds
> > 
> > -- 
> > Sincerely yours,
> > Mike.


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>