linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* bootconfig length parse error in kernel
@ 2020-11-10 15:39 Chen Yu
  2020-11-11  9:37 ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: Chen Yu @ 2020-11-10 15:39 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: Linux Kernel Mailing List

Hi Masami,
Thanks for writing bootconfig and it is useful for boot up trace event
debugging.
However it was found that on 5.10-rc2 the bootconfig does not work and it shows
"'bootconfig' found on command line, but no bootconfig found"
And the reason for this is the kernel found the magic number to be incorrect.
I've added some hack in kernel to dump the first 12 bytes, it shows:
"OTCONFIG". So printed more content ahead we can find
"#BOOTCONFIG" ahead. So it looks that there is some alignment during
initrd load, and get_boot_config_from_initrd() might also deal with it. That is
to say:
data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
might do some alignment?



-- 
Thanks,
Chenyu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bootconfig length parse error in kernel
  2020-11-10 15:39 bootconfig length parse error in kernel Chen Yu
@ 2020-11-11  9:37 ` Masami Hiramatsu
  2020-11-12  4:34   ` Chen Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2020-11-11  9:37 UTC (permalink / raw)
  To: Chen Yu; +Cc: Linux Kernel Mailing List

Hi Chen,

On Tue, 10 Nov 2020 23:39:53 +0800
Chen Yu <yu.chen.surf@gmail.com> wrote:

> Hi Masami,
> Thanks for writing bootconfig and it is useful for boot up trace event
> debugging.

Thanks for testing!

> However it was found that on 5.10-rc2 the bootconfig does not work and it shows
> "'bootconfig' found on command line, but no bootconfig found"
> And the reason for this is the kernel found the magic number to be incorrect.
> I've added some hack in kernel to dump the first 12 bytes, it shows:
> "OTCONFIG". So printed more content ahead we can find
> "#BOOTCONFIG" ahead. So it looks that there is some alignment during
> initrd load, and get_boot_config_from_initrd() might also deal with it. That is
> to say:
> data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> might do some alignment?

Hrm, interesting. So initrd_end might be aligned. Could you print out the
actuall address of initrd_end? And could you tell me which platform are
you tested?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bootconfig length parse error in kernel
  2020-11-11  9:37 ` Masami Hiramatsu
@ 2020-11-12  4:34   ` Chen Yu
  2020-11-12  5:50     ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: Chen Yu @ 2020-11-12  4:34 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: Linux Kernel Mailing List, Chen Yu

Hi Masami,

On Wed, Nov 11, 2020 at 5:37 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Chen,
>
> On Tue, 10 Nov 2020 23:39:53 +0800
> Chen Yu <yu.chen.surf@gmail.com> wrote:
>
> > Hi Masami,
> > Thanks for writing bootconfig and it is useful for boot up trace event
> > debugging.
>
> Thanks for testing!
>
> > However it was found that on 5.10-rc2 the bootconfig does not work and it shows
> > "'bootconfig' found on command line, but no bootconfig found"
> > And the reason for this is the kernel found the magic number to be incorrect.
> > I've added some hack in kernel to dump the first 12 bytes, it shows:
> > "OTCONFIG". So printed more content ahead we can find
> > "#BOOTCONFIG" ahead. So it looks that there is some alignment during
> > initrd load, and get_boot_config_from_initrd() might also deal with it. That is
> > to say:
> > data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > might do some alignment?
>
> Hrm, interesting. So initrd_end might be aligned. Could you print out the
> actuall address of initrd_end?
I've done some investigation, it looks like this issue is not related
to alignment, but related to
the bootloader that has provided an inaccurate ramdisk size via
boot_params.hdr.ramdisk_size.
The actual size of initrd is:
ls /boot/initrd.img-5.10.0-rc3-e1000e-hw+ -l
-rw-r--r-- 1 root root 48689230 11月 12 00:08
/boot/initrd.img-5.10.0-rc3-e1000e-hw+
while the ramdisk size provided by bootloader via
boot_params.hdr.ramdisk_size is
48689232, which is 2 bytes bigger than the actual size, and this is
why the initrd_end
is bigger than expected and causing the missmatch of magic number.
Since there is no guarantee that bootloader provides the accurate
ramdisk size, an compromised
proposal might be that to search for the magic number a little ahead.
For example, the
following patch works for me:
diff --git a/init/main.c b/init/main.c
index 130376ec10ba..60fb125d44f4 100644
--- a/init/main.c
+++ b/init/main.c
@@ -273,7 +273,10 @@ static void * __init
get_boot_config_from_initrd(u32 *_size, u32 *_csum)
        if (!initrd_end)
                return NULL;

