[PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
instead of MIN_KIMG_ALIGN to check the kernel image alignment,
which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).

This removes the not-that-useful min_kimg_align helper and instead
uses the appropriate aligment in the respective call sites:

efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
used when kaslr is on, and all other cases go into alignment
check code which always need to check (and enforce) MIN_KIMG_ALIGN

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
---
 drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index 7bf0a7acae5e..e264ff90ba03 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
 	return EFI_SUCCESS;
 }
 
-/*
- * Although relocatable kernels can fix up the misalignment with respect to
- * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
- * sync with those recorded in the vmlinux when kaslr is disabled but the
- * image required relocation anyway. Therefore retain 2M alignment unless
- * KASLR is in use.
- */
-static u64 min_kimg_align(void)
-{
-	return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
-}
-
 efi_status_t handle_kernel_image(unsigned long *image_addr,
 				 unsigned long *image_size,
 				 unsigned long *reserve_addr,
@@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 		/*
 		 * If KASLR is enabled, and we have some randomness available,
 		 * locate the kernel at a randomized offset in physical memory.
+		 *
+		 * In that case, we don't need to preserve the 2M alignment
 		 */
-		status = efi_random_alloc(*reserve_size, min_kimg_align(),
+		status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
 					  reserve_addr, phys_seed);
 	} else {
 		status = EFI_OUT_OF_RESOURCES;
 	}
 
 	if (status != EFI_SUCCESS) {
-		if (IS_ALIGNED((u64)_text, min_kimg_align())) {
+		/*
+		 * Although relocatable kernels can fix up the misalignment with respect to
+		 * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
+		 * sync with those recorded in the vmlinux when kaslr is disabled but the
+		 * image required relocation anyway. Therefore retain 2M alignment unless
+		 * KASLR is in use.
+		 */
+		if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
 			/*
 			 * Just execute from wherever we were loaded by the
 			 * UEFI PE/COFF loader if the alignment is suitable.
@@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 		}
 
 		status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
-						    ULONG_MAX, min_kimg_align());
+						    ULONG_MAX, MIN_KIMG_ALIGN);
 
 		if (status != EFI_SUCCESS) {
 			efi_err("Failed to relocate kernel\n");

[PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
instead of MIN_KIMG_ALIGN to check the kernel image alignment,
which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).

This removes the not-that-useful min_kimg_align helper and instead
uses the appropriate aligment in the respective call sites:

efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
used when kaslr is on, and all other cases go into alignment
check code which always need to check (and enforce) MIN_KIMG_ALIGN

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
---
 drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index 7bf0a7acae5e..e264ff90ba03 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
 	return EFI_SUCCESS;
 }
 
-/*
- * Although relocatable kernels can fix up the misalignment with respect to
- * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
- * sync with those recorded in the vmlinux when kaslr is disabled but the
- * image required relocation anyway. Therefore retain 2M alignment unless
- * KASLR is in use.
- */
-static u64 min_kimg_align(void)
-{
-	return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
-}
-
 efi_status_t handle_kernel_image(unsigned long *image_addr,
 				 unsigned long *image_size,
 				 unsigned long *reserve_addr,
@@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 		/*
 		 * If KASLR is enabled, and we have some randomness available,
 		 * locate the kernel at a randomized offset in physical memory.
+		 *
+		 * In that case, we don't need to preserve the 2M alignment
 		 */
-		status = efi_random_alloc(*reserve_size, min_kimg_align(),
+		status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
 					  reserve_addr, phys_seed);
 	} else {
 		status = EFI_OUT_OF_RESOURCES;
 	}
 
 	if (status != EFI_SUCCESS) {
-		if (IS_ALIGNED((u64)_text, min_kimg_align())) {
+		/*
+		 * Although relocatable kernels can fix up the misalignment with respect to
+		 * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
+		 * sync with those recorded in the vmlinux when kaslr is disabled but the
+		 * image required relocation anyway. Therefore retain 2M alignment unless
+		 * KASLR is in use.
+		 */
+		if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
 			/*
 			 * Just execute from wherever we were loaded by the
 			 * UEFI PE/COFF loader if the alignment is suitable.
@@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
 		}
 
 		status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
-						    ULONG_MAX, min_kimg_align());
+						    ULONG_MAX, MIN_KIMG_ALIGN);
 
 		if (status != EFI_SUCCESS) {
 			efi_err("Failed to relocate kernel\n");




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

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 13:14, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
> instead of MIN_KIMG_ALIGN to check the kernel image alignment,
> which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).
>

Why? Relocatable kernels can happily execute from any 64k aligned
address, and the PE/COFF header carries this value of 64k as the
minimum alignment.

> This removes the not-that-useful min_kimg_align helper and instead
> uses the appropriate aligment in the respective call sites:
>
> efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
> used when kaslr is on, and all other cases go into alignment
> check code which always need to check (and enforce) MIN_KIMG_ALIGN
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index 7bf0a7acae5e..e264ff90ba03 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
>         return EFI_SUCCESS;
>  }
>
> -/*
> - * Although relocatable kernels can fix up the misalignment with respect to
> - * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> - * sync with those recorded in the vmlinux when kaslr is disabled but the
> - * image required relocation anyway. Therefore retain 2M alignment unless
> - * KASLR is in use.
> - */
> -static u64 min_kimg_align(void)
> -{
> -       return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
> -}
> -
>  efi_status_t handle_kernel_image(unsigned long *image_addr,
>                                  unsigned long *image_size,
>                                  unsigned long *reserve_addr,
> @@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>                 /*
>                  * If KASLR is enabled, and we have some randomness available,
>                  * locate the kernel at a randomized offset in physical memory.
> +                *
> +                * In that case, we don't need to preserve the 2M alignment
>                  */
> -               status = efi_random_alloc(*reserve_size, min_kimg_align(),
> +               status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
>                                           reserve_addr, phys_seed);
>         } else {
>                 status = EFI_OUT_OF_RESOURCES;
>         }
>
>         if (status != EFI_SUCCESS) {
> -               if (IS_ALIGNED((u64)_text, min_kimg_align())) {
> +               /*
> +                * Although relocatable kernels can fix up the misalignment with respect to
> +                * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> +                * sync with those recorded in the vmlinux when kaslr is disabled but the
> +                * image required relocation anyway. Therefore retain 2M alignment unless
> +                * KASLR is in use.
> +                */
> +               if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
>                         /*
>                          * Just execute from wherever we were loaded by the
>                          * UEFI PE/COFF loader if the alignment is suitable.
> @@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>                 }
>
>                 status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> -                                                   ULONG_MAX, min_kimg_align());
> +                                                   ULONG_MAX, MIN_KIMG_ALIGN);
>
>                 if (status != EFI_SUCCESS) {
>                         efi_err("Failed to relocate kernel\n");
>
>
>

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 13:14, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
> instead of MIN_KIMG_ALIGN to check the kernel image alignment,
> which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).
>

Why? Relocatable kernels can happily execute from any 64k aligned
address, and the PE/COFF header carries this value of 64k as the
minimum alignment.

> This removes the not-that-useful min_kimg_align helper and instead
> uses the appropriate aligment in the respective call sites:
>
> efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
> used when kaslr is on, and all other cases go into alignment
> check code which always need to check (and enforce) MIN_KIMG_ALIGN
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index 7bf0a7acae5e..e264ff90ba03 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
>         return EFI_SUCCESS;
>  }
>
> -/*
> - * Although relocatable kernels can fix up the misalignment with respect to
> - * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> - * sync with those recorded in the vmlinux when kaslr is disabled but the
> - * image required relocation anyway. Therefore retain 2M alignment unless
> - * KASLR is in use.
> - */
> -static u64 min_kimg_align(void)
> -{
> -       return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
> -}
> -
>  efi_status_t handle_kernel_image(unsigned long *image_addr,
>                                  unsigned long *image_size,
>                                  unsigned long *reserve_addr,
> @@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>                 /*
>                  * If KASLR is enabled, and we have some randomness available,
>                  * locate the kernel at a randomized offset in physical memory.
> +                *
> +                * In that case, we don't need to preserve the 2M alignment
>                  */
> -               status = efi_random_alloc(*reserve_size, min_kimg_align(),
> +               status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
>                                           reserve_addr, phys_seed);
>         } else {
>                 status = EFI_OUT_OF_RESOURCES;
>         }
>
>         if (status != EFI_SUCCESS) {
> -               if (IS_ALIGNED((u64)_text, min_kimg_align())) {
> +               /*
> +                * Although relocatable kernels can fix up the misalignment with respect to
> +                * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> +                * sync with those recorded in the vmlinux when kaslr is disabled but the
> +                * image required relocation anyway. Therefore retain 2M alignment unless
> +                * KASLR is in use.
> +                */
> +               if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
>                         /*
>                          * Just execute from wherever we were loaded by the
>                          * UEFI PE/COFF loader if the alignment is suitable.
> @@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>                 }
>
>                 status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> -                                                   ULONG_MAX, min_kimg_align());
> +                                                   ULONG_MAX, MIN_KIMG_ALIGN);
>
>                 if (status != EFI_SUCCESS) {
>                         efi_err("Failed to relocate kernel\n");
>
>
>

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

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

On Tue, 2021-07-20 at 14:57 +0200, Ard Biesheuvel wrote:
> On Tue, 20 Jul 2021 at 13:14, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
> > instead of MIN_KIMG_ALIGN to check the kernel image alignment,
> > which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).
> > 
> 
> Why? Relocatable kernels can happily execute from any 64k aligned
> address, and the PE/COFF header carries this value of 64k as the
> minimum alignment.

This is not what I'm changing. If you look at the code before the
patch, it was *already* only allowing 64k alignment with kaslr enabled
(commit 7c116db24d94). There's even a big fat comment explaining why
though it could use more details.

In any case, the code isn't even testing for CONFIG_RELOCATABLE today,
it makes its decisions entirely based on kaslr and doesn't properly
handle the case where efi_random_alloc() fails.

This fixes it.

Cheers,
Ben.


> > This removes the not-that-useful min_kimg_align helper and instead
> > uses the appropriate aligment in the respective call sites:
> > 
> > efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
> > used when kaslr is on, and all other cases go into alignment
> > check code which always need to check (and enforce) MIN_KIMG_ALIGN
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
> > ---
> >  drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> > index 7bf0a7acae5e..e264ff90ba03 100644
> > --- a/drivers/firmware/efi/libstub/arm64-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> > @@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
> >         return EFI_SUCCESS;
> >  }
> > 
> > -/*
> > - * Although relocatable kernels can fix up the misalignment with respect to
> > - * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > - * sync with those recorded in the vmlinux when kaslr is disabled but the
> > - * image required relocation anyway. Therefore retain 2M alignment unless
> > - * KASLR is in use.
> > - */
> > -static u64 min_kimg_align(void)
> > -{
> > -       return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
> > -}
> > -
> >  efi_status_t handle_kernel_image(unsigned long *image_addr,
> >                                  unsigned long *image_size,
> >                                  unsigned long *reserve_addr,
> > @@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> >                 /*
> >                  * If KASLR is enabled, and we have some randomness available,
> >                  * locate the kernel at a randomized offset in physical memory.
> > +                *
> > +                * In that case, we don't need to preserve the 2M alignment
> >                  */
> > -               status = efi_random_alloc(*reserve_size, min_kimg_align(),
> > +               status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
> >                                           reserve_addr, phys_seed);
> >         } else {
> >                 status = EFI_OUT_OF_RESOURCES;
> >         }
> > 
> >         if (status != EFI_SUCCESS) {
> > -               if (IS_ALIGNED((u64)_text, min_kimg_align())) {
> > +               /*
> > +                * Although relocatable kernels can fix up the misalignment with respect to
> > +                * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > +                * sync with those recorded in the vmlinux when kaslr is disabled but the
> > +                * image required relocation anyway. Therefore retain 2M alignment unless
> > +                * KASLR is in use.
> > +                */
> > +               if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
> >                         /*
> >                          * Just execute from wherever we were loaded by the
> >                          * UEFI PE/COFF loader if the alignment is suitable.
> > @@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> >                 }
> > 
> >                 status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > -                                                   ULONG_MAX, min_kimg_align());
> > +                                                   ULONG_MAX, MIN_KIMG_ALIGN);
> > 
> >                 if (status != EFI_SUCCESS) {
> >                         efi_err("Failed to relocate kernel\n");
> > 
> > 
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

On Tue, 2021-07-20 at 14:57 +0200, Ard Biesheuvel wrote:
> On Tue, 20 Jul 2021 at 13:14, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
> > instead of MIN_KIMG_ALIGN to check the kernel image alignment,
> > which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).
> > 
> 
> Why? Relocatable kernels can happily execute from any 64k aligned
> address, and the PE/COFF header carries this value of 64k as the
> minimum alignment.

This is not what I'm changing. If you look at the code before the
patch, it was *already* only allowing 64k alignment with kaslr enabled
(commit 7c116db24d94). There's even a big fat comment explaining why
though it could use more details.

In any case, the code isn't even testing for CONFIG_RELOCATABLE today,
it makes its decisions entirely based on kaslr and doesn't properly
handle the case where efi_random_alloc() fails.

This fixes it.

Cheers,
Ben.


> > This removes the not-that-useful min_kimg_align helper and instead
> > uses the appropriate aligment in the respective call sites:
> > 
> > efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
> > used when kaslr is on, and all other cases go into alignment
> > check code which always need to check (and enforce) MIN_KIMG_ALIGN
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
> > ---
> >  drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> > index 7bf0a7acae5e..e264ff90ba03 100644
> > --- a/drivers/firmware/efi/libstub/arm64-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> > @@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
> >         return EFI_SUCCESS;
> >  }
> > 
> > -/*
> > - * Although relocatable kernels can fix up the misalignment with respect to
> > - * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > - * sync with those recorded in the vmlinux when kaslr is disabled but the
> > - * image required relocation anyway. Therefore retain 2M alignment unless
> > - * KASLR is in use.
> > - */
> > -static u64 min_kimg_align(void)
> > -{
> > -       return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
> > -}
> > -
> >  efi_status_t handle_kernel_image(unsigned long *image_addr,
> >                                  unsigned long *image_size,
> >                                  unsigned long *reserve_addr,
> > @@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> >                 /*
> >                  * If KASLR is enabled, and we have some randomness available,
> >                  * locate the kernel at a randomized offset in physical memory.
> > +                *
> > +                * In that case, we don't need to preserve the 2M alignment
> >                  */
> > -               status = efi_random_alloc(*reserve_size, min_kimg_align(),
> > +               status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
> >                                           reserve_addr, phys_seed);
> >         } else {
> >                 status = EFI_OUT_OF_RESOURCES;
> >         }
> > 
> >         if (status != EFI_SUCCESS) {
> > -               if (IS_ALIGNED((u64)_text, min_kimg_align())) {
> > +               /*
> > +                * Although relocatable kernels can fix up the misalignment with respect to
> > +                * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > +                * sync with those recorded in the vmlinux when kaslr is disabled but the
> > +                * image required relocation anyway. Therefore retain 2M alignment unless
> > +                * KASLR is in use.
> > +                */
> > +               if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
> >                         /*
> >                          * Just execute from wherever we were loaded by the
> >                          * UEFI PE/COFF loader if the alignment is suitable.
> > @@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> >                 }
> > 
> >                 status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > -                                                   ULONG_MAX, min_kimg_align());
> > +                                                   ULONG_MAX, MIN_KIMG_ALIGN);
> > 
> >                 if (status != EFI_SUCCESS) {
> >                         efi_err("Failed to relocate kernel\n");
> > 
> > 
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


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

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 15:10, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2021-07-20 at 14:57 +0200, Ard Biesheuvel wrote:
> > On Tue, 20 Jul 2021 at 13:14, Benjamin Herrenschmidt
> > <benh@kernel.crashing.org> wrote:
> > > If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
> > > instead of MIN_KIMG_ALIGN to check the kernel image alignment,
> > > which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).
> > >
> >
> > Why? Relocatable kernels can happily execute from any 64k aligned
> > address, and the PE/COFF header carries this value of 64k as the
> > minimum alignment.
>
> This is not what I'm changing. If you look at the code before the
> patch, it was *already* only allowing 64k alignment with kaslr enabled
> (commit 7c116db24d94). There's even a big fat comment explaining why
> though it could use more details.
>

But that only takes effect if efi_nokaslr is true, which is the
default if CONFIG_RANDOMIZE_BASE is not set

> In any case, the code isn't even testing for CONFIG_RELOCATABLE today,

No, it tests for CONFIG_RANDOMIZE_BASE, which implies CONFIG_RELOCATABLE.

> it makes its decisions entirely based on kaslr and doesn't properly
> handle the case where efi_random_alloc() fails.
>
> This fixes it.
>

You are replacing min_kimg_align() with MIN_KIMG_ALIGN in a place
where it could return either value: efi_nokaslr will be false by
default on relocatable kernels, in which case min_kimg_align() will
return EFI_KIMG_ALIGN, unless you specifically request KASLR to be
disabled.

The result is that relocatable kernels that would not require to be
moved will now be moved to a 2 MB aligned offset before booting them.

Similarly for the efi_allocate_pages_aligned() call: that call would
only request 64k alignment before on a relocatable kernel if booting
without randomization.



> > > This removes the not-that-useful min_kimg_align helper and instead
> > > uses the appropriate aligment in the respective call sites:
> > >
> > > efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
> > > used when kaslr is on, and all other cases go into alignment
> > > check code which always need to check (and enforce) MIN_KIMG_ALIGN
> > >
> > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
> > > ---
> > >  drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
> > >  1 file changed, 12 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> > > index 7bf0a7acae5e..e264ff90ba03 100644
> > > --- a/drivers/firmware/efi/libstub/arm64-stub.c
> > > +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> > > @@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
> > >         return EFI_SUCCESS;
> > >  }
> > >
> > > -/*
> > > - * Although relocatable kernels can fix up the misalignment with respect to
> > > - * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > > - * sync with those recorded in the vmlinux when kaslr is disabled but the
> > > - * image required relocation anyway. Therefore retain 2M alignment unless
> > > - * KASLR is in use.
> > > - */
> > > -static u64 min_kimg_align(void)
> > > -{
> > > -       return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
> > > -}
> > > -
> > >  efi_status_t handle_kernel_image(unsigned long *image_addr,
> > >                                  unsigned long *image_size,
> > >                                  unsigned long *reserve_addr,
> > > @@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > >                 /*
> > >                  * If KASLR is enabled, and we have some randomness available,
> > >                  * locate the kernel at a randomized offset in physical memory.
> > > +                *
> > > +                * In that case, we don't need to preserve the 2M alignment
> > >                  */
> > > -               status = efi_random_alloc(*reserve_size, min_kimg_align(),
> > > +               status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
> > >                                           reserve_addr, phys_seed);
> > >         } else {
> > >                 status = EFI_OUT_OF_RESOURCES;
> > >         }
> > >
> > >         if (status != EFI_SUCCESS) {
> > > -               if (IS_ALIGNED((u64)_text, min_kimg_align())) {
> > > +               /*
> > > +                * Although relocatable kernels can fix up the misalignment with respect to
> > > +                * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > > +                * sync with those recorded in the vmlinux when kaslr is disabled but the
> > > +                * image required relocation anyway. Therefore retain 2M alignment unless
> > > +                * KASLR is in use.
> > > +                */
> > > +               if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
> > >                         /*
> > >                          * Just execute from wherever we were loaded by the
> > >                          * UEFI PE/COFF loader if the alignment is suitable.
> > > @@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > >                 }
> > >
> > >                 status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > > -                                                   ULONG_MAX, min_kimg_align());
> > > +                                                   ULONG_MAX, MIN_KIMG_ALIGN);
> > >
> > >                 if (status != EFI_SUCCESS) {
> > >                         efi_err("Failed to relocate kernel\n");
> > >
> > >
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 15:10, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2021-07-20 at 14:57 +0200, Ard Biesheuvel wrote:
> > On Tue, 20 Jul 2021 at 13:14, Benjamin Herrenschmidt
> > <benh@kernel.crashing.org> wrote:
> > > If efi_random_alloc() fails, we still try to use EFI_KIMG_ALIGN
> > > instead of MIN_KIMG_ALIGN to check the kernel image alignment,
> > > which is incorrect, we need to fallback to MIN_KIMG_ALIGN (2M).
> > >
> >
> > Why? Relocatable kernels can happily execute from any 64k aligned
> > address, and the PE/COFF header carries this value of 64k as the
> > minimum alignment.
>
> This is not what I'm changing. If you look at the code before the
> patch, it was *already* only allowing 64k alignment with kaslr enabled
> (commit 7c116db24d94). There's even a big fat comment explaining why
> though it could use more details.
>

But that only takes effect if efi_nokaslr is true, which is the
default if CONFIG_RANDOMIZE_BASE is not set

> In any case, the code isn't even testing for CONFIG_RELOCATABLE today,

No, it tests for CONFIG_RANDOMIZE_BASE, which implies CONFIG_RELOCATABLE.

> it makes its decisions entirely based on kaslr and doesn't properly
> handle the case where efi_random_alloc() fails.
>
> This fixes it.
>

You are replacing min_kimg_align() with MIN_KIMG_ALIGN in a place
where it could return either value: efi_nokaslr will be false by
default on relocatable kernels, in which case min_kimg_align() will
return EFI_KIMG_ALIGN, unless you specifically request KASLR to be
disabled.

The result is that relocatable kernels that would not require to be
moved will now be moved to a 2 MB aligned offset before booting them.

Similarly for the efi_allocate_pages_aligned() call: that call would
only request 64k alignment before on a relocatable kernel if booting
without randomization.



> > > This removes the not-that-useful min_kimg_align helper and instead
> > > uses the appropriate aligment in the respective call sites:
> > >
> > > efi_random_alloc() always wants EFI_KIMG_ALIGN as this is only
> > > used when kaslr is on, and all other cases go into alignment
> > > check code which always need to check (and enforce) MIN_KIMG_ALIGN
> > >
> > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Fixes: 7c116db24d94 (efi/libstub/arm64: Retain 2MB kernel Image alignment if !KASLR)
> > > ---
> > >  drivers/firmware/efi/libstub/arm64-stub.c | 27 ++++++++++-------------
> > >  1 file changed, 12 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> > > index 7bf0a7acae5e..e264ff90ba03 100644
> > > --- a/drivers/firmware/efi/libstub/arm64-stub.c
> > > +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> > > @@ -34,18 +34,6 @@ efi_status_t check_platform_features(void)
> > >         return EFI_SUCCESS;
> > >  }
> > >
> > > -/*
> > > - * Although relocatable kernels can fix up the misalignment with respect to
> > > - * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > > - * sync with those recorded in the vmlinux when kaslr is disabled but the
> > > - * image required relocation anyway. Therefore retain 2M alignment unless
> > > - * KASLR is in use.
> > > - */
> > > -static u64 min_kimg_align(void)
> > > -{
> > > -       return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
> > > -}
> > > -
> > >  efi_status_t handle_kernel_image(unsigned long *image_addr,
> > >                                  unsigned long *image_size,
> > >                                  unsigned long *reserve_addr,
> > > @@ -84,15 +72,24 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > >                 /*
> > >                  * If KASLR is enabled, and we have some randomness available,
> > >                  * locate the kernel at a randomized offset in physical memory.
> > > +                *
> > > +                * In that case, we don't need to preserve the 2M alignment
> > >                  */
> > > -               status = efi_random_alloc(*reserve_size, min_kimg_align(),
> > > +               status = efi_random_alloc(*reserve_size, EFI_KIMG_ALIGN,
> > >                                           reserve_addr, phys_seed);
> > >         } else {
> > >                 status = EFI_OUT_OF_RESOURCES;
> > >         }
> > >
> > >         if (status != EFI_SUCCESS) {
> > > -               if (IS_ALIGNED((u64)_text, min_kimg_align())) {
> > > +               /*
> > > +                * Although relocatable kernels can fix up the misalignment with respect to
> > > +                * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
> > > +                * sync with those recorded in the vmlinux when kaslr is disabled but the
> > > +                * image required relocation anyway. Therefore retain 2M alignment unless
> > > +                * KASLR is in use.
> > > +                */
> > > +               if (IS_ALIGNED((u64)_text, MIN_KIMG_ALIGN)) {
> > >                         /*
> > >                          * Just execute from wherever we were loaded by the
> > >                          * UEFI PE/COFF loader if the alignment is suitable.
> > > @@ -103,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > >                 }
> > >
> > >                 status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > > -                                                   ULONG_MAX, min_kimg_align());
> > > +                                                   ULONG_MAX, MIN_KIMG_ALIGN);
> > >
> > >                 if (status != EFI_SUCCESS) {
> > >                         efi_err("Failed to relocate kernel\n");
> > >
> > >
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

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

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

On Tue, 2021-07-20 at 15:48 +0200, Ard Biesheuvel wrote:
> 
> You are replacing min_kimg_align() with MIN_KIMG_ALIGN in a place
> where it could return either value: efi_nokaslr will be false by
> default on relocatable kernels

Not exactly:

drivers/firmware/efi/libstub/efi-stub-helper.c:bool efi_nokaslr =
!IS_ENABLED(CONFIG_RANDOMIZE_BASE);

So if CONFIG_RANDOMIZE_BASE is off (KASLR disabled in the config),
efi_nokaslr is true.

If CONFIG_RANDOMIZE_BASE is on, then it depends on the command line
(and the availability of the RNG protocol).

None of this depends on CONFIG_RELOCATABLE which is indeed not entirely
orthogonal, but not particularily relevant in how the code is written
today.

> , in which case min_kimg_align() will
> return EFI_KIMG_ALIGN, unless you specifically request KASLR to be
> disabled.

Nope. See above. It will only be EFI_KIMG_ALIGN if
CONFIG_RANDOMIZE_BASE is on and KASLR isn't otherwise disabled.

> The result is that relocatable kernels that would not require to be
> moved will now be moved to a 2 MB aligned offset before booting them.
> 
> Similarly for the efi_allocate_pages_aligned() call: that call would
> only request 64k alignment before on a relocatable kernel if booting
> without randomization.

I'm not sure I'm following you here. If you look at the changelog for
commit 7c116db24d94, it pretty clearly says:

"Adjust the EFI stub for arm64 so that the minimum Image alignment is
2MB unless KASLR is in use."

Which is also pretty much what is spelled in the comment
above min_kimg_align() (which I moved but kept in my patch).

Basically, what you describe is what the code used to do afaik, but not
what it does since 7c116db24d94.

The current code (prior) to my patch is pretty clear, it uses 64k
alignment if KASLR is on, otherwise 2MB. So the big if (status !=
EFI_SUCCESS) statement with the alignment check & relocation is all
only meant to be used in the !KASLR case, which is always going to want
2MB (again based on the code as written today).

My patch simply ensures that this is also true when KASLR fails to
randomize the kernel address.

Cheers,
Ben.

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

On Tue, 2021-07-20 at 15:48 +0200, Ard Biesheuvel wrote:
> 
> You are replacing min_kimg_align() with MIN_KIMG_ALIGN in a place
> where it could return either value: efi_nokaslr will be false by
> default on relocatable kernels

Not exactly:

drivers/firmware/efi/libstub/efi-stub-helper.c:bool efi_nokaslr =
!IS_ENABLED(CONFIG_RANDOMIZE_BASE);

So if CONFIG_RANDOMIZE_BASE is off (KASLR disabled in the config),
efi_nokaslr is true.

If CONFIG_RANDOMIZE_BASE is on, then it depends on the command line
(and the availability of the RNG protocol).

None of this depends on CONFIG_RELOCATABLE which is indeed not entirely
orthogonal, but not particularily relevant in how the code is written
today.

> , in which case min_kimg_align() will
> return EFI_KIMG_ALIGN, unless you specifically request KASLR to be
> disabled.

Nope. See above. It will only be EFI_KIMG_ALIGN if
CONFIG_RANDOMIZE_BASE is on and KASLR isn't otherwise disabled.

> The result is that relocatable kernels that would not require to be
> moved will now be moved to a 2 MB aligned offset before booting them.
> 
> Similarly for the efi_allocate_pages_aligned() call: that call would
> only request 64k alignment before on a relocatable kernel if booting
> without randomization.

I'm not sure I'm following you here. If you look at the changelog for
commit 7c116db24d94, it pretty clearly says:

"Adjust the EFI stub for arm64 so that the minimum Image alignment is
2MB unless KASLR is in use."

Which is also pretty much what is spelled in the comment
above min_kimg_align() (which I moved but kept in my patch).

Basically, what you describe is what the code used to do afaik, but not
what it does since 7c116db24d94.

The current code (prior) to my patch is pretty clear, it uses 64k
alignment if KASLR is on, otherwise 2MB. So the big if (status !=
EFI_SUCCESS) statement with the alignment check & relocation is all
only meant to be used in the !KASLR case, which is always going to want
2MB (again based on the code as written today).

My patch simply ensures that this is also true when KASLR fails to
randomize the kernel address.

Cheers,
Ben.


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

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 16:04, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2021-07-20 at 15:48 +0200, Ard Biesheuvel wrote:
> >
> > You are replacing min_kimg_align() with MIN_KIMG_ALIGN in a place
> > where it could return either value: efi_nokaslr will be false by
> > default on relocatable kernels
>
> Not exactly:
>
> drivers/firmware/efi/libstub/efi-stub-helper.c:bool efi_nokaslr =
> !IS_ENABLED(CONFIG_RANDOMIZE_BASE);
>
> So if CONFIG_RANDOMIZE_BASE is off (KASLR disabled in the config),
> efi_nokaslr is true.
>
> If CONFIG_RANDOMIZE_BASE is on, then it depends on the command line
> (and the availability of the RNG protocol).
>
> None of this depends on CONFIG_RELOCATABLE which is indeed not entirely
> orthogonal, but not particularily relevant in how the code is written
> today.
>
> > , in which case min_kimg_align() will
> > return EFI_KIMG_ALIGN, unless you specifically request KASLR to be
> > disabled.
>
> Nope. See above. It will only be EFI_KIMG_ALIGN if
> CONFIG_RANDOMIZE_BASE is on and KASLR isn't otherwise disabled.
>
> > The result is that relocatable kernels that would not require to be
> > moved will now be moved to a 2 MB aligned offset before booting them.
> >
> > Similarly for the efi_allocate_pages_aligned() call: that call would
> > only request 64k alignment before on a relocatable kernel if booting
> > without randomization.
>
> I'm not sure I'm following you here. If you look at the changelog for
> commit 7c116db24d94, it pretty clearly says:
>
> "Adjust the EFI stub for arm64 so that the minimum Image alignment is
> 2MB unless KASLR is in use."
>
> Which is also pretty much what is spelled in the comment
> above min_kimg_align() (which I moved but kept in my patch).
>
> Basically, what you describe is what the code used to do afaik, but not
> what it does since 7c116db24d94.
>
> The current code (prior) to my patch is pretty clear, it uses 64k
> alignment if KASLR is on, otherwise 2MB. So the big if (status !=
> EFI_SUCCESS) statement with the alignment check & relocation is all
> only meant to be used in the !KASLR case, which is always going to want
> 2MB (again based on the code as written today).
>
> My patch simply ensures that this is also true when KASLR fails to
> randomize the kernel address.
>

Fair enough.

The history here is that passing nokaslr on the command line would
force 2M alignment even if KASLR was not enabled to begin with,
without affecting the alignment policy of KASLR capable kernels if
KASLR was not explicitly disabled, but not available on the platform.

I realize now that my commit d32de9130f6c7 has interfered with this:
efi_nokaslr is set to true there so the later code does not complain
about the EFI_RNG_PROTOCOL being unavailable, but it has the side
effect of affecting the alignment policy in the remainder of the
function.

So what I would prefer here is to capture efi_nokaslr at entry, and
use that to decide the alignment. That way, efi_nokaslr can be set to
true without affecting the subsequent allocation logic.

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 16:04, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2021-07-20 at 15:48 +0200, Ard Biesheuvel wrote:
> >
> > You are replacing min_kimg_align() with MIN_KIMG_ALIGN in a place
> > where it could return either value: efi_nokaslr will be false by
> > default on relocatable kernels
>
> Not exactly:
>
> drivers/firmware/efi/libstub/efi-stub-helper.c:bool efi_nokaslr =
> !IS_ENABLED(CONFIG_RANDOMIZE_BASE);
>
> So if CONFIG_RANDOMIZE_BASE is off (KASLR disabled in the config),
> efi_nokaslr is true.
>
> If CONFIG_RANDOMIZE_BASE is on, then it depends on the command line
> (and the availability of the RNG protocol).
>
> None of this depends on CONFIG_RELOCATABLE which is indeed not entirely
> orthogonal, but not particularily relevant in how the code is written
> today.
>
> > , in which case min_kimg_align() will
> > return EFI_KIMG_ALIGN, unless you specifically request KASLR to be
> > disabled.
>
> Nope. See above. It will only be EFI_KIMG_ALIGN if
> CONFIG_RANDOMIZE_BASE is on and KASLR isn't otherwise disabled.
>
> > The result is that relocatable kernels that would not require to be
> > moved will now be moved to a 2 MB aligned offset before booting them.
> >
> > Similarly for the efi_allocate_pages_aligned() call: that call would
> > only request 64k alignment before on a relocatable kernel if booting
> > without randomization.
>
> I'm not sure I'm following you here. If you look at the changelog for
> commit 7c116db24d94, it pretty clearly says:
>
> "Adjust the EFI stub for arm64 so that the minimum Image alignment is
> 2MB unless KASLR is in use."
>
> Which is also pretty much what is spelled in the comment
> above min_kimg_align() (which I moved but kept in my patch).
>
> Basically, what you describe is what the code used to do afaik, but not
> what it does since 7c116db24d94.
>
> The current code (prior) to my patch is pretty clear, it uses 64k
> alignment if KASLR is on, otherwise 2MB. So the big if (status !=
> EFI_SUCCESS) statement with the alignment check & relocation is all
> only meant to be used in the !KASLR case, which is always going to want
> 2MB (again based on the code as written today).
>
> My patch simply ensures that this is also true when KASLR fails to
> randomize the kernel address.
>

Fair enough.

The history here is that passing nokaslr on the command line would
force 2M alignment even if KASLR was not enabled to begin with,
without affecting the alignment policy of KASLR capable kernels if
KASLR was not explicitly disabled, but not available on the platform.

I realize now that my commit d32de9130f6c7 has interfered with this:
efi_nokaslr is set to true there so the later code does not complain
about the EFI_RNG_PROTOCOL being unavailable, but it has the side
effect of affecting the alignment policy in the remainder of the
function.

So what I would prefer here is to capture efi_nokaslr at entry, and
use that to decide the alignment. That way, efi_nokaslr can be set to
true without affecting the subsequent allocation logic.

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

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

On Tue, 2021-07-20 at 16:10 +0200, Ard Biesheuvel wrote:
> 
> > My patch simply ensures that this is also true when KASLR fails to
> > randomize the kernel address.
> > 
> 
> Fair enough.
> 
> The history here is that passing nokaslr on the command line would
> force 2M alignment even if KASLR was not enabled to begin with,
> without affecting the alignment policy of KASLR capable kernels if
> KASLR was not explicitly disabled, but not available on the platform.
> 
> I realize now that my commit d32de9130f6c7 has interfered with this:
> efi_nokaslr is set to true there so the later code does not complain
> about the EFI_RNG_PROTOCOL being unavailable, but it has the side
> effect of affecting the alignment policy in the remainder of the
> function.
> 
> So what I would prefer here is to capture efi_nokaslr at entry, and
> use that to decide the alignment. That way, efi_nokaslr can be set to
> true without affecting the subsequent allocation logic.

So interestingly, the bug I am trying to fix with this patch seems to
indicate that this doesn't work (though I would need to debug further
as to why) on my systems (EC2 c6g.metal instances).

IE, in my case what happens is:

 - kernel has CONFIG_RANDOMIZE_BASE and CONFIG_RELOCATABLE both enabled
 - RNG protocol exists, it gets a random seed, but due to the other
bug, efi_random_alloc() fails. It thus falls back to the "normal"
alignment check & relocation case (the big if (status != EFI_SUCCESS))
 - That alignment check uses 64K and not 2M however. In my case it
passes (_text is already 64K aligned) and we boot...
 - And nothing happens. IE, it dies somewhere in/after exit boot
services, I haven't had a chance to figure out in more details why,
those machines take 20mn to reboot.

So *something* is wrong when we stick to a 64K alignment and don't
randomize the kernel base.

Interestingly we don't set nokaslr in that case, so we should still be
hitting all the kaslr path in the main kernel. The problem could relate
to running from that initial address. I'll have to debug further, maybe
try a repro-case in qemu.

In the meantime, please apply patch 1 which solves the main issue and
I'll continue digging.

Note (in case this is relevant): This was all tested/debugged on 5.10.y

Cheers,
Ben.

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Benjamin Herrenschmidt]

On Tue, 2021-07-20 at 16:10 +0200, Ard Biesheuvel wrote:
> 
> > My patch simply ensures that this is also true when KASLR fails to
> > randomize the kernel address.
> > 
> 
> Fair enough.
> 
> The history here is that passing nokaslr on the command line would
> force 2M alignment even if KASLR was not enabled to begin with,
> without affecting the alignment policy of KASLR capable kernels if
> KASLR was not explicitly disabled, but not available on the platform.
> 
> I realize now that my commit d32de9130f6c7 has interfered with this:
> efi_nokaslr is set to true there so the later code does not complain
> about the EFI_RNG_PROTOCOL being unavailable, but it has the side
> effect of affecting the alignment policy in the remainder of the
> function.
> 
> So what I would prefer here is to capture efi_nokaslr at entry, and
> use that to decide the alignment. That way, efi_nokaslr can be set to
> true without affecting the subsequent allocation logic.

So interestingly, the bug I am trying to fix with this patch seems to
indicate that this doesn't work (though I would need to debug further
as to why) on my systems (EC2 c6g.metal instances).

IE, in my case what happens is:

 - kernel has CONFIG_RANDOMIZE_BASE and CONFIG_RELOCATABLE both enabled
 - RNG protocol exists, it gets a random seed, but due to the other
bug, efi_random_alloc() fails. It thus falls back to the "normal"
alignment check & relocation case (the big if (status != EFI_SUCCESS))
 - That alignment check uses 64K and not 2M however. In my case it
passes (_text is already 64K aligned) and we boot...
 - And nothing happens. IE, it dies somewhere in/after exit boot
services, I haven't had a chance to figure out in more details why,
those machines take 20mn to reboot.

So *something* is wrong when we stick to a 64K alignment and don't
randomize the kernel base.

Interestingly we don't set nokaslr in that case, so we should still be
hitting all the kaslr path in the main kernel. The problem could relate
to running from that initial address. I'll have to debug further, maybe
try a repro-case in qemu.

In the meantime, please apply patch 1 which solves the main issue and
I'll continue digging.

Note (in case this is relevant): This was all tested/debugged on 5.10.y

Cheers,
Ben.



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

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 16:25, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2021-07-20 at 16:10 +0200, Ard Biesheuvel wrote:
> >
> > > My patch simply ensures that this is also true when KASLR fails to
> > > randomize the kernel address.
> > >
> >
> > Fair enough.
> >
> > The history here is that passing nokaslr on the command line would
> > force 2M alignment even if KASLR was not enabled to begin with,
> > without affecting the alignment policy of KASLR capable kernels if
> > KASLR was not explicitly disabled, but not available on the platform.
> >
> > I realize now that my commit d32de9130f6c7 has interfered with this:
> > efi_nokaslr is set to true there so the later code does not complain
> > about the EFI_RNG_PROTOCOL being unavailable, but it has the side
> > effect of affecting the alignment policy in the remainder of the
> > function.
> >
> > So what I would prefer here is to capture efi_nokaslr at entry, and
> > use that to decide the alignment. That way, efi_nokaslr can be set to
> > true without affecting the subsequent allocation logic.
>
> So interestingly, the bug I am trying to fix with this patch seems to
> indicate that this doesn't work (though I would need to debug further
> as to why) on my systems (EC2 c6g.metal instances).
>
> IE, in my case what happens is:
>
>  - kernel has CONFIG_RANDOMIZE_BASE and CONFIG_RELOCATABLE both enabled
>  - RNG protocol exists, it gets a random seed, but due to the other
> bug, efi_random_alloc() fails. It thus falls back to the "normal"
> alignment check & relocation case (the big if (status != EFI_SUCCESS))
>  - That alignment check uses 64K and not 2M however. In my case it
> passes (_text is already 64K aligned) and we boot...
>  - And nothing happens. IE, it dies somewhere in/after exit boot
> services, I haven't had a chance to figure out in more details why,
> those machines take 20mn to reboot.
>
> So *something* is wrong when we stick to a 64K alignment and don't
> randomize the kernel base.
>

Interesting. Could this be related to caching? If EFI loads Image and
it remains in place, we boot the image that we are executing from, and
if your ExitBootServices() implementation plays tricks with
[non-architected] caches, this could cause problems, I suppose. Just
guessing ...


> Interestingly we don't set nokaslr in that case, so we should still be
> hitting all the kaslr path in the main kernel. The problem could relate
> to running from that initial address. I'll have to debug further, maybe
> try a repro-case in qemu.
>
> In the meantime, please apply patch 1 which solves the main issue and
> I'll continue digging.
>

OK

> Note (in case this is relevant): This was all tested/debugged on 5.10.y
>
> Cheers,
> Ben.
>
>

Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 16:25, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2021-07-20 at 16:10 +0200, Ard Biesheuvel wrote:
> >
> > > My patch simply ensures that this is also true when KASLR fails to
> > > randomize the kernel address.
> > >
> >
> > Fair enough.
> >
> > The history here is that passing nokaslr on the command line would
> > force 2M alignment even if KASLR was not enabled to begin with,
> > without affecting the alignment policy of KASLR capable kernels if
> > KASLR was not explicitly disabled, but not available on the platform.
> >
> > I realize now that my commit d32de9130f6c7 has interfered with this:
> > efi_nokaslr is set to true there so the later code does not complain
> > about the EFI_RNG_PROTOCOL being unavailable, but it has the side
> > effect of affecting the alignment policy in the remainder of the
> > function.
> >
> > So what I would prefer here is to capture efi_nokaslr at entry, and
> > use that to decide the alignment. That way, efi_nokaslr can be set to
> > true without affecting the subsequent allocation logic.
>
> So interestingly, the bug I am trying to fix with this patch seems to
> indicate that this doesn't work (though I would need to debug further
> as to why) on my systems (EC2 c6g.metal instances).
>
> IE, in my case what happens is:
>
>  - kernel has CONFIG_RANDOMIZE_BASE and CONFIG_RELOCATABLE both enabled
>  - RNG protocol exists, it gets a random seed, but due to the other
> bug, efi_random_alloc() fails. It thus falls back to the "normal"
> alignment check & relocation case (the big if (status != EFI_SUCCESS))
>  - That alignment check uses 64K and not 2M however. In my case it
> passes (_text is already 64K aligned) and we boot...
>  - And nothing happens. IE, it dies somewhere in/after exit boot
> services, I haven't had a chance to figure out in more details why,
> those machines take 20mn to reboot.
>
> So *something* is wrong when we stick to a 64K alignment and don't
> randomize the kernel base.
>

Interesting. Could this be related to caching? If EFI loads Image and
it remains in place, we boot the image that we are executing from, and
if your ExitBootServices() implementation plays tricks with
[non-architected] caches, this could cause problems, I suppose. Just
guessing ...


> Interestingly we don't set nokaslr in that case, so we should still be
> hitting all the kaslr path in the main kernel. The problem could relate
> to running from that initial address. I'll have to debug further, maybe
> try a repro-case in qemu.
>
> In the meantime, please apply patch 1 which solves the main issue and
> I'll continue digging.
>

OK

> Note (in case this is relevant): This was all tested/debugged on 5.10.y
>
> Cheers,
> Ben.
>
>

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