[PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Ard Biesheuvel]

Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
PE/COFF entrypoint") did some code refactoring to get rid of the
EFI entry point assembler code, and in the process, it got rid of the
assignment of image_addr to the value of _text. Instead, it switched
to using the image_base field of the efi_loaded_image struct provided
by UEFI, which should contain the same value.

However, Michael reports that this is not the case: older GRUB builds
corrupt this value in some way, and since we can easily switch back to
referring to _text to discover this value, let's simply do that.

While at it, fix another issue in commit 9f9223778ef3, which may result
in the unassigned image_addr to be misidentified as the preferred load
offset of the kernel, which is unlikely but will cause a boot crash if
it does occur.

Finally, let's add a warning if the _text vs. image_base discrepancy is
detected, so we can tell more easily how widespread this issue actually
is.

Reported-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/arm64-stub.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index 9254cd8ab2d3..db0c1a9c1699 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -116,6 +116,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 		 * Mustang), we can still place the kernel at the address
 		 * 'dram_base + TEXT_OFFSET'.
 		 */
+		*image_addr = (unsigned long)_text;
 		if (*image_addr == preferred_offset)
 			return EFI_SUCCESS;
 
@@ -140,7 +141,11 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 		}
 		*image_addr = *reserve_addr + TEXT_OFFSET;
 	}
-	memcpy((void *)*image_addr, image->image_base, kernel_size);
+
+	if (image->image_base != _text)
+		pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
+
+	memcpy((void *)*image_addr, _text, kernel_size);
 
 	return EFI_SUCCESS;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Michael Kelley]

From: Ard Biesheuvel <ardb@kernel.org> Sent: Saturday, March 28, 2020 1:58 PM
> 
> Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> PE/COFF entrypoint") did some code refactoring to get rid of the
> EFI entry point assembler code, and in the process, it got rid of the
> assignment of image_addr to the value of _text. Instead, it switched
> to using the image_base field of the efi_loaded_image struct provided
> by UEFI, which should contain the same value.
> 
> However, Michael reports that this is not the case: older GRUB builds
> corrupt this value in some way, and since we can easily switch back to
> referring to _text to discover this value, let's simply do that.
> 
> While at it, fix another issue in commit 9f9223778ef3, which may result
> in the unassigned image_addr to be misidentified as the preferred load
> offset of the kernel, which is unlikely but will cause a boot crash if
> it does occur.
> 
> Finally, let's add a warning if the _text vs. image_base discrepancy is
> detected, so we can tell more easily how widespread this issue actually
> is.
> 
> Reported-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Tested in a Hyper-V VM.  With the 2.02~beta2 version of grub,
the FIRMWARE BUG message is output and then Linux correctly
boots.  With the 2.04-4 version of grub, Linux correctly boots with
no error messages.

Tested-by: Michael Kelley <mikelley@microsoft.com>

> 
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c
> b/drivers/firmware/efi/libstub/arm64-stub.c
> index 9254cd8ab2d3..db0c1a9c1699 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -116,6 +116,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>  		 * Mustang), we can still place the kernel at the address
>  		 * 'dram_base + TEXT_OFFSET'.
>  		 */
> +		*image_addr = (unsigned long)_text;
>  		if (*image_addr == preferred_offset)
>  			return EFI_SUCCESS;
> 
> @@ -140,7 +141,11 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>  		}
>  		*image_addr = *reserve_addr + TEXT_OFFSET;
>  	}
> -	memcpy((void *)*image_addr, image->image_base, kernel_size);
> +
> +	if (image->image_base != _text)
> +		pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus
> value\n");
> +
> +	memcpy((void *)*image_addr, _text, kernel_size);
> 
>  	return EFI_SUCCESS;
>  }
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Leif Lindholm]

On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> PE/COFF entrypoint") did some code refactoring to get rid of the
> EFI entry point assembler code, and in the process, it got rid of the
> assignment of image_addr to the value of _text. Instead, it switched
> to using the image_base field of the efi_loaded_image struct provided
> by UEFI, which should contain the same value.
> 
> However, Michael reports that this is not the case: older GRUB builds
> corrupt this value in some way, and since we can easily switch back to
> referring to _text to discover this value, let's simply do that.

