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

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

[Yuntao Wang]

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.

Fixes: f5c7310ac73e ("init/main: add checks for the return value of memblock_alloc*()")
Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 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;
 
 	static_command_line = memblock_alloc(len, SMP_CACHE_BYTES);
 	if (!static_command_line)
-- 
2.44.0

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

[Geert Uytterhoeven]

CC Hiramatsu-san

On Thu, Apr 11, 2024 at 5:25 AM Yuntao Wang <ytcoode@gmail.com> 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.
>
> Fixes: f5c7310ac73e ("init/main: add checks for the return value of memblock_alloc*()")
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> ---
>  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;
>
>         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@linux-m68k.org

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

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

[Mike Rapoport]

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@gmail.com> 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?

> > 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.

> > Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> > ---
> >  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;
> >
> >         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@linux-m68k.org
> 
> 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.

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

[Yuntao Wang]

On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <rppt@kernel.org> 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@gmail.com> 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.

> > > Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> > > ---
> > >  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;
> > >
> > >         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@linux-m68k.org
> > 
> > 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.

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

[Masami Hiramatsu]

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

> On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <rppt@kernel.org> 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@gmail.com> 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@gmail.com>
> > > > ---
> > > >  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@linux-m68k.org
> > > 
> > > 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@kernel.org>

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

[Yuntao Wang]

On Fri, 12 Apr 2024 00:08:58 +0900, Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Thu, 11 Apr 2024 22:27:23 +0800
> Yuntao Wang <ytcoode@gmail.com> wrote:
> 
> > On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <rppt@kernel.org> 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@gmail.com> 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@gmail.com>
> > > > > ---
> > > > >  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,

The other few lines of modification are some very small optimization.
I thought they were not worth posting a separate patch for at the time,
so I post them together. If necessary, I can separate them.

Thanks.

> > > > >
> > > > >         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@linux-m68k.org
> > > > 
> > > > 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@kernel.org>

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

[Yuntao Wang]

On Fri, 12 Apr 2024 00:08:58 +0900, Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Thu, 11 Apr 2024 22:27:23 +0800
> Yuntao Wang <ytcoode@gmail.com> wrote:
> 
> > On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <rppt@kernel.org> 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@gmail.com> 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@gmail.com>
> > > > > ---
> > > > >  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.

Hi Masami,

Do I need to split this patch into two? Or should I just repost this patch
with any other lines not related to this bug removed?

Actually, I think these lines are still necessary as they make the code
look a bit cleaner.

Thanks,
Yuntao

> 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@linux-m68k.org
> > > > 
> > > > 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@kernel.org>

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

[Masami Hiramatsu]

On Fri, 12 Apr 2024 11:51:07 +0800
Yuntao Wang <ytcoode@gmail.com> wrote:

> On Fri, 12 Apr 2024 00:08:58 +0900, Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Thu, 11 Apr 2024 22:27:23 +0800
> > Yuntao Wang <ytcoode@gmail.com> wrote:
> > 
> > > On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <rppt@kernel.org> 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@gmail.com> 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@gmail.com>
> > > > > > ---
> > > > > >  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.
> 
> Hi Masami,
> 
> Do I need to split this patch into two? Or should I just repost this patch
> with any other lines not related to this bug removed?