-       data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
+       data = memchr((char *)initrd_end - 2 * BOOTCONFIG_MAGIC_LEN,
+                      '#', BOOTCONFIG_MAGIC_LEN);
+       if (!data)
+               return NULL;
        if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
                return NULL;


> And could you tell me which platform are you tested?
>
It is HP ZHAN 99 Mobile Workstation G1 with i5-8300H, Ubuntu 20.04.

Thanks,
Chenyu

> Thank you,
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: bootconfig length parse error in kernel
  2020-11-12  4:34   ` Chen Yu
@ 2020-11-12  5:50     ` Masami Hiramatsu
  2020-11-12  6:49       ` Chen Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2020-11-12  5:50 UTC (permalink / raw)
  To: Chen Yu; +Cc: Linux Kernel Mailing List, Chen Yu

Hi Chen,

On Thu, 12 Nov 2020 12:34:36 +0800
Chen Yu <yu.chen.surf@gmail.com> wrote:

> Hi Masami,
> 
> On Wed, Nov 11, 2020 at 5:37 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Hi Chen,
> >
> > On Tue, 10 Nov 2020 23:39:53 +0800
> > Chen Yu <yu.chen.surf@gmail.com> wrote:
> >
> > > Hi Masami,
> > > Thanks for writing bootconfig and it is useful for boot up trace event
> > > debugging.
> >
> > Thanks for testing!
> >
> > > However it was found that on 5.10-rc2 the bootconfig does not work and it shows
> > > "'bootconfig' found on command line, but no bootconfig found"
> > > And the reason for this is the kernel found the magic number to be incorrect.
> > > I've added some hack in kernel to dump the first 12 bytes, it shows:
> > > "OTCONFIG". So printed more content ahead we can find
> > > "#BOOTCONFIG" ahead. So it looks that there is some alignment during
> > > initrd load, and get_boot_config_from_initrd() might also deal with it. That is
> > > to say:
> > > data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > > might do some alignment?
> >
> > Hrm, interesting. So initrd_end might be aligned. Could you print out the
> > actuall address of initrd_end?
> I've done some investigation, it looks like this issue is not related
> to alignment, but related to
> the bootloader that has provided an inaccurate ramdisk size via
> boot_params.hdr.ramdisk_size.

Yeah, it seems to happen. bootloader can pass wrong (bigger) size
to kernel. BTW, what bootloader would you use?

> The actual size of initrd is:
> ls /boot/initrd.img-5.10.0-rc3-e1000e-hw+ -l
> -rw-r--r-- 1 root root 48689230 11月 12 00:08
> /boot/initrd.img-5.10.0-rc3-e1000e-hw+
> while the ramdisk size provided by bootloader via
> boot_params.hdr.ramdisk_size is
> 48689232, which is 2 bytes bigger than the actual size, and this is
> why the initrd_end
> is bigger than expected and causing the missmatch of magic number.

OK. It seems that the bootloader might cut it up to 16 bytes
aligned. (But I think that's wrong behavior, there is no reason
to do it)

> Since there is no guarantee that bootloader provides the accurate
> ramdisk size, an compromised
> proposal might be that to search for the magic number a little ahead.

If the bootloader does such wrong behavior, there is no guarantee
that the size is "a little" bigger. IOW, it can be aligned to the
page size (4KB-)

> For example, the
> following patch works for me:
> diff --git a/init/main.c b/init/main.c
> index 130376ec10ba..60fb125d44f4 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -273,7 +273,10 @@ static void * __init
> get_boot_config_from_initrd(u32 *_size, u32 *_csum)
>         if (!initrd_end)
>                 return NULL;
> 
> -       data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> +       data = memchr((char *)initrd_end - 2 * BOOTCONFIG_MAGIC_LEN,
> +                      '#', BOOTCONFIG_MAGIC_LEN);
> +       if (!data)
> +               return NULL;

So this also does not guarantee that we can find "#" in BOOTCONFIG_MAGIC_LEN.
We need to find actual code in the bootloader, what it does.

>         if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
>                 return NULL;
> 
> 
> > And could you tell me which platform are you tested?
> >
> It is HP ZHAN 99 Mobile Workstation G1 with i5-8300H, Ubuntu 20.04.

Hmm, this means x86 Grub2 does this change. Let me check it.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bootconfig length parse error in kernel
  2020-11-12  5:50     ` Masami Hiramatsu