It is not clear to me how "older GRUB builds" would differ here.
I think more investigation is needed before making that claim.
My suspicion is that some (old) version of non-upstream, shim-enabled
distro-specific build is playing a part.

So, do we have the option for more detailed investigations, or can we
vague the claim up to say "some GRUB builds seen in the wild, based
on an upstream 2.02" or suchlike?

/
    Leif

> While at it, fix another issue in commit 9f9223778ef3, which may result
> in the unassigned image_addr to be misidentified as the preferred load
> offset of the kernel, which is unlikely but will cause a boot crash if
> it does occur.
> 
> Finally, let's add a warning if the _text vs. image_base discrepancy is
> detected, so we can tell more easily how widespread this issue actually
> is.
> 
> Reported-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index 9254cd8ab2d3..db0c1a9c1699 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -116,6 +116,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>  		 * Mustang), we can still place the kernel at the address
>  		 * 'dram_base + TEXT_OFFSET'.
>  		 */
> +		*image_addr = (unsigned long)_text;
>  		if (*image_addr == preferred_offset)
>  			return EFI_SUCCESS;
>  
> @@ -140,7 +141,11 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>  		}
>  		*image_addr = *reserve_addr + TEXT_OFFSET;
>  	}
> -	memcpy((void *)*image_addr, image->image_base, kernel_size);
> +
> +	if (image->image_base != _text)
> +		pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> +
> +	memcpy((void *)*image_addr, _text, kernel_size);
>  
>  	return EFI_SUCCESS;
>  }
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Ard Biesheuvel]

On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > PE/COFF entrypoint") did some code refactoring to get rid of the
> > EFI entry point assembler code, and in the process, it got rid of the
> > assignment of image_addr to the value of _text. Instead, it switched
> > to using the image_base field of the efi_loaded_image struct provided
> > by UEFI, which should contain the same value.
> >
> > However, Michael reports that this is not the case: older GRUB builds
> > corrupt this value in some way, and since we can easily switch back to
> > referring to _text to discover this value, let's simply do that.
>
> It is not clear to me how "older GRUB builds" would differ here.
> I think more investigation is needed before making that claim.
> My suspicion is that some (old) version of non-upstream, shim-enabled
> distro-specific build is playing a part.
>
> So, do we have the option for more detailed investigations, or can we
> vague the claim up to say "some GRUB builds seen in the wild, based
> on an upstream 2.02" or suchlike?
>

I've queued a fix that prints a nastygram if the value deviates from
the expected one. Let's see if this triggers any reports.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Ard Biesheuvel]

On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > > PE/COFF entrypoint") did some code refactoring to get rid of the
> > > EFI entry point assembler code, and in the process, it got rid of the
> > > assignment of image_addr to the value of _text. Instead, it switched
> > > to using the image_base field of the efi_loaded_image struct provided
> > > by UEFI, which should contain the same value.
> > >
> > > However, Michael reports that this is not the case: older GRUB builds
> > > corrupt this value in some way, and since we can easily switch back to
> > > referring to _text to discover this value, let's simply do that.
> >
> > It is not clear to me how "older GRUB builds" would differ here.
> > I think more investigation is needed before making that claim.
> > My suspicion is that some (old) version of non-upstream, shim-enabled
> > distro-specific build is playing a part.
> >
> > So, do we have the option for more detailed investigations, or can we
> > vague the claim up to say "some GRUB builds seen in the wild, based
> > on an upstream 2.02" or suchlike?
> >
>
> I've queued a fix that prints a nastygram if the value deviates from
> the expected one. Let's see if this triggers any reports.

(/me looks at context)

*This* is the fix that prints a nastygram.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Michael Kelley]

