efi/libstub: arm/arm64: don't use TASK_SIZE when randomising the RT space

PATCH New Ard Biesheuvel 4 participants 4 messages Apr 10, 2017 EFI

Share & tools

Ard Biesheuvel

author

2017-04-10 10:30 UTC #0 lore raw

PATCH 1/1 efi/libstub: arm/arm64: don't use TASK_SIZE when randomising the RT space

As reported by James, Catalin and Mark, commit e69176d68d26 ("ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region") results in a crash in the firmware regardless of whether KASLR is in effect or not, and whether the firmware implements EFI_RNG_PROTOCOL or not.

Mark has identified the root cause to be the inappropriate use of TASK_SIZE in the stub, which arm64 defines as

  #define TASK_SIZE             (test_thread_flag(TIF_32BIT) ? \
                                TASK_SIZE_32 : TASK_SIZE_64)

and testing thread flags at this point results in the dereference of pointers in uninitialized structures.

So instead, introduce a preprocessor symbol EFI_RT_VIRTUAL_LIMIT and define it to TASK_SIZE_64 on arm64 and TASK_SIZE on ARM, both of which are compile time constants. Also, change the 'headroom' variable to static const to force an error if this changes in the future.

Cc James Morse <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---

Apologies for the breakage. On the systems I have tested, sp_el0 apparently pointed somewhere sane when I inadvertently dereferenced it, and the resulting addresses looked sufficiently random to me.

Ingo, once we have some confirmation that this makes the problem go away, could you please take this straight into efi/core? Thanks.

 drivers/firmware/efi/libstub/arm-stub.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/firmware/efi/libstub/arm-stub.c +9 -2

index 1e45ec51b094..34010ff3b77e 100644
@@ -32,6 +32,12 @@
 #define EFI_RT_VIRTUAL_BASE	SZ_512M
 #define EFI_RT_VIRTUAL_SIZE	SZ_512M
 
+#ifdef CONFIG_ARM64
+#define EFI_RT_VIRTUAL_LIMIT	TASK_SIZE_64
+#else
+#define EFI_RT_VIRTUAL_LIMIT	TASK_SIZE
+#endif
+
 static u64 virtmap_base = EFI_RT_VIRTUAL_BASE;
 
 efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,

@@ -236,8 +242,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 		 * shift of 21 bit positions into account when scaling
 		 * the headroom value using a 32-bit random value.
 		 */
-		u64 headroom = TASK_SIZE - EFI_RT_VIRTUAL_BASE -
-			       EFI_RT_VIRTUAL_SIZE;
+		static const u64 headroom = EFI_RT_VIRTUAL_LIMIT -
+					    EFI_RT_VIRTUAL_BASE -
+					    EFI_RT_VIRTUAL_SIZE;
 		u32 rnd;
 
 		status = efi_get_random_bytes(sys_table, sizeof(rnd),

Signature · 1 line

2.9.3

Mark Rutland

2017-04-10 10:44 UTC #1 lore raw

PATCH 1/1 efi/libstub: arm/arm64: don't use TASK_SIZE when randomising the RT space

Quoted 20 lines · Ard Biesheuvel

As reported by James, Catalin and Mark, commit e69176d68d26 ("ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region") results in a crash in the firmware regardless of whether KASLR is in effect or not, and whether the firmware implements EFI_RNG_PROTOCOL or not.
Mark has identified the root cause to be the inappropriate use of TASK_SIZE in the stub, which arm64 defines as
  #define TASK_SIZE             (test_thread_flag(TIF_32BIT) ? \
                                TASK_SIZE_32 : TASK_SIZE_64)
and testing thread flags at this point results in the dereference of pointers in uninitialized structures.
So instead, introduce a preprocessor symbol EFI_RT_VIRTUAL_LIMIT and define it to TASK_SIZE_64 on arm64 and TASK_SIZE on ARM, both of which are compile time constants. Also, change the 'headroom' variable to static const to force an error if this changes in the future.

Quoted 6 lines

Cc James Morse <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Matt Fleming <[email protected]>
Cc Ingo Molnar <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

With this patch applied atop of next-20170410, a defconfig arm64 kernel built with the Linaro 15.08 toolchain boots happily for me on Juno R1.

So FWIW:

Tested-by Mark Rutland <[email protected]>

Thanks,
Mark.

Quoted 44 lines

---