@ 2020-11-12  6:49       ` Chen Yu
  2020-11-12 15:36         ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: Chen Yu @ 2020-11-12  6:49 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: Linux Kernel Mailing List, Chen Yu

On Thu, Nov 12, 2020 at 1:50 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Chen,
>
> On Thu, 12 Nov 2020 12:34:36 +0800
> Chen Yu <yu.chen.surf@gmail.com> wrote:
>
> > Hi Masami,
> >
> > On Wed, Nov 11, 2020 at 5:37 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > Hi Chen,
> > >
> > > On Tue, 10 Nov 2020 23:39:53 +0800
> > > Chen Yu <yu.chen.surf@gmail.com> wrote:
> > >
> > > > Hi Masami,
> > > > Thanks for writing bootconfig and it is useful for boot up trace event
> > > > debugging.
> > >
> > > Thanks for testing!
> > >
> > > > However it was found that on 5.10-rc2 the bootconfig does not work and it shows
> > > > "'bootconfig' found on command line, but no bootconfig found"
> > > > And the reason for this is the kernel found the magic number to be incorrect.
> > > > I've added some hack in kernel to dump the first 12 bytes, it shows:
> > > > "OTCONFIG". So printed more content ahead we can find
> > > > "#BOOTCONFIG" ahead. So it looks that there is some alignment during
> > > > initrd load, and get_boot_config_from_initrd() might also deal with it. That is
> > > > to say:
> > > > data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > > > might do some alignment?
> > >
> > > Hrm, interesting. So initrd_end might be aligned. Could you print out the
> > > actuall address of initrd_end?
> > I've done some investigation, it looks like this issue is not related
> > to alignment, but related to
> > the bootloader that has provided an inaccurate ramdisk size via
> > boot_params.hdr.ramdisk_size.
>
> Yeah, it seems to happen. bootloader can pass wrong (bigger) size
> to kernel. BTW, what bootloader would you use?
>
It is
$ grub-install --version
grub-install (GRUB) 2.04-1ubuntu26.2
> > The actual size of initrd is:
> > ls /boot/initrd.img-5.10.0-rc3-e1000e-hw+ -l
> > -rw-r--r-- 1 root root 48689230 11月 12 00:08
> > /boot/initrd.img-5.10.0-rc3-e1000e-hw+
> > while the ramdisk size provided by bootloader via
> > boot_params.hdr.ramdisk_size is
> > 48689232, which is 2 bytes bigger than the actual size, and this is
> > why the initrd_end
> > is bigger than expected and causing the missmatch of magic number.
>
> OK. It seems that the bootloader might cut it up to 16 bytes
> aligned. (But I think that's wrong behavior, there is no reason
> to do it)
Agree.
>
> > Since there is no guarantee that bootloader provides the accurate
> > ramdisk size, an compromised
> > proposal might be that to search for the magic number a little ahead.
>
> If the bootloader does such wrong behavior, there is no guarantee
> that the size is "a little" bigger. IOW, it can be aligned to the
> page size (4KB-)
>
Right. How about inserting the bootconfig at initrd_start if
initrd_end could not be trusted?
> > For example, the
> > following patch works for me:
> > diff --git a/init/main.c b/init/main.c
> > index 130376ec10ba..60fb125d44f4 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -273,7 +273,10 @@ static void * __init
> > get_boot_config_from_initrd(u32 *_size, u32 *_csum)
> >         if (!initrd_end)
> >                 return NULL;
> >
> > -       data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > +       data = memchr((char *)initrd_end - 2 * BOOTCONFIG_MAGIC_LEN,
> > +                      '#', BOOTCONFIG_MAGIC_LEN);
> > +       if (!data)
> > +               return NULL;
>
> So this also does not guarantee that we can find "#" in BOOTCONFIG_MAGIC_LEN.
> We need to find actual code in the bootloader, what it does.
>
Indeed.
> >         if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
> >                 return NULL;
> >
> >
> > > And could you tell me which platform are you tested?
> > >
> > It is HP ZHAN 99 Mobile Workstation G1 with i5-8300H, Ubuntu 20.04.
>
> Hmm, this means x86 Grub2 does this change. Let me check it.
>
Okay.

Thanks,
Chenyu

> Thank you,
>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bootconfig length parse error in kernel
  2020-11-12  6:49       ` Chen Yu