From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
> 
> On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> > >
> > > On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > > > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > > > PE/COFF entrypoint") did some code refactoring to get rid of the
> > > > EFI entry point assembler code, and in the process, it got rid of the
> > > > assignment of image_addr to the value of _text. Instead, it switched
> > > > to using the image_base field of the efi_loaded_image struct provided
> > > > by UEFI, which should contain the same value.
> > > >
> > > > However, Michael reports that this is not the case: older GRUB builds
> > > > corrupt this value in some way, and since we can easily switch back to
> > > > referring to _text to discover this value, let's simply do that.
> > >
> > > It is not clear to me how "older GRUB builds" would differ here.
> > > I think more investigation is needed before making that claim.
> > > My suspicion is that some (old) version of non-upstream, shim-enabled
> > > distro-specific build is playing a part.
> > >
> > > So, do we have the option for more detailed investigations, or can we
> > > vague the claim up to say "some GRUB builds seen in the wild, based
> > > on an upstream 2.02" or suchlike?
> > >
> >
> > I've queued a fix that prints a nastygram if the value deviates from
> > the expected one. Let's see if this triggers any reports.
> 
> (/me looks at context)
> 
> *This* is the fix that prints a nastygram.

FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
and CentOS 8.0 binary packages and tested both in my Hyper-V VM. 
Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
shows: 

	User-Agent: GRUB 2.02~beta2

The CentOS 8.0 grubaa64.efi shows:

	User-Agent: GRUB 2.03

Both versions produce the FIRMWARE BUG warning when using Ard's
latest patch.  I'll assume the equivalent RHEL versions are the same.
So we've got official distro releases that show the problem.

As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
Debian release (not exactly sure which one) do not produce the
FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.

Michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Ard Biesheuvel]

On Mon, 30 Mar 2020 at 20:12, Michael Kelley <mikelley@microsoft.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
> >
> > On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> > > >
> > > > On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > > > > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > > > > PE/COFF entrypoint") did some code refactoring to get rid of the
> > > > > EFI entry point assembler code, and in the process, it got rid of the
> > > > > assignment of image_addr to the value of _text. Instead, it switched
> > > > > to using the image_base field of the efi_loaded_image struct provided
> > > > > by UEFI, which should contain the same value.
> > > > >
> > > > > However, Michael reports that this is not the case: older GRUB builds
> > > > > corrupt this value in some way, and since we can easily switch back to
> > > > > referring to _text to discover this value, let's simply do that.
> > > >
> > > > It is not clear to me how "older GRUB builds" would differ here.
> > > > I think more investigation is needed before making that claim.
> > > > My suspicion is that some (old) version of non-upstream, shim-enabled
> > > > distro-specific build is playing a part.
> > > >
> > > > So, do we have the option for more detailed investigations, or can we
> > > > vague the claim up to say "some GRUB builds seen in the wild, based
> > > > on an upstream 2.02" or suchlike?
> > > >
> > >
> > > I've queued a fix that prints a nastygram if the value deviates from
> > > the expected one. Let's see if this triggers any reports.
> >
> > (/me looks at context)
> >
> > *This* is the fix that prints a nastygram.
>
> FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
> and CentOS 8.0 binary packages and tested both in my Hyper-V VM.
> Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
> shows:
>
>         User-Agent: GRUB 2.02~beta2
>
> The CentOS 8.0 grubaa64.efi shows:
>
>         User-Agent: GRUB 2.03
>
> Both versions produce the FIRMWARE BUG warning when using Ard's
> latest patch.  I'll assume the equivalent RHEL versions are the same.
> So we've got official distro releases that show the problem.
>
> As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
> Debian release (not exactly sure which one) do not produce the
> FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.
>

Thanks a lot Michael, that is really helpful.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Ard Biesheuvel]