Ingo, once we have some confirmation that this makes the problem go away, could you please take this straight into efi/core? Thanks.

 drivers/firmware/efi/libstub/arm-stub.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/firmware/efi/libstub/arm-stub.c +9 -2

index 1e45ec51b094..34010ff3b77e 100644
@@ -32,6 +32,12 @@
 #define EFI_RT_VIRTUAL_BASE	SZ_512M
 #define EFI_RT_VIRTUAL_SIZE	SZ_512M
 
+#ifdef CONFIG_ARM64
+#define EFI_RT_VIRTUAL_LIMIT	TASK_SIZE_64
+#else
+#define EFI_RT_VIRTUAL_LIMIT	TASK_SIZE
+#endif
+
 static u64 virtmap_base = EFI_RT_VIRTUAL_BASE;
 
 efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,

@@ -236,8 +242,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 		 * shift of 21 bit positions into account when scaling
 		 * the headroom value using a 32-bit random value.
 		 */
-		u64 headroom = TASK_SIZE - EFI_RT_VIRTUAL_BASE -
-			       EFI_RT_VIRTUAL_SIZE;
+		static const u64 headroom = EFI_RT_VIRTUAL_LIMIT -
+					    EFI_RT_VIRTUAL_BASE -
+					    EFI_RT_VIRTUAL_SIZE;
 		u32 rnd;
 
 		status = efi_get_random_bytes(sys_table, sizeof(rnd),

Signature · 1 line

2.9.3

James Morse

2017-04-10 11:34 UTC #2 lore raw

PATCH 1/1 efi/libstub: arm/arm64: don't use TASK_SIZE when randomising the RT space

Hi Ard, Mark

Quoted 28 lines · Mark Rutland

Quoted 20 lines · Ard Biesheuvel
As reported by James, Catalin and Mark, commit e69176d68d26 ("ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region") results in a crash in the firmware regardless of whether KASLR is in effect or not, and whether the firmware implements EFI_RNG_PROTOCOL or not.
Mark has identified the root cause to be the inappropriate use of TASK_SIZE in the stub, which arm64 defines as
  #define TASK_SIZE             (test_thread_flag(TIF_32BIT) ? \
                                TASK_SIZE_32 : TASK_SIZE_64)
and testing thread flags at this point results in the dereference of pointers in uninitialized structures.
So instead, introduce a preprocessor symbol EFI_RT_VIRTUAL_LIMIT and define it to TASK_SIZE_64 on arm64 and TASK_SIZE on ARM, both of which are compile time constants. Also, change the 'headroom' variable to static const to force an error if this changes in the future.
Quoted 6 lines
Cc James Morse <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Matt Fleming <[email protected]>
Cc Ingo Molnar <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

Quoted 2 lines

With this patch applied atop of next-20170410, a defconfig arm64 kernel built with the Linaro 15.08 toolchain boots happily for me on Juno R1.

Likewise, this fixes the problem I was seeing on Seattle. If it's useful:

Tested-by James Morse <[email protected]>

Thanks for debugging that so quickly Mark!

James

Catalin Marinas

2017-04-10 14:42 UTC #3 lore raw

PATCH 1/1 efi/libstub: arm/arm64: don't use TASK_SIZE when randomising the RT space

In reply to Ard Biesheuvel

Quoted 20 lines · Ard Biesheuvel

As reported by James, Catalin and Mark, commit e69176d68d26 ("ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region") results in a crash in the firmware regardless of whether KASLR is in effect or not, and whether the firmware implements EFI_RNG_PROTOCOL or not.
Mark has identified the root cause to be the inappropriate use of TASK_SIZE in the stub, which arm64 defines as
  #define TASK_SIZE             (test_thread_flag(TIF_32BIT) ? \
                                TASK_SIZE_32 : TASK_SIZE_64)
and testing thread flags at this point results in the dereference of pointers in uninitialized structures.
So instead, introduce a preprocessor symbol EFI_RT_VIRTUAL_LIMIT and define it to TASK_SIZE_64 on arm64 and TASK_SIZE on ARM, both of which are compile time constants. Also, change the 'headroom' variable to static const to force an error if this changes in the future.

Quoted 6 lines

Cc James Morse <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Matt Fleming <[email protected]>
Cc Ingo Molnar <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

It works for me as well:

Tested-by Catalin Marinas <[email protected]>

Thanks Ard and Mark.

Signature · 1 line

Catalin