@ 2020-11-12 15:36         ` Masami Hiramatsu
  2020-11-12 17:27           ` [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes Masami Hiramatsu
  2020-11-13  0:49           ` bootconfig length parse error in kernel Chen Yu
  0 siblings, 2 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-11-12 15:36 UTC (permalink / raw)
  To: Chen Yu; +Cc: Linux Kernel Mailing List, Chen Yu, Steven Rostedt

On Thu, 12 Nov 2020 14:49:16 +0800
Chen Yu <yu.chen.surf@gmail.com> wrote:

> On Thu, Nov 12, 2020 at 1:50 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Hi Chen,
> >
> > On Thu, 12 Nov 2020 12:34:36 +0800
> > Chen Yu <yu.chen.surf@gmail.com> wrote:
> >
> > > Hi Masami,
> > >
> > > On Wed, Nov 11, 2020 at 5:37 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > >
> > > > Hi Chen,
> > > >
> > > > On Tue, 10 Nov 2020 23:39:53 +0800
> > > > Chen Yu <yu.chen.surf@gmail.com> wrote:
> > > >
> > > > > Hi Masami,
> > > > > Thanks for writing bootconfig and it is useful for boot up trace event
> > > > > debugging.
> > > >
> > > > Thanks for testing!
> > > >
> > > > > However it was found that on 5.10-rc2 the bootconfig does not work and it shows
> > > > > "'bootconfig' found on command line, but no bootconfig found"
> > > > > And the reason for this is the kernel found the magic number to be incorrect.
> > > > > I've added some hack in kernel to dump the first 12 bytes, it shows:
> > > > > "OTCONFIG". So printed more content ahead we can find
> > > > > "#BOOTCONFIG" ahead. So it looks that there is some alignment during
> > > > > initrd load, and get_boot_config_from_initrd() might also deal with it. That is
> > > > > to say:
> > > > > data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > > > > might do some alignment?
> > > >
> > > > Hrm, interesting. So initrd_end might be aligned. Could you print out the
> > > > actuall address of initrd_end?
> > > I've done some investigation, it looks like this issue is not related
> > > to alignment, but related to
> > > the bootloader that has provided an inaccurate ramdisk size via
> > > boot_params.hdr.ramdisk_size.
> >
> > Yeah, it seems to happen. bootloader can pass wrong (bigger) size
> > to kernel. BTW, what bootloader would you use?
> >
> It is
> $ grub-install --version
> grub-install (GRUB) 2.04-1ubuntu26.2
> > > The actual size of initrd is:
> > > ls /boot/initrd.img-5.10.0-rc3-e1000e-hw+ -l
> > > -rw-r--r-- 1 root root 48689230 11月 12 00:08
> > > /boot/initrd.img-5.10.0-rc3-e1000e-hw+
> > > while the ramdisk size provided by bootloader via
> > > boot_params.hdr.ramdisk_size is
> > > 48689232, which is 2 bytes bigger than the actual size, and this is
> > > why the initrd_end
> > > is bigger than expected and causing the missmatch of magic number.
> >
> > OK. It seems that the bootloader might cut it up to 16 bytes
> > aligned. (But I think that's wrong behavior, there is no reason
> > to do it)
> Agree.
> >
> > > Since there is no guarantee that bootloader provides the accurate
> > > ramdisk size, an compromised
> > > proposal might be that to search for the magic number a little ahead.
> >
> > If the bootloader does such wrong behavior, there is no guarantee
> > that the size is "a little" bigger. IOW, it can be aligned to the
> > page size (4KB-)
> >
> Right. How about inserting the bootconfig at initrd_start if
> initrd_end could not be trusted?
> > > For example, the
> > > following patch works for me:
> > > diff --git a/init/main.c b/init/main.c
> > > index 130376ec10ba..60fb125d44f4 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -273,7 +273,10 @@ static void * __init
> > > get_boot_config_from_initrd(u32 *_size, u32 *_csum)
> > >         if (!initrd_end)
> > >                 return NULL;
> > >
> > > -       data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > > +       data = memchr((char *)initrd_end - 2 * BOOTCONFIG_MAGIC_LEN,
> > > +                      '#', BOOTCONFIG_MAGIC_LEN);
> > > +       if (!data)
> > > +               return NULL;
> >
> > So this also does not guarantee that we can find "#" in BOOTCONFIG_MAGIC_LEN.
> > We need to find actual code in the bootloader, what it does.
> >
> Indeed.
> > >         if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
> > >                 return NULL;
> > >
> > >
> > > > And could you tell me which platform are you tested?
> > > >
> > > It is HP ZHAN 99 Mobile Workstation G1 with i5-8300H, Ubuntu 20.04.
> >
> > Hmm, this means x86 Grub2 does this change. Let me check it.
> >

I found the 4 byte alignment code in the grub 
(grub_initrd_init()grub-core/loader/linux.c), but that seems to happen
only when load a file with newc:/PACKAGE/FILE format. (And this is not
documented.) At a glance, u-boot may not do that (but of course user
can pass different size directly by command), EDK2 doesn't too.
So, we should check at least 3 byte back for grub. 

BTW, just out of curious, what is your "initrd" command line in grub.conf? :)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes
  2020-11-12 15:36         ` Masami Hiramatsu
@ 2020-11-12 17:27           ` Masami Hiramatsu
  2020-11-13  1:27             ` Chen Yu
  2020-11-13  0:49           ` bootconfig length parse error in kernel Chen Yu
  1 sibling, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2020-11-12 17:27 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Chen Yu, Chen Yu, Masami Hiramatsu, linux-kernel

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.

Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
Reported-by: Chen Yu <yu.chen.surf@gmail.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 init/main.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

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];


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: bootconfig length parse error in kernel
  2020-11-12 15:36         ` Masami Hiramatsu
  2020-11-12 17:27           ` [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes Masami Hiramatsu
@ 2020-11-13  0:49           ` Chen Yu
  1 sibling, 0 replies; 11+ messages in thread
From: Chen Yu @ 2020-11-13  0:49 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: Linux Kernel Mailing List, Chen Yu, Steven Rostedt

On Thu, Nov 12, 2020 at 11:36 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Thu, 12 Nov 2020 14:49:16 +0800
> Chen Yu <yu.chen.surf@gmail.com> wrote:
>
> > On Thu, Nov 12, 2020 at 1:50 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > Hi Chen,
> > >
> > > On Thu, 12 Nov 2020 12:34:36 +0800
> > > Chen Yu <yu.chen.surf@gmail.com> wrote:
> > >
> > > > Hi Masami,
> > > >
> > > > On Wed, Nov 11, 2020 at 5:37 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > >
> > > > > Hi Chen,
> > > > >
> > > > > On Tue, 10 Nov 2020 23:39:53 +0800
> > > > > Chen Yu <yu.chen.surf@gmail.com> wrote:
> > > > >
> > > > > > Hi Masami,
> > > > > > Thanks for writing bootconfig and it is useful for boot up trace event
> > > > > > debugging.
> > > > >
> > > > > Thanks for testing!
> > > > >
> > > > > > However it was found that on 5.10-rc2 the bootconfig does not work and it shows
> > > > > > "'bootconfig' found on command line, but no bootconfig found"
> > > > > > And the reason for this is the kernel found the magic number to be incorrect.
> > > > > > I've added some hack in kernel to dump the first 12 bytes, it shows:
> > > > > > "OTCONFIG". So printed more content ahead we can find
> > > > > > "#BOOTCONFIG" ahead. So it looks that there is some alignment during
> > > > > > initrd load, and get_boot_config_from_initrd() might also deal with it. That is
> > > > > > to say:
> > > > > > data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > > > > > might do some alignment?
> > > > >
> > > > > Hrm, interesting. So initrd_end might be aligned. Could you print out the
> > > > > actuall address of initrd_end?
> > > > I've done some investigation, it looks like this issue is not related
> > > > to alignment, but related to
> > > > the bootloader that has provided an inaccurate ramdisk size via
> > > > boot_params.hdr.ramdisk_size.
> > >
> > > Yeah, it seems to happen. bootloader can pass wrong (bigger) size
> > > to kernel. BTW, what bootloader would you use?
> > >
> > It is
> > $ grub-install --version
> > grub-install (GRUB) 2.04-1ubuntu26.2
> > > > The actual size of initrd is:
> > > > ls /boot/initrd.img-5.10.0-rc3-e1000e-hw+ -l
> > > > -rw-r--r-- 1 root root 48689230 11月 12 00:08
> > > > /boot/initrd.img-5.10.0-rc3-e1000e-hw+
> > > > while the ramdisk size provided by bootloader via
> > > > boot_params.hdr.ramdisk_size is
> > > > 48689232, which is 2 bytes bigger than the actual size, and this is
> > > > why the initrd_end
> > > > is bigger than expected and causing the missmatch of magic number.
> > >
> > > OK. It seems that the bootloader might cut it up to 16 bytes
> > > aligned. (But I think that's wrong behavior, there is no reason
> > > to do it)
> > Agree.
> > >
> > > > Since there is no guarantee that bootloader provides the accurate
> > > > ramdisk size, an compromised
> > > > proposal might be that to search for the magic number a little ahead.
> > >
> > > If the bootloader does such wrong behavior, there is no guarantee
> > > that the size is "a little" bigger. IOW, it can be aligned to the
> > > page size (4KB-)
> > >
> > Right. How about inserting the bootconfig at initrd_start if
> > initrd_end could not be trusted?
> > > > For example, the
> > > > following patch works for me:
> > > > diff --git a/init/main.c b/init/main.c
> > > > index 130376ec10ba..60fb125d44f4 100644
> > > > --- a/init/main.c
> > > > +++ b/init/main.c
> > > > @@ -273,7 +273,10 @@ static void * __init
> > > > get_boot_config_from_initrd(u32 *_size, u32 *_csum)
> > > >         if (!initrd_end)
> > > >                 return NULL;
> > > >
> > > > -       data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
> > > > +       data = memchr((char *)initrd_end - 2 * BOOTCONFIG_MAGIC_LEN,
> > > > +                      '#', BOOTCONFIG_MAGIC_LEN);
> > > > +       if (!data)
> > > > +               return NULL;
> > >
> > > So this also does not guarantee that we can find "#" in BOOTCONFIG_MAGIC_LEN.
> > > We need to find actual code in the bootloader, what it does.
> > >
> > Indeed.
> > > >         if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
> > > >                 return NULL;
> > > >
> > > >
> > > > > And could you tell me which platform are you tested?
> > > > >
> > > > It is HP ZHAN 99 Mobile Workstation G1 with i5-8300H, Ubuntu 20.04.
> > >
> > > Hmm, this means x86 Grub2 does this change. Let me check it.
> > >
>
> I found the 4 byte alignment code in the grub
> (grub_initrd_init()grub-core/loader/linux.c), but that seems to happen
> only when load a file with newc:/PACKAGE/FILE format. (And this is not
> documented.) At a glance, u-boot may not do that (but of course user
> can pass different size directly by command), EDK2 doesn't too.
> So, we should check at least 3 byte back for grub.
>
> BTW, just out of curious, what is your "initrd" command line in grub.conf? :)
>
It is:
 linux   /boot/vmlinuz-5.10.0-rc3-e1000e-hw+