Latter one should be easier. Only add above one line and just explain that this
recovers strlen(command_line) which was miss-consolidated with strlen(boot_command_line)
in the commit f5c7310ac73e ("init/main: add checks for the return value of
memblock_alloc*()"). Simple fix does not confuse reviewers.

> 
> Actually, I think these lines are still necessary as they make the code
> look a bit cleaner.

That is a cleanup, and should be separated from bugfix, because bugfix must
be backported but the cleanup doesn't. As far as I can see, the cleanup
part can not apply to the commit f5c7310ac73e.

Thank you,

> 
> Thanks,
> Yuntao
> 
> > 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@linux-m68k.org
> > > > > 
> > > > > 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@kernel.org>


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

[PATCH v2 0/2] Fix potential static_command_line memory overflow

[Yuntao Wang]

v1 -> v2: Split the v1 version patch into a bugfix patch and a cleanup patch

Yuntao Wang (2):
  init/main.c: Fix potential static_command_line memory overflow
  init/main.c: Minor cleanup for the setup_command_line() function

 init/main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.44.0

[PATCH v2 1/2] init/main.c: Fix potential static_command_line memory overflow

[Yuntao Wang]

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.

This patch just recovers strlen(command_line) which was miss-consolidated
with strlen(boot_command_line) in the commit f5c7310ac73e ("init/main: add
checks for the return value of memblock_alloc*()")

Fixes: f5c7310ac73e ("init/main: add checks for the return value of memblock_alloc*()")
Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 init/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/init/main.c b/init/main.c
index 881f6230ee59..5dcf5274c09c 100644
--- a/init/main.c
+++ b/init/main.c
@@ -636,6 +636,8 @@ static void __init setup_command_line(char *command_line)
 	if (!saved_command_line)
 		panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen);
 
+	len = xlen + strlen(command_line) + 1;
+
 	static_command_line = memblock_alloc(len, SMP_CACHE_BYTES);
 	if (!static_command_line)
 		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
-- 
2.44.0

[PATCH v2 2/2] init/main.c: Minor cleanup for the setup_command_line() function

[Yuntao Wang]

This is just a minor cleanup to make the code look a bit cleaner.

Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 init/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/init/main.c b/init/main.c
index 5dcf5274c09c..d6383e397e9f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -630,11 +630,11 @@ 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;
 
-- 
2.44.0

Re: [PATCH v2 0/2] Fix potential static_command_line memory overflow

[Masami Hiramatsu]

On Fri, 12 Apr 2024 16:17:31 +0800
Yuntao Wang <ytcoode@gmail.com> wrote:

> v1 -> v2: Split the v1 version patch into a bugfix patch and a cleanup patch
> 
> Yuntao Wang (2):
>   init/main.c: Fix potential static_command_line memory overflow
>   init/main.c: Minor cleanup for the setup_command_line() function
> 
>  init/main.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

OK, both looks good to me. Let me pick it.

Thanks, 

> 
> -- 
> 2.44.0
> 


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

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

[Masami Hiramatsu]

On Thu, 11 Apr 2024 09:23:47 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> CC Hiramatsu-san
> 
> On Thu, Apr 11, 2024 at 5:25 AM Yuntao Wang <ytcoode@gmail.com> 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.

Hi Yuntao,

OK, but this is not a good way to fix.
We should introduce "slen = strlen(command_line) + xlen + 1" and use it for
allocating static_command_line.

> >
> > Fixes: f5c7310ac73e ("init/main: add checks for the return value of memblock_alloc*()")

As Mike pointed, this is not the best commit.

Fixes: 51887d03aca1 ("bootconfig: init: Allow admin to use bootconfig for kernel command line")

Thank you,

> > Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> > ---
> >  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;
> >
> >         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@linux-m68korg
> 
> 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
> 


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

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

[Yuntao Wang]

On Thu, 11 Apr 2024 23:29:01 +0900, Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Thu, 11 Apr 2024 09:23:47 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > CC Hiramatsu-san
> > 
> > On Thu, Apr 11, 2024 at 5:25 AM Yuntao Wang <ytcoode@gmail.com> 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.
> 
> Hi Yuntao,
> 
> OK, but this is not a good way to fix.
> We should introduce "slen = strlen(command_line) + xlen + 1" and use it for
> allocating static_command_line.

Hi Masami,

But this would introduce an additional variable 'slen', which seems unnecessary.

In fact, we can use 'len' directly, this makes the code more concise.

> > >
> > > Fixes: f5c7310ac73e ("init/main: add checks for the return value of memblock_alloc*()")
> 
> As Mike pointed, this is not the best commit.
> 
> Fixes: 51887d03aca1 ("bootconfig: init: Allow admin to use bootconfig for kernel command line")
> 
> Thank you,

I have reviewed the commit 51887d03aca1, but I don't think this commit introduced the issue.
I still think it was introduced by the f5c7310ac73e commit. Or perhaps I missed something?

> > > Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> > > ---
> > >  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;
> > >
> > >         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@linux-m68korg
> > 
> > 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
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>