lkml.org 
[lkml]   [2021]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] ARM: decompressor: Avoid UNPREDICTABLE NOP encoding
On Wed, 8 Sept 2021 at 18:26, Andre Przywara <andre.przywara@arm.com> wrote:
>
> In the decompressor's head.S we need to start with an instruction that
> is some kind of NOP, but also mimics as the PE/COFF header, when the
> kernel is linked as an UEFI application. The clever solution here is
> "tstne r0, #0x4d000", which in the worst case just clobbers the
> condition flags, and bears the magic "MZ" signature in the lowest 16 bits.
>
> However the encoding used (0x13105a4d) is actually not valid, since bits
> [15:12] are supposed to be 0 (written as "(0)" in the ARM ARM).
> Violating this is UNPREDICTABLE, and *can* trigger an UNDEFINED
> exception. Common Cortex cores seem to ignore those bits, but QEMU
> chooses to trap, so the code goes fishing because of a missing exception
> handler at this point. We are just saved by the fact that commonly (with
> -kernel or when running from U-Boot) the "Z" bit is set, so the
> instruction is never executed. See [0] for more details.
>
> To make things more robust and avoid UNPREDICTABLE behaviour in the
> kernel code, lets replace this with a "two-instruction NOP":
> The first instruction is an exclusive OR, the effect of which the second
> instruction reverts. This does not leave any trace, neither in a
> register nor in the condition flags. Also it's a perfectly valid
> encoding. Kudos to Peter Maydell for coming up with this gem.
>
> [0] https://lore.kernel.org/qemu-devel/YTPIdbUCmwagL5%2FD@os.inf.tu-dresden.de/T/
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Adam Lackorzynski <adam@l4re.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>

Thanks a lot for fixing this! I had no idea the encoding was invalid.

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
> arch/arm/boot/compressed/efi-header.S | 22 ++++++++++++++--------
> arch/arm/boot/compressed/head.S | 3 ++-
> 2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
> index c0e7a745103e..230030c13085 100644
> --- a/arch/arm/boot/compressed/efi-header.S
> +++ b/arch/arm/boot/compressed/efi-header.S
> @@ -9,16 +9,22 @@
> #include <linux/sizes.h>
>
> .macro __nop
> -#ifdef CONFIG_EFI_STUB
> - @ This is almost but not quite a NOP, since it does clobber the
> - @ condition flags. But it is the best we can do for EFI, since
> - @ PE/COFF expects the magic string "MZ" at offset 0, while the
> - @ ARM/Linux boot protocol expects an executable instruction
> - @ there.
> - .inst MZ_MAGIC | (0x1310 << 16) @ tstne r0, #0x4d000
> -#else
> AR_CLASS( mov r0, r0 )
> M_CLASS( nop.w )
> + .endm
> +
> + .macro __initial_nops
> +#ifdef CONFIG_EFI_STUB
> + @ This is a two-instruction NOP, which happens to bear the
> + @ PE/COFF signature "MZ" in the first two bytes, so the kernel
> + @ is accepted as an EFI binary. Booting via the UEFI stub
> + @ will not execute those instructions, but the ARM/Linux
> + @ boot protocol does, so we need some NOPs here.
> + .inst MZ_MAGIC | (0xe225 << 16) @ eor r5, r5, 0x4d000
> + eor r5, r5, 0x4d000 @ undo previous insn
> +#else
> + __nop
> + __nop
> #endif
> .endm
>
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index b1cb1972361b..bf79f2f78d23 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -203,7 +203,8 @@ start:
> * were patching the initial instructions of the kernel, i.e
> * had started to exploit this "patch area".
> */
> - .rept 7
> + __initial_nops
> + .rept 5
> __nop
> .endr
> #ifndef CONFIG_THUMB2_KERNEL
> --
> 2.17.1
>

\
 
 \ /
  Last update: 2021-09-09 12:49    [W:0.377 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site