On Mon, 30 Mar 2020 at 20:24, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 30 Mar 2020 at 20:12, Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
> > >
> > > On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> > > > >
> > > > > On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > > > > > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > > > > > PE/COFF entrypoint") did some code refactoring to get rid of the
> > > > > > EFI entry point assembler code, and in the process, it got rid of the
> > > > > > assignment of image_addr to the value of _text. Instead, it switched
> > > > > > to using the image_base field of the efi_loaded_image struct provided
> > > > > > by UEFI, which should contain the same value.
> > > > > >
> > > > > > However, Michael reports that this is not the case: older GRUB builds
> > > > > > corrupt this value in some way, and since we can easily switch back to
> > > > > > referring to _text to discover this value, let's simply do that.
> > > > >
> > > > > It is not clear to me how "older GRUB builds" would differ here.
> > > > > I think more investigation is needed before making that claim.
> > > > > My suspicion is that some (old) version of non-upstream, shim-enabled
> > > > > distro-specific build is playing a part.
> > > > >
> > > > > So, do we have the option for more detailed investigations, or can we
> > > > > vague the claim up to say "some GRUB builds seen in the wild, based
> > > > > on an upstream 2.02" or suchlike?
> > > > >
> > > >
> > > > I've queued a fix that prints a nastygram if the value deviates from
> > > > the expected one. Let's see if this triggers any reports.
> > >
> > > (/me looks at context)
> > >
> > > *This* is the fix that prints a nastygram.
> >
> > FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
> > and CentOS 8.0 binary packages and tested both in my Hyper-V VM.
> > Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
> > shows:
> >
> >         User-Agent: GRUB 2.02~beta2
> >
> > The CentOS 8.0 grubaa64.efi shows:
> >
> >         User-Agent: GRUB 2.03
> >
> > Both versions produce the FIRMWARE BUG warning when using Ard's
> > latest patch.  I'll assume the equivalent RHEL versions are the same.
> > So we've got official distro releases that show the problem.
> >
> > As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
> > Debian release (not exactly sure which one) do not produce the
> > FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.
> >
>
> Thanks a lot Michael, that is really helpful.

I could not reproduce the issue with Debian Stretch's
2.02~beta3-5+deb9u2, so it does appear to be RedHat's value add that
is to blame here.

@Laszlo: TL;DR RedHat's GRUB for arm64 appears to clobber the
image_base field of the efi_loaded_image struct passed to the kernel.
Could you please recommend a way to report this?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Michael Kelley]

From: Ard Biesheuvel <ardb@kernel.org>  Sent: Tuesday, March 31, 2020 12:56 AM
> 
> On Mon, 30 Mar 2020 at 20:24, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 30 Mar 2020 at 20:12, Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
> > > >
> > > > On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >
> > > > > On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> > > > > >
> > > > > > On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > > > > > > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > > > > > > PE/COFF entrypoint") did some code refactoring to get rid of the
> > > > > > > EFI entry point assembler code, and in the process, it got rid of the
> > > > > > > assignment of image_addr to the value of _text. Instead, it switched
> > > > > > > to using the image_base field of the efi_loaded_image struct provided
> > > > > > > by UEFI, which should contain the same value.
> > > > > > >
> > > > > > > However, Michael reports that this is not the case: older GRUB builds
> > > > > > > corrupt this value in some way, and since we can easily switch back to
> > > > > > > referring to _text to discover this value, let's simply do that.
> > > > > >
> > > > > > It is not clear to me how "older GRUB builds" would differ here.
> > > > > > I think more investigation is needed before making that claim.
> > > > > > My suspicion is that some (old) version of non-upstream, shim-enabled
> > > > > > distro-specific build is playing a part.
> > > > > >
> > > > > > So, do we have the option for more detailed investigations, or can we
> > > > > > vague the claim up to say "some GRUB builds seen in the wild, based
> > > > > > on an upstream 2.02" or suchlike?
> > > > > >
> > > > >
> > > > > I've queued a fix that prints a nastygram if the value deviates from
> > > > > the expected one. Let's see if this triggers any reports.
> > > >
> > > > (/me looks at context)
> > > >
> > > > *This* is the fix that prints a nastygram.
> > >
> > > FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
> > > and CentOS 8.0 binary packages and tested both in my Hyper-V VM.
> > > Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
> > > shows:
> > >
> > >         User-Agent: GRUB 2.02~beta2
> > >
> > > The CentOS 8.0 grubaa64.efi shows:
> > >
> > >         User-Agent: GRUB 2.03
> > >
> > > Both versions produce the FIRMWARE BUG warning when using Ard's
> > > latest patch.  I'll assume the equivalent RHEL versions are the same.
> > > So we've got official distro releases that show the problem.
> > >
> > > As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
> > > Debian release (not exactly sure which one) do not produce the
> > > FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.
> > >
> >
> > Thanks a lot Michael, that is really helpful.
> 
> I could not reproduce the issue with Debian Stretch's
> 2.02~beta3-5+deb9u2, so it does appear to be RedHat's value add that
> is to blame here.
> 
> @Laszlo: TL;DR RedHat's GRUB for arm64 appears to clobber the
> image_base field of the efi_loaded_image struct passed to the kernel.
> Could you please recommend a way to report this?