root=UUID=21f729c7-4ab0-40a9-b8cf-6f22b3851b9f ro  ignore_loglevel
no_console_suspend bootconfig
 initrd  /boot/initrd.img-5.10.0-rc3-e1000e-hw+
Thanks,
Chenyu

> Thank you,
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes
  2020-11-12 17:27           ` [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes Masami Hiramatsu
@ 2020-11-13  1:27             ` Chen Yu
  2020-11-13  2:21               ` Masami Hiramatsu
  0 siblings, 1 reply; 11+ messages in thread
From: Chen Yu @ 2020-11-13  1:27 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: Steven Rostedt, Chen Yu, Linux Kernel Mailing List

On Fri, Nov 13, 2020 at 1:27 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> 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.
>
> Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
> Reported-by: Chen Yu <yu.chen.surf@gmail.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
Works for me, thanks!
Tested-by: Chen Yu <yu.chen.surf@gmail.com>

Best,
Chenyu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes
  2020-11-13  1:27             ` Chen Yu
@ 2020-11-13  2:21               ` Masami Hiramatsu
  2020-11-13  3:16                 ` Steven Rostedt
  0 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2020-11-13  2:21 UTC (permalink / raw)
  To: Chen Yu; +Cc: Steven Rostedt, Chen Yu, Linux Kernel Mailing List

