Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address
On Mon, Oct 14, 2019 at 6:14 PM Borislav Petkov wrote: > > On Sat, Oct 12, 2019 at 11:44:21AM +0800, Kairui Song wrote: > > Currently, kernel fails to boot on some HyperV VMs when using EFI. > > And it's a potential issue on all platforms. > > > > It's caused a broken kernel relocation on EFI systems, when below three > > conditions are met: > > > > 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR) > >by the loader. > > 2. There isn't enough room to contain the kernel, starting from the > >default load address (eg. something else occupied part the region). > > 3. In the memmap provided by EFI firmware, there is a memory region > >starts below LOAD_PHYSICAL_ADDR, and suitable for containing the > >kernel. > > > > Efi stub will perform a kernel relocation when condition 1 is met. But > > due to condition 2, efi stub can't relocate kernel to the preferred > > address, so it fallback to query and alloc from EFI firmware for lowest > > Your spelling of "EFI" is like a random number generator in this > paragraph: "Efi", "efi" and "EFI". Can you please be more careful when > writing your commit messages? They're not some random text you hurriedly > jot down before sending the patch but a most important description of > why a change is being done. Sorry I just ignored the acronym usage problems, I did double check the text but didn't realize this is a problem... Will correct them. > > And if you don't see their importance now, just try doing some git > archeology, trying to understand why a change has been done in the past > and then encounter a commit message two-liner which doesn't say sh*t. > Then you'll start appreciating properly written commit messages. > > > usable memory region. > > > > It's incorrect to use the lowest memory address. In later stage, kernel > > will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address, > > but efi stub will end up relocating kernel below it. > > Why don't you simply explain what > choose_random_location()->find_random_virt_addr() does? That's the > problem you're solving, right? KASLR using LOAD_PHYSICAL_ADDR as the > minimum... > > > The later kernel decompressing code will forcefully correct the wrong > > kernel load location, > > ... or do you mean by that the dance in > arch/x86/boot/compressed/head_64.S where we move the kernel temporarily > to LOAD_PHYSICAL_ADDR for the decompression? The kernel move in arch/x86/boot/compressed/head_64.S is the problem I'm saying here. I thought it's a bad idea to include too much details about codes and details in the commit message, so tried to describe it without mentioning the implementation details. It's making things confusing indeed. I'll rethink about how the commit message should be composed... > > You can simply say that here... > OK, then I'll do so. Will update the commit message. -- Best Regards, Kairui Song
Re: [PATCH] efi: libstub/arm: account for firmware reserved memory at the base of RAM
On Mon, Oct 14, 2019 at 06:33:09PM +0200, Ard Biesheuvel wrote:
> The EFI stubloader for ARM starts out by allocating a 32 MB window
> at the base of RAM, in order to ensure that the decompressor (which
> blindly copies the uncompressed kernel into that window) does not
> overwrite other allocations that are made while running in the context
> of the EFI firmware.
>
> In some cases, (e.g., U-Boot running on the Raspberry Pi 2), this is
> causing boot failures because this initial allocation conflicts with
> a page of reserved memory at the base of RAM that contains the SMP spin
> tables and other pieces of firmware data and which was put there by
> the bootloader under the assumption that the TEXT_OFFSET window right
> below the kernel is only used partially during early boot, and will be
> left alone once the memory reservations are processed and taken into
> account.
>
> So let's permit reserved memory regions to exist in the region starting
> at the base of RAM, and ending at TEXT_OFFSET - 5 * PAGE_SIZE, which is
> the window below the kernel that is not touched by the early boot code.
>
> Cc: Guillaume Gardet
> Cc: Chester Lin
> Signed-off-by: Ard Biesheuvel
> ---
> drivers/firmware/efi/libstub/Makefile | 1 +
> drivers/firmware/efi/libstub/arm32-stub.c | 16 +---
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile
> b/drivers/firmware/efi/libstub/Makefile
> index 0460c7581220..ee0661ddb25b 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -52,6 +52,7 @@ lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o string.o
> random.o \
>
> lib-$(CONFIG_ARM)+= arm32-stub.o
> lib-$(CONFIG_ARM64) += arm64-stub.o
> +CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>
> #
> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c
> b/drivers/firmware/efi/libstub/arm32-stub.c
> index e8f7aefb6813..47aafeff3e01 100644
> --- a/drivers/firmware/efi/libstub/arm32-stub.c
> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> @@ -195,6 +195,7 @@ efi_status_t handle_kernel_image(efi_system_table_t
> *sys_table,
>unsigned long dram_base,
>efi_loaded_image_t *image)
> {
> + unsigned long kernel_base;
> efi_status_t status;
>
> /*
> @@ -204,9 +205,18 @@ efi_status_t handle_kernel_image(efi_system_table_t
> *sys_table,
>* loaded. These assumptions are made by the decompressor,
>* before any memory map is available.
>*/
> - dram_base = round_up(dram_base, SZ_128M);
> + kernel_base = round_up(dram_base, SZ_128M);
>
> - status = reserve_kernel_base(sys_table, dram_base, reserve_addr,
> + /*
> + * Note that some platforms (notably, the Raspberry Pi 2) put
> + * spin-tables and other pieces of firmware at the base of RAM,
> + * abusing the fact that the window of TEXT_OFFSET bytes at the
> + * base of the kernel image is only partially used at the moment.
> + * (Up to 5 pages are used for the swapper page table)
> + */
> + kernel_base += TEXT_OFFSET - 5 * PAGE_SIZE;
> +
> + status = reserve_kernel_base(sys_table, kernel_base, reserve_addr,
>reserve_size);
> if (status != EFI_SUCCESS) {
> pr_efi_err(sys_table, "Unable to allocate memory for
> uncompressed kernel.\n");
> @@ -220,7 +230,7 @@ efi_status_t handle_kernel_image(efi_system_table_t
> *sys_table,
> *image_size = image->image_size;
> status = efi_relocate_kernel(sys_table, image_addr, *image_size,
>*image_size,
> - dram_base + MAX_UNCOMP_KERNEL_SIZE, 0);
> + kernel_base + MAX_UNCOMP_KERNEL_SIZE, 0);
> if (status != EFI_SUCCESS) {
> pr_efi_err(sys_table, "Failed to relocate kernel.\n");
> efi_free(sys_table, *reserve_size, *reserve_addr);
Acked-by: Chester Lin
Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address
On Mon, Oct 14, 2019 at 11:21:11PM +0300, Jarkko Sakkinen wrote: > Was there a section in the patch submission documentation to point out > when people send patches with all the possible twists for an acronym? I don't think so. > This is giving me constantly gray hairs with TPM patches. Well, I'm slowly getting tired of repeating the same crap over and over again about how important it is to document one's changes and to write good commit messages. The most repeated answers I'm simply putting into canned reply templates because, well, saying it once or twice is not enough anymore. :-\ And yeah, I see your pain. Same here, actually. In the acronym case, I'd probably add a regex to my patch massaging script and convert those typos automatically and be done with it. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address
On Mon, Oct 14, 2019 at 12:14:19PM +0200, Borislav Petkov wrote: > Your spelling of "EFI" is like a random number generator in this > paragraph: "Efi", "efi" and "EFI". Can you please be more careful when > writing your commit messages? They're not some random text you hurriedly > jot down before sending the patch but a most important description of > why a change is being done. Was there a section in the patch submission documentation to point out when people send patches with all the possible twists for an acronym? This is giving me constantly gray hairs with TPM patches. /Jarkko
[PATCH] efi/tpm: return -EINVAL when determining tpm final events log size fails
Currently nothing checks the return value of efi_tpm_eventlog_init,
but in case that changes in the future make sure an error is
returned when it fails to determine the tpm final events log
size.
Cc: Ard Biesheuvel
Cc: Jarkko Sakkinen
Cc: linux-efi@vger.kernel.org
Cc: linux-integr...@vger.kernel.org
Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after
successful event log parsing")
Suggested-by: Dan Carpenter
Signed-off-by: Jerry Snitselaar
---
drivers/firmware/efi/tpm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index ebd7977653a8..31f9f0e369b9 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -88,6 +88,7 @@ int __init efi_tpm_eventlog_init(void)
if (tbl_size < 0) {
pr_err(FW_BUG "Failed to parse event in TPM Final Events
Log\n");
+ ret = -EINVAL;
goto out_calc;
}
--
2.23.0
[PATCH] efi: libstub/arm: account for firmware reserved memory at the base of RAM
The EFI stubloader for ARM starts out by allocating a 32 MB window
at the base of RAM, in order to ensure that the decompressor (which
blindly copies the uncompressed kernel into that window) does not
overwrite other allocations that are made while running in the context
of the EFI firmware.
In some cases, (e.g., U-Boot running on the Raspberry Pi 2), this is
causing boot failures because this initial allocation conflicts with
a page of reserved memory at the base of RAM that contains the SMP spin
tables and other pieces of firmware data and which was put there by
the bootloader under the assumption that the TEXT_OFFSET window right
below the kernel is only used partially during early boot, and will be
left alone once the memory reservations are processed and taken into
account.
So let's permit reserved memory regions to exist in the region starting
at the base of RAM, and ending at TEXT_OFFSET - 5 * PAGE_SIZE, which is
the window below the kernel that is not touched by the early boot code.
Cc: Guillaume Gardet
Cc: Chester Lin
Signed-off-by: Ard Biesheuvel
---
drivers/firmware/efi/libstub/Makefile | 1 +
drivers/firmware/efi/libstub/arm32-stub.c | 16 +---
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/efi/libstub/Makefile
b/drivers/firmware/efi/libstub/Makefile
index 0460c7581220..ee0661ddb25b 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -52,6 +52,7 @@ lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o string.o
random.o \
lib-$(CONFIG_ARM) += arm32-stub.o
lib-$(CONFIG_ARM64)+= arm64-stub.o
+CFLAGS_arm32-stub.o:= -DTEXT_OFFSET=$(TEXT_OFFSET)
CFLAGS_arm64-stub.o:= -DTEXT_OFFSET=$(TEXT_OFFSET)
#
diff --git a/drivers/firmware/efi/libstub/arm32-stub.c
b/drivers/firmware/efi/libstub/arm32-stub.c
index e8f7aefb6813..47aafeff3e01 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -195,6 +195,7 @@ efi_status_t handle_kernel_image(efi_system_table_t
*sys_table,
unsigned long dram_base,
efi_loaded_image_t *image)
{
+ unsigned long kernel_base;
efi_status_t status;
/*
@@ -204,9 +205,18 @@ efi_status_t handle_kernel_image(efi_system_table_t
*sys_table,
* loaded. These assumptions are made by the decompressor,
* before any memory map is available.
*/
- dram_base = round_up(dram_base, SZ_128M);
+ kernel_base = round_up(dram_base, SZ_128M);
- status = reserve_kernel_base(sys_table, dram_base, reserve_addr,
+ /*
+* Note that some platforms (notably, the Raspberry Pi 2) put
+* spin-tables and other pieces of firmware at the base of RAM,
+* abusing the fact that the window of TEXT_OFFSET bytes at the
+* base of the kernel image is only partially used at the moment.
+* (Up to 5 pages are used for the swapper page table)
+*/
+ kernel_base += TEXT_OFFSET - 5 * PAGE_SIZE;
+
+ status = reserve_kernel_base(sys_table, kernel_base, reserve_addr,
reserve_size);
if (status != EFI_SUCCESS) {
pr_efi_err(sys_table, "Unable to allocate memory for
uncompressed kernel.\n");
@@ -220,7 +230,7 @@ efi_status_t handle_kernel_image(efi_system_table_t
*sys_table,
*image_size = image->image_size;
status = efi_relocate_kernel(sys_table, image_addr, *image_size,
*image_size,
-dram_base + MAX_UNCOMP_KERNEL_SIZE, 0);
+kernel_base + MAX_UNCOMP_KERNEL_SIZE, 0);
if (status != EFI_SUCCESS) {
pr_efi_err(sys_table, "Failed to relocate kernel.\n");
efi_free(sys_table, *reserve_size, *reserve_addr);
--
2.20.1
Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address
On Sat, Oct 12, 2019 at 11:44:21AM +0800, Kairui Song wrote: > Currently, kernel fails to boot on some HyperV VMs when using EFI. > And it's a potential issue on all platforms. > > It's caused a broken kernel relocation on EFI systems, when below three > conditions are met: > > 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR) >by the loader. > 2. There isn't enough room to contain the kernel, starting from the >default load address (eg. something else occupied part the region). > 3. In the memmap provided by EFI firmware, there is a memory region >starts below LOAD_PHYSICAL_ADDR, and suitable for containing the >kernel. > > Efi stub will perform a kernel relocation when condition 1 is met. But > due to condition 2, efi stub can't relocate kernel to the preferred > address, so it fallback to query and alloc from EFI firmware for lowest Your spelling of "EFI" is like a random number generator in this paragraph: "Efi", "efi" and "EFI". Can you please be more careful when writing your commit messages? They're not some random text you hurriedly jot down before sending the patch but a most important description of why a change is being done. And if you don't see their importance now, just try doing some git archeology, trying to understand why a change has been done in the past and then encounter a commit message two-liner which doesn't say sh*t. Then you'll start appreciating properly written commit messages. > usable memory region. > > It's incorrect to use the lowest memory address. In later stage, kernel > will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address, > but efi stub will end up relocating kernel below it. Why don't you simply explain what choose_random_location()->find_random_virt_addr() does? That's the problem you're solving, right? KASLR using LOAD_PHYSICAL_ADDR as the minimum... > The later kernel decompressing code will forcefully correct the wrong > kernel load location, ... or do you mean by that the dance in arch/x86/boot/compressed/head_64.S where we move the kernel temporarily to LOAD_PHYSICAL_ADDR for the decompression? You can simply say that here... -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v1] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y
On Mon, Oct 14, 2019 at 08:49:51AM +0200, Ard Biesheuvel wrote: > > [EXTERNAL EMAIL] > > On Mon, 14 Oct 2019 at 08:41, Geert Uytterhoeven wrote: > > > > Hi Narendra, > > > > On Sun, Oct 13, 2019 at 8:57 PM wrote: > > > From: Narendra K > > > > > > For the EFI_RCI2_TABLE kconfig option, 'make oldconfig' asks the user > > > for input on platforms where the option may not be applicable. This patch > > > modifies the kconfig option to ask the user for input only when CONFIG_X86 > > > or CONFIG_COMPILE_TEST is set to y. > > > > > > Reported-by: Geert Uytterhoeven > > > Fix-suggested-by: Geert Uytterhoeven > > > > Suggested-by: ...? > > > > Indeed Hi Ard/Geert, Thank you for correcting this. Should the patch be resubmitted with the above change made ? > > > > Signed-off-by: Narendra K > > > > Thanks for your patch! > > > > Tested-by: Geert Uytterhoeven > > > > Thanks all. I'll get this queued as a fix. Thank you for review comments. -- With regards, Narendra K
Re: [PATCH v1] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y
On Mon, 14 Oct 2019 at 08:41, Geert Uytterhoeven wrote: > > Hi Narendra, > > On Sun, Oct 13, 2019 at 8:57 PM wrote: > > From: Narendra K > > > > For the EFI_RCI2_TABLE kconfig option, 'make oldconfig' asks the user > > for input on platforms where the option may not be applicable. This patch > > modifies the kconfig option to ask the user for input only when CONFIG_X86 > > or CONFIG_COMPILE_TEST is set to y. > > > > Reported-by: Geert Uytterhoeven > > Fix-suggested-by: Geert Uytterhoeven > > Suggested-by: ...? > Indeed > > Signed-off-by: Narendra K > > Thanks for your patch! > > Tested-by: Geert Uytterhoeven > Thanks all. I'll get this queued as a fix.
Re: [PATCH v1] Ask user input only when CONFIG_X86 or CONFIG_COMPILE_TEST is set to y
Hi Narendra,
On Sun, Oct 13, 2019 at 8:57 PM wrote:
> From: Narendra K
>
> For the EFI_RCI2_TABLE kconfig option, 'make oldconfig' asks the user
> for input on platforms where the option may not be applicable. This patch
> modifies the kconfig option to ask the user for input only when CONFIG_X86
> or CONFIG_COMPILE_TEST is set to y.
>
> Reported-by: Geert Uytterhoeven
> Fix-suggested-by: Geert Uytterhoeven
Suggested-by: ...?
> Signed-off-by: Narendra K
Thanks for your patch!
Tested-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