Or there could be something weird about my Hyper-V VM.  I have a
couple of ARM64 bare metal machines, but they are in the office
with no remote access, and we're on the work-from-home plan for
now.  But I may be able to get into the office later this week and
try it.  I'd like to rule out anything related to Hyper-V, and will
update this thread if I can.

Michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Michael Kelley]

From: Michael Kelley  Sent: Tuesday, March 31, 2020 6:38 AM
> 
> From: Ard Biesheuvel <ardb@kernel.org>  Sent: Tuesday, March 31, 2020 12:56 AM
> >
> > On Mon, 30 Mar 2020 at 20:24, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Mon, 30 Mar 2020 at 20:12, Michael Kelley <mikelley@microsoft.com> wrote:
> > > >
> > > > From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
> > > > >
> > > > > On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> > > > > > >
> > > > > > > On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > > > > > > > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > > > > > > > PE/COFF entrypoint") did some code refactoring to get rid of the
> > > > > > > > EFI entry point assembler code, and in the process, it got rid of the
> > > > > > > > assignment of image_addr to the value of _text. Instead, it switched
> > > > > > > > to using the image_base field of the efi_loaded_image struct provided
> > > > > > > > by UEFI, which should contain the same value.
> > > > > > > >
> > > > > > > > However, Michael reports that this is not the case: older GRUB builds
> > > > > > > > corrupt this value in some way, and since we can easily switch back to
> > > > > > > > referring to _text to discover this value, let's simply do that.
> > > > > > >
> > > > > > > It is not clear to me how "older GRUB builds" would differ here.
> > > > > > > I think more investigation is needed before making that claim.
> > > > > > > My suspicion is that some (old) version of non-upstream, shim-enabled
> > > > > > > distro-specific build is playing a part.
> > > > > > >
> > > > > > > So, do we have the option for more detailed investigations, or can we
> > > > > > > vague the claim up to say "some GRUB builds seen in the wild, based
> > > > > > > on an upstream 2.02" or suchlike?
> > > > > > >
> > > > > >
> > > > > > I've queued a fix that prints a nastygram if the value deviates from
> > > > > > the expected one. Let's see if this triggers any reports.
> > > > >
> > > > > (/me looks at context)
> > > > >
> > > > > *This* is the fix that prints a nastygram.
> > > >
> > > > FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
> > > > and CentOS 8.0 binary packages and tested both in my Hyper-V VM.
> > > > Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
> > > > shows:
> > > >
> > > >         User-Agent: GRUB 2.02~beta2
> > > >
> > > > The CentOS 8.0 grubaa64.efi shows:
> > > >
> > > >         User-Agent: GRUB 2.03
> > > >
> > > > Both versions produce the FIRMWARE BUG warning when using Ard's
> > > > latest patch.  I'll assume the equivalent RHEL versions are the same.
> > > > So we've got official distro releases that show the problem.
> > > >
> > > > As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
> > > > Debian release (not exactly sure which one) do not produce the
> > > > FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.
> > > >
> > >
> > > Thanks a lot Michael, that is really helpful.
> >
> > I could not reproduce the issue with Debian Stretch's
> > 2.02~beta3-5+deb9u2, so it does appear to be RedHat's value add that
> > is to blame here.
> >
> > @Laszlo: TL;DR RedHat's GRUB for arm64 appears to clobber the
> > image_base field of the efi_loaded_image struct passed to the kernel.
> > Could you please recommend a way to report this?
> 
> Or there could be something weird about my Hyper-V VM.  I have a
> couple of ARM64 bare metal machines, but they are in the office
> with no remote access, and we're on the work-from-home plan for
> now.  But I may be able to get into the office later this week and
> try it.  I'd like to rule out anything related to Hyper-V, and will
> update this thread if I can.
> 