On Fri, 13 Nov 2020 09:27:38 +0800
Chen Yu <yu.chen.surf@gmail.com> wrote:

> On Fri, Nov 13, 2020 at 1:27 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > 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.
> >
> > Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
> > Reported-by: Chen Yu <yu.chen.surf@gmail.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> Works for me, thanks!
> Tested-by: Chen Yu <yu.chen.surf@gmail.com>

Thank you Chen!

Steve, could you merge this fix? 

Thank you,

> 
> Best,
> Chenyu


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes
  2020-11-13  2:21               ` Masami Hiramatsu
@ 2020-11-13  3:16                 ` Steven Rostedt
  0 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2020-11-13  3:16 UTC (permalink / raw)
  To: Masami Hiramatsu, Chen Yu; +Cc: Chen Yu, Linux Kernel Mailing List



On November 12, 2020 9:21:08 PM EST, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>On Fri, 13 Nov 2020 09:27:38 +0800
>Chen Yu <yu.chen.surf@gmail.com> wrote:
>
>> On Fri, Nov 13, 2020 at 1:27 AM Masami Hiramatsu
><mhiramat@kernel.org> wrote:
>> >
>> > 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.
>> >
>> > Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for
>indicating bootconfig explicitly")
>> > Reported-by: Chen Yu <yu.chen.surf@gmail.com>
>> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
>> > ---
>> Works for me, thanks!
>> Tested-by: Chen Yu <yu.chen.surf@gmail.com>
>
>Thank you Chen!
>
>Steve, could you merge this fix? 
>

I'm testing it now.

-- Steve

>Thank you,
>
>> 
>> Best,
>> Chenyu

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity and top posting.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-11-13  3:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-10 15:39 bootconfig length parse error in kernel Chen Yu
2020-11-11  9:37 ` Masami Hiramatsu
2020-11-12  4:34   ` Chen Yu
2020-11-12  5:50     ` Masami Hiramatsu
2020-11-12  6:49       ` Chen Yu
2020-11-12 15:36         ` Masami Hiramatsu
2020-11-12 17:27           ` [PATCH] bootconfig: Extend the magic check range to the preceding 3 bytes Masami Hiramatsu
2020-11-13  1:27             ` Chen Yu
2020-11-13  2:21               ` Masami Hiramatsu
2020-11-13  3:16                 ` Steven Rostedt
2020-11-13  0:49           ` bootconfig length parse error in kernel Chen Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).