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 <[email protected]>
---
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
CC Hiramatsu-san
On Thu, Apr 11, 2024 at 5:25 AM Yuntao Wang <[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 -- [email protected]
>
> 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.
On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 -- [email protected]
> >
> > 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.
On Thu, 11 Apr 2024 09:23:47 +0200
Geert Uytterhoeven <[email protected]> wrote:
> CC Hiramatsu-san
>
> On Thu, Apr 11, 2024 at 5:25 AM Yuntao Wang <[email protected]> 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 <[email protected]>
> > ---
> > 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) <[email protected]>
On Thu, 11 Apr 2024 23:29:01 +0900, Masami Hiramatsu (Google) <[email protected]> wrote:
> On Thu, 11 Apr 2024 09:23:47 +0200
> Geert Uytterhoeven <[email protected]> wrote:
>
> > CC Hiramatsu-san
> >
> > On Thu, Apr 11, 2024 at 5:25 AM Yuntao Wang <[email protected]> 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 <[email protected]>
> > > ---
> > > 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) <[email protected]>
On Thu, 11 Apr 2024 22:27:23 +0800
Yuntao Wang <[email protected]> wrote:
> On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 -- [email protected]
> > >
> > > 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) <[email protected]>
On Fri, 12 Apr 2024 00:08:58 +0900, Masami Hiramatsu (Google) <[email protected]> wrote:
> On Thu, 11 Apr 2024 22:27:23 +0800
> Yuntao Wang <[email protected]> wrote:
>
> > On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 -- [email protected]
> > > >
> > > > 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) <[email protected]>
On Fri, 12 Apr 2024 00:08:58 +0900, Masami Hiramatsu (Google) <[email protected]> wrote:
> On Thu, 11 Apr 2024 22:27:23 +0800
> Yuntao Wang <[email protected]> wrote:
>
> > On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 -- [email protected]
> > > >
> > > > 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) <[email protected]>
On Fri, 12 Apr 2024 11:51:07 +0800
Yuntao Wang <[email protected]> wrote:
> On Fri, 12 Apr 2024 00:08:58 +0900, Masami Hiramatsu (Google) <[email protected]> wrote:
>
> > On Thu, 11 Apr 2024 22:27:23 +0800
> > Yuntao Wang <[email protected]> wrote:
> >
> > > On Thu, 11 Apr 2024 16:21:37 +0300, Mike Rapoport <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > > > 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 -- [email protected]
> > > > >
> > > > > 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) <[email protected]>
--
Masami Hiramatsu (Google) <[email protected]>
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
On Fri, 12 Apr 2024 16:17:31 +0800
Yuntao Wang <[email protected]> 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) <[email protected]>