I have tried this scenario on bare metal running RHEL 7.4, and the
problem reproduces as described above.  So this is *not* related to
running in a Hyper-V VM, which is what I wanted to make sure of.

Michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Ard Biesheuvel]

On Mon, 6 Apr 2020 at 19:13, Michael Kelley <mikelley@microsoft.com> wrote:
>
> From: Michael Kelley  Sent: Tuesday, March 31, 2020 6:38 AM
> >
> > From: Ard Biesheuvel <ardb@kernel.org>  Sent: Tuesday, March 31, 2020 12:56 AM
> > >
> > > On Mon, 30 Mar 2020 at 20:24, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Mon, 30 Mar 2020 at 20:12, Michael Kelley <mikelley@microsoft.com> wrote:
> > > > >
> > > > > From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
> > > > > >
> > > > > > On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > > >
> > > > > > > On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> > > > > > > > > Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> > > > > > > > > PE/COFF entrypoint") did some code refactoring to get rid of the
> > > > > > > > > EFI entry point assembler code, and in the process, it got rid of the
> > > > > > > > > assignment of image_addr to the value of _text. Instead, it switched
> > > > > > > > > to using the image_base field of the efi_loaded_image struct provided
> > > > > > > > > by UEFI, which should contain the same value.
> > > > > > > > >
> > > > > > > > > However, Michael reports that this is not the case: older GRUB builds
> > > > > > > > > corrupt this value in some way, and since we can easily switch back to
> > > > > > > > > referring to _text to discover this value, let's simply do that.
> > > > > > > >
> > > > > > > > It is not clear to me how "older GRUB builds" would differ here.
> > > > > > > > I think more investigation is needed before making that claim.
> > > > > > > > My suspicion is that some (old) version of non-upstream, shim-enabled
> > > > > > > > distro-specific build is playing a part.
> > > > > > > >
> > > > > > > > So, do we have the option for more detailed investigations, or can we
> > > > > > > > vague the claim up to say "some GRUB builds seen in the wild, based
> > > > > > > > on an upstream 2.02" or suchlike?
> > > > > > > >
> > > > > > >
> > > > > > > I've queued a fix that prints a nastygram if the value deviates from
> > > > > > > the expected one. Let's see if this triggers any reports.
> > > > > >
> > > > > > (/me looks at context)
> > > > > >
> > > > > > *This* is the fix that prints a nastygram.
> > > > >
> > > > > FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
> > > > > and CentOS 8.0 binary packages and tested both in my Hyper-V VM.
> > > > > Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
> > > > > shows:
> > > > >
> > > > >         User-Agent: GRUB 2.02~beta2
> > > > >
> > > > > The CentOS 8.0 grubaa64.efi shows:
> > > > >
> > > > >         User-Agent: GRUB 2.03
> > > > >
> > > > > Both versions produce the FIRMWARE BUG warning when using Ard's
> > > > > latest patch.  I'll assume the equivalent RHEL versions are the same.
> > > > > So we've got official distro releases that show the problem.
> > > > >
> > > > > As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
> > > > > Debian release (not exactly sure which one) do not produce the
> > > > > FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.
> > > > >
> > > >
> > > > Thanks a lot Michael, that is really helpful.
> > >
> > > I could not reproduce the issue with Debian Stretch's
> > > 2.02~beta3-5+deb9u2, so it does appear to be RedHat's value add that
> > > is to blame here.
> > >
> > > @Laszlo: TL;DR RedHat's GRUB for arm64 appears to clobber the
> > > image_base field of the efi_loaded_image struct passed to the kernel.
> > > Could you please recommend a way to report this?
> >
> > Or there could be something weird about my Hyper-V VM.  I have a
> > couple of ARM64 bare metal machines, but they are in the office
> > with no remote access, and we're on the work-from-home plan for
> > now.  But I may be able to get into the office later this week and
> > try it.  I'd like to rule out anything related to Hyper-V, and will
> > update this thread if I can.
> >
>
> I have tried this scenario on bare metal running RHEL 7.4, and the
> problem reproduces as described above.  So this is *not* related to
> running in a Hyper-V VM, which is what I wanted to make sure of.
>

