* [PATCH] init/main.c: Fix potential static_command_line memory overflow
@ 2024-04-11 3:21 Yuntao Wang
2024-04-11 7:23 ` Geert Uytterhoeven
0 siblings, 1 reply; 14+ messages in thread
From: Yuntao Wang @ 2024-04-11 3:21 UTC (permalink / raw
To: linux-kernel
Cc: Thomas Gleixner, Andrew Morton, ndesaulniers@google.com,
Josh Poimboeuf, Peter Zijlstra (Intel), Tejun Heo,
Christophe Leroy, Krister Johansen, Arnd Bergmann,
Geert Uytterhoeven, Mike Rapoport, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 3:21 [PATCH] init/main.c: Fix potential static_command_line memory overflow Yuntao Wang
@ 2024-04-11 7:23 ` Geert Uytterhoeven
2024-04-11 13:21 ` Mike Rapoport
2024-04-11 14:29 ` [PATCH] init/main.c: " Masami Hiramatsu
0 siblings, 2 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2024-04-11 7:23 UTC (permalink / raw
To: Yuntao Wang
Cc: linux-kernel, Thomas Gleixner, Andrew Morton,
ndesaulniers@google.com, Josh Poimboeuf, Peter Zijlstra (Intel),
Tejun Heo, Christophe Leroy, Krister Johansen, Arnd Bergmann,
Mike Rapoport, Masami Hiramatsu
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 7:23 ` Geert Uytterhoeven
@ 2024-04-11 13:21 ` Mike Rapoport
2024-04-11 14:27 ` Yuntao Wang
2024-04-11 14:29 ` [PATCH] init/main.c: " Masami Hiramatsu
1 sibling, 1 reply; 14+ messages in thread
From: Mike Rapoport @ 2024-04-11 13:21 UTC (permalink / raw
To: Geert Uytterhoeven
Cc: Yuntao Wang, linux-kernel, Thomas Gleixner, Andrew Morton,
ndesaulniers@google.com, Josh Poimboeuf, Peter Zijlstra (Intel),
Tejun Heo, Christophe Leroy, Krister Johansen, Arnd Bergmann,
Masami Hiramatsu
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 13:21 ` Mike Rapoport
@ 2024-04-11 14:27 ` Yuntao Wang
2024-04-11 15:08 ` Masami Hiramatsu
0 siblings, 1 reply; 14+ messages in thread
From: Yuntao Wang @ 2024-04-11 14:27 UTC (permalink / raw
To: rppt
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
mhiramat, ndesaulniers, peterz, tglx, tj, ytcoode
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 7:23 ` Geert Uytterhoeven
2024-04-11 13:21 ` Mike Rapoport
@ 2024-04-11 14:29 ` Masami Hiramatsu
2024-04-11 14:59 ` Yuntao Wang
1 sibling, 1 reply; 14+ messages in thread
From: Masami Hiramatsu @ 2024-04-11 14:29 UTC (permalink / raw
To: Geert Uytterhoeven
Cc: Yuntao Wang, linux-kernel, Thomas Gleixner, Andrew Morton,
ndesaulniers@google.com, Josh Poimboeuf, Peter Zijlstra (Intel),
Tejun Heo, Christophe Leroy, Krister Johansen, Arnd Bergmann,
Mike Rapoport, 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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 14:29 ` [PATCH] init/main.c: " Masami Hiramatsu
@ 2024-04-11 14:59 ` Yuntao Wang
0 siblings, 0 replies; 14+ messages in thread
From: Yuntao Wang @ 2024-04-11 14:59 UTC (permalink / raw
To: mhiramat
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj, ytcoode
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 14:27 ` Yuntao Wang
@ 2024-04-11 15:08 ` Masami Hiramatsu
2024-04-11 15:47 ` Yuntao Wang
2024-04-12 3:51 ` Yuntao Wang
0 siblings, 2 replies; 14+ messages in thread
From: Masami Hiramatsu @ 2024-04-11 15:08 UTC (permalink / raw
To: Yuntao Wang
Cc: rppt, akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx,
linux-kernel, mhiramat, ndesaulniers, peterz, tglx, tj
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 15:08 ` Masami Hiramatsu
@ 2024-04-11 15:47 ` Yuntao Wang
2024-04-12 3:51 ` Yuntao Wang
1 sibling, 0 replies; 14+ messages in thread
From: Yuntao Wang @ 2024-04-11 15:47 UTC (permalink / raw
To: mhiramat
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj, ytcoode
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-11 15:08 ` Masami Hiramatsu
2024-04-11 15:47 ` Yuntao Wang
@ 2024-04-12 3:51 ` Yuntao Wang
2024-04-12 5:15 ` Masami Hiramatsu
1 sibling, 1 reply; 14+ messages in thread
From: Yuntao Wang @ 2024-04-12 3:51 UTC (permalink / raw
To: mhiramat
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj, ytcoode
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] init/main.c: Fix potential static_command_line memory overflow
2024-04-12 3:51 ` Yuntao Wang
@ 2024-04-12 5:15 ` Masami Hiramatsu
2024-04-12 8:17 ` [PATCH v2 0/2] " Yuntao Wang
0 siblings, 1 reply; 14+ messages in thread
From: Masami Hiramatsu @ 2024-04-12 5:15 UTC (permalink / raw
To: Yuntao Wang
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 0/2] Fix potential static_command_line memory overflow
2024-04-12 5:15 ` Masami Hiramatsu
@ 2024-04-12 8:17 ` Yuntao Wang
2024-04-12 8:17 ` [PATCH v2 1/2] init/main.c: " Yuntao Wang
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Yuntao Wang @ 2024-04-12 8:17 UTC (permalink / raw
To: mhiramat
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj, ytcoode
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] init/main.c: Fix potential static_command_line memory overflow
2024-04-12 8:17 ` [PATCH v2 0/2] " Yuntao Wang
@ 2024-04-12 8:17 ` Yuntao Wang
2024-04-12 8:17 ` [PATCH v2 2/2] init/main.c: Minor cleanup for the setup_command_line() function Yuntao Wang
2024-04-12 15:58 ` [PATCH v2 0/2] Fix potential static_command_line memory overflow Masami Hiramatsu
2 siblings, 0 replies; 14+ messages in thread
From: Yuntao Wang @ 2024-04-12 8:17 UTC (permalink / raw
To: mhiramat
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj, ytcoode
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] init/main.c: Minor cleanup for the setup_command_line() function
2024-04-12 8:17 ` [PATCH v2 0/2] " Yuntao Wang
2024-04-12 8:17 ` [PATCH v2 1/2] init/main.c: " Yuntao Wang
@ 2024-04-12 8:17 ` Yuntao Wang
2024-04-12 15:58 ` [PATCH v2 0/2] Fix potential static_command_line memory overflow Masami Hiramatsu
2 siblings, 0 replies; 14+ messages in thread
From: Yuntao Wang @ 2024-04-12 8:17 UTC (permalink / raw
To: mhiramat
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj, ytcoode
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/2] Fix potential static_command_line memory overflow
2024-04-12 8:17 ` [PATCH v2 0/2] " Yuntao Wang
2024-04-12 8:17 ` [PATCH v2 1/2] init/main.c: " Yuntao Wang
2024-04-12 8:17 ` [PATCH v2 2/2] init/main.c: Minor cleanup for the setup_command_line() function Yuntao Wang
@ 2024-04-12 15:58 ` Masami Hiramatsu
2 siblings, 0 replies; 14+ messages in thread
From: Masami Hiramatsu @ 2024-04-12 15:58 UTC (permalink / raw
To: Yuntao Wang
Cc: akpm, arnd, christophe.leroy, geert, jpoimboe, kjlx, linux-kernel,
ndesaulniers, peterz, rppt, tglx, tj
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-04-12 15:58 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-11 3:21 [PATCH] init/main.c: Fix potential static_command_line memory overflow Yuntao Wang
2024-04-11 7:23 ` Geert Uytterhoeven
2024-04-11 13:21 ` Mike Rapoport
2024-04-11 14:27 ` Yuntao Wang
2024-04-11 15:08 ` Masami Hiramatsu
2024-04-11 15:47 ` Yuntao Wang
2024-04-12 3:51 ` Yuntao Wang
2024-04-12 5:15 ` Masami Hiramatsu
2024-04-12 8:17 ` [PATCH v2 0/2] " Yuntao Wang
2024-04-12 8:17 ` [PATCH v2 1/2] init/main.c: " Yuntao Wang
2024-04-12 8:17 ` [PATCH v2 2/2] init/main.c: Minor cleanup for the setup_command_line() function Yuntao Wang
2024-04-12 15:58 ` [PATCH v2 0/2] Fix potential static_command_line memory overflow Masami Hiramatsu
2024-04-11 14:29 ` [PATCH] init/main.c: " Masami Hiramatsu
2024-04-11 14:59 ` Yuntao Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.