[GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[Steven Rostedt]

Linus,

Fix alignment of bootconfig

GRUB may align the init ramdisk size to 4 bytes, the magic number at the
end of the init ramdisk that denotes bootconfig is attached may not be at
the exact end of the ramdisk. The kernel needs to check back at least 4
bytes.


Please pull the latest trace-v5.10-rc3 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v5.10-rc3

Tag SHA1: e20c1d4f9314c2296b72a02f3e21c6116099f573
Head SHA1: 50b8a742850fce7293bed45753152c425f7e931b


Masami Hiramatsu (1):
      bootconfig: Extend the magic check range to the preceding 3 bytes

----
 init/main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
---------------------------
commit 50b8a742850fce7293bed45753152c425f7e931b
Author: Masami Hiramatsu <mhiramat@kernel.org>
Date:   Fri Nov 13 02:27:31 2020 +0900

    bootconfig: Extend the magic check range to the preceding 3 bytes
    
    Since Grub may align the size of initrd to 4 if user pass
    initrd from cpio, we have to check the preceding 3 bytes as well.
    
    Link: https://lkml.kernel.org/r/160520205132.303174.4876760192433315429.stgit@devnote2
    
    Cc: stable@vger.kernel.org
    Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
    Reported-by: Chen Yu <yu.chen.surf@gmail.com>
    Tested-by: Chen Yu <yu.chen.surf@gmail.com>
    Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
    Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

diff --git a/init/main.c b/init/main.c
index 130376ec10ba..20baced721ad 100644
--- a/init/main.c
+++ b/init/main.c
@@ -269,14 +269,24 @@ static void * __init get_boot_config_from_initrd(u32 *_size, u32 *_csum)
 	u32 size, csum;
 	char *data;
 	u32 *hdr;
+	int i;
 
 	if (!initrd_end)
 		return NULL;
 
 	data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
-	if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
-		return NULL;
+	/*
+	 * Since Grub may align the size of initrd to 4, we must
+	 * check the preceding 3 bytes as well.
+	 */
+	for (i = 0; i < 4; i++) {
+		if (!memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
+			goto found;
+		data--;
+	}
+	return NULL;
 
+found:
 	hdr = (u32 *)(data - 8);
 	size = hdr[0];
 	csum = hdr[1];

Re: [GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[Linus Torvalds]

On Fri, Nov 13, 2020 at 5:29 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Fix alignment of bootconfig
>
> GRUB may align the init ramdisk size to 4 bytes, the magic number at the
> end of the init ramdisk that denotes bootconfig is attached may not be at
> the exact end of the ramdisk. The kernel needs to check back at least 4
> bytes.

I've pulled this, but this really smells to me.

Isn't the thing that actually _writes_ that BOOTCONFIG_MAGIC able to
fix this properly? I'm looking at the bootconfig tool, and wondering
why that doesn't know about the alignment thing, for example.

And the fact that this got screwed up means that the BOOTCONFIG
documentation needs updating too, so that the rules are documented and
proper.

                      Linus

Re: [GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[pr-tracker-bot]

The pull request you sent on Fri, 13 Nov 2020 08:29:30 -0500:

> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git trace-v5.10-rc3

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/6186313d06dfadbfd0cda5e36e485877d6600179

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[Steven Rostedt]

On Fri, 13 Nov 2020 09:43:31 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Nov 13, 2020 at 5:29 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Fix alignment of bootconfig
> >
> > GRUB may align the init ramdisk size to 4 bytes, the magic number at the
> > end of the init ramdisk that denotes bootconfig is attached may not be at
> > the exact end of the ramdisk. The kernel needs to check back at least 4
> > bytes.  
> 
> I've pulled this, but this really smells to me.
> 
> Isn't the thing that actually _writes_ that BOOTCONFIG_MAGIC able to
> fix this properly? I'm looking at the bootconfig tool, and wondering
> why that doesn't know about the alignment thing, for example.
> 
> And the fact that this got screwed up means that the BOOTCONFIG
> documentation needs updating too, so that the rules are documented and
> proper.
>

The issue is with grub. It will pad the initrd that it is given to make
sure that it ends on a 4 byte memory boundary. That is the bootconfig
tool adds itself at the end of the ramdisk. But, after grub loads it
into memory, if the ramdisk loaded ends at an off by one from finishing
at a 4 byte boundary, grub will append 3 more bytes. It then passes that
ending to the kernel.

The issue is that grub padded the end of the ramdisk after loading it
into memory. I'm not sure how the bootconfig tool can fix this. Perhaps
make sure the ram disk size is 4 bytes aligned?

Masami, correct me if my above explanation is incorrect. Thanks!

-- Steve

Re: [GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[Linus Torvalds]

On Fri, Nov 13, 2020 at 9:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> The issue is that grub padded the end of the ramdisk after loading it
> into memory. I'm not sure how the bootconfig tool can fix this. Perhaps
> make sure the ram disk size is 4 bytes aligned?

Exactly. Since - as far as I can tell - the _only_ thing that actually
generates that BOOTCONFIG_MAGIC marker is the bootconfig tool, you
control the vertical and the horizontal. No need for some "heuristic"
and searching for things.

And then that thing needs to be documented, so that if somebody else
starts generating BOOTCONFIG_MAGIC markers, we have a hard rule in
place that "look, the bootconfig is always aligned".

Might as well align it more than 4 bytes while at it and make it even stricter.

               Linus

Re: [GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[Steven Rostedt]

On Fri, 13 Nov 2020 09:57:54 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Nov 13, 2020 at 9:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > The issue is that grub padded the end of the ramdisk after loading it
> > into memory. I'm not sure how the bootconfig tool can fix this. Perhaps
> > make sure the ram disk size is 4 bytes aligned?  
> 
> Exactly. Since - as far as I can tell - the _only_ thing that actually
> generates that BOOTCONFIG_MAGIC marker is the bootconfig tool, you
> control the vertical and the horizontal. No need for some "heuristic"
> and searching for things.
> 
> And then that thing needs to be documented, so that if somebody else
> starts generating BOOTCONFIG_MAGIC markers, we have a hard rule in
> place that "look, the bootconfig is always aligned".
> 
> Might as well align it more than 4 bytes while at it and make it even stricter.
> 

OK, yes I agree with this.

Masami, can you send a patch to fix the bootconfig tool to make sure
that when it appends to the initrd that it makes sure the file size is
aligned. Would 32 bytes be big enough for an alignment?

-- Steve

Re: [GIT PULL] bootconfig: Extend the magic check range to the preceding 3 bytes

[Masami Hiramatsu]

On Fri, 13 Nov 2020 13:03:05 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 13 Nov 2020 09:57:54 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Fri, Nov 13, 2020 at 9:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > The issue is that grub padded the end of the ramdisk after loading it
> > > into memory. I'm not sure how the bootconfig tool can fix this. Perhaps
> > > make sure the ram disk size is 4 bytes aligned?  
> > 
> > Exactly. Since - as far as I can tell - the _only_ thing that actually
> > generates that BOOTCONFIG_MAGIC marker is the bootconfig tool, you
> > control the vertical and the horizontal. No need for some "heuristic"
> > and searching for things.
> > 
> > And then that thing needs to be documented, so that if somebody else
> > starts generating BOOTCONFIG_MAGIC markers, we have a hard rule in
> > place that "look, the bootconfig is always aligned".
> > 
> > Might as well align it more than 4 bytes while at it and make it even stricter.
> > 
> 
> OK, yes I agree with this.

OK, but note that the initrd_end can be modified by the bootloader anyway.

The bootloader can pass any "size" bigger than actual initramfs size
because initramfs is a cpio file which has a "TRAILER!!!" magic as the
end of file. This means kernel can ignore or use (as the bootconfig does)
for a tailing data storage.

But I agree that we need to document it, so that anyone can refer the
data format.

> Masami, can you send a patch to fix the bootconfig tool to make sure
> that when it appends to the initrd that it makes sure the file size is
> aligned. Would 32 bytes be big enough for an alignment?

OK, it is easy to me to update bootconfig tool to align up the total size
to 32bytes, but I think 4 bytes align is OK if we document it. Without
documentation, no one in the bootloader decides what is the correct
format.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>