Thanks Michael. I'll mention this in the bugzilla entry.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Laszlo Ersek]

On 03/31/20 09:56, Ard Biesheuvel wrote:
> On Mon, 30 Mar 2020 at 20:24, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Mon, 30 Mar 2020 at 20:12, Michael Kelley <mikelley@microsoft.com> wrote:
>>>
>>> From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
>>>>
>>>> On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>>
>>>>> On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
>>>>>>
>>>>>> On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
>>>>>>> Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
>>>>>>> PE/COFF entrypoint") did some code refactoring to get rid of the
>>>>>>> EFI entry point assembler code, and in the process, it got rid of the
>>>>>>> assignment of image_addr to the value of _text. Instead, it switched
>>>>>>> to using the image_base field of the efi_loaded_image struct provided
>>>>>>> by UEFI, which should contain the same value.
>>>>>>>
>>>>>>> However, Michael reports that this is not the case: older GRUB builds
>>>>>>> corrupt this value in some way, and since we can easily switch back to
>>>>>>> referring to _text to discover this value, let's simply do that.
>>>>>>
>>>>>> It is not clear to me how "older GRUB builds" would differ here.
>>>>>> I think more investigation is needed before making that claim.
>>>>>> My suspicion is that some (old) version of non-upstream, shim-enabled
>>>>>> distro-specific build is playing a part.
>>>>>>
>>>>>> So, do we have the option for more detailed investigations, or can we
>>>>>> vague the claim up to say "some GRUB builds seen in the wild, based
>>>>>> on an upstream 2.02" or suchlike?
>>>>>>
>>>>>
>>>>> I've queued a fix that prints a nastygram if the value deviates from
>>>>> the expected one. Let's see if this triggers any reports.
>>>>
>>>> (/me looks at context)
>>>>
>>>> *This* is the fix that prints a nastygram.
>>>
>>> FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
>>> and CentOS 8.0 binary packages and tested both in my Hyper-V VM.
>>> Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
>>> shows:
>>>
>>>         User-Agent: GRUB 2.02~beta2
>>>
>>> The CentOS 8.0 grubaa64.efi shows:
>>>
>>>         User-Agent: GRUB 2.03
>>>
>>> Both versions produce the FIRMWARE BUG warning when using Ard's
>>> latest patch.  I'll assume the equivalent RHEL versions are the same.
>>> So we've got official distro releases that show the problem.
>>>
>>> As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
>>> Debian release (not exactly sure which one) do not produce the
>>> FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.
>>>
>>
>> Thanks a lot Michael, that is really helpful.
> 
> I could not reproduce the issue with Debian Stretch's
> 2.02~beta3-5+deb9u2, so it does appear to be RedHat's value add that
> is to blame here.
> 
> @Laszlo: TL;DR RedHat's GRUB for arm64 appears to clobber the
> image_base field of the efi_loaded_image struct passed to the kernel.
> Could you please recommend a way to report this?

Yes. I seem to recall that you already have an account at
<https://bugzilla.redhat.com/>. Please log in, then go to the following
link:

https://bugzilla.redhat.com/enter_bug.cgi?product=Red%20Hat%20Enterprise%20Linux%208

In the "Component" field, please enter "grub2", then fill in Summary /
Description / etc.

Please be thorough, as if you wanted me to reproduce the issue :)

After filing the bug, please send the BZ link to me (or add me to the
bug's CC list), so I can ping some RH bootloader folks directly.

Thanks!
Laszlo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] efi/libstub/arm64: avoid image_base value from efi_loaded_image

[Ard Biesheuvel]

On Tue, 31 Mar 2020 at 21:20, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 03/31/20 09:56, Ard Biesheuvel wrote:
> > On Mon, 30 Mar 2020 at 20:24, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Mon, 30 Mar 2020 at 20:12, Michael Kelley <mikelley@microsoft.com> wrote:
> >>>
> >>> From: Ard Biesheuvel <ardb@kernel.org>  Sent: Monday, March 30, 2020 12:51 AM
> >>>>
> >>>> On Mon, 30 Mar 2020 at 09:50, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>>>
> >>>>> On Mon, 30 Mar 2020 at 09:47, Leif Lindholm <leif@nuviainc.com> wrote:
> >>>>>>
> >>>>>> On Sat, Mar 28, 2020 at 21:58:09 +0100, Ard Biesheuvel wrote:
> >>>>>>> Commit 9f9223778ef3 ("efi/libstub/arm: Make efi_entry() an ordinary
> >>>>>>> PE/COFF entrypoint") did some code refactoring to get rid of the
> >>>>>>> EFI entry point assembler code, and in the process, it got rid of the
> >>>>>>> assignment of image_addr to the value of _text. Instead, it switched
> >>>>>>> to using the image_base field of the efi_loaded_image struct provided
> >>>>>>> by UEFI, which should contain the same value.
> >>>>>>>
> >>>>>>> However, Michael reports that this is not the case: older GRUB builds
> >>>>>>> corrupt this value in some way, and since we can easily switch back to
> >>>>>>> referring to _text to discover this value, let's simply do that.
> >>>>>>
> >>>>>> It is not clear to me how "older GRUB builds" would differ here.
> >>>>>> I think more investigation is needed before making that claim.
> >>>>>> My suspicion is that some (old) version of non-upstream, shim-enabled
> >>>>>> distro-specific build is playing a part.
> >>>>>>
> >>>>>> So, do we have the option for more detailed investigations, or can we
> >>>>>> vague the claim up to say "some GRUB builds seen in the wild, based
> >>>>>> on an upstream 2.02" or suchlike?
> >>>>>>
> >>>>>
> >>>>> I've queued a fix that prints a nastygram if the value deviates from
> >>>>> the expected one. Let's see if this triggers any reports.
> >>>>
> >>>> (/me looks at context)
> >>>>
> >>>> *This* is the fix that prints a nastygram.
> >>>
> >>> FWIW, I pulled the BOOTAA64.EFI and grubaa64.efi files from CentOS 7.6
> >>> and CentOS 8.0 binary packages and tested both in my Hyper-V VM.
> >>> Using strings | grep '2\.' to get version info, the CentOS 7.6 grubaa64.efi
> >>> shows:
> >>>
> >>>         User-Agent: GRUB 2.02~beta2
> >>>
> >>> The CentOS 8.0 grubaa64.efi shows:
> >>>
> >>>         User-Agent: GRUB 2.03
> >>>
> >>> Both versions produce the FIRMWARE BUG warning when using Ard's
> >>> latest patch.  I'll assume the equivalent RHEL versions are the same.
> >>> So we've got official distro releases that show the problem.
> >>>
> >>> As reported earlier, the BOOTAA64.EFI and grubaa64.efi from a
> >>> Debian release (not exactly sure which one) do not produce the
> >>> FIRMWARE BUG warning.  The grubaa64.efi reports as 2.04-4.
> >>>
> >>
> >> Thanks a lot Michael, that is really helpful.
> >
> > I could not reproduce the issue with Debian Stretch's
> > 2.02~beta3-5+deb9u2, so it does appear to be RedHat's value add that
> > is to blame here.
> >
> > @Laszlo: TL;DR RedHat's GRUB for arm64 appears to clobber the
> > image_base field of the efi_loaded_image struct passed to the kernel.
> > Could you please recommend a way to report this?
>
> Yes. I seem to recall that you already have an account at
> <https://bugzilla.redhat.com/>. Please log in, then go to the following
> link:
>
> https://bugzilla.redhat.com/enter_bug.cgi?product=Red%20Hat%20Enterprise%20Linux%208
>
> In the "Component" field, please enter "grub2", then fill in Summary /
> Description / etc.
>
> Please be thorough, as if you wanted me to reproduce the issue :)
>
> After filing the bug, please send the BZ link to me (or add me to the
> bug's CC list), so I can ping some RH bootloader folks directly.
>

https://bugzilla.redhat.com/show_bug.cgi?id=1819624

Thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel