On Wed, 8 Sept 2021 at 18:26, Andre Przywara <andre.przywara@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx/T/ > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> > Reported-by: Adam Lackorzynski <adam@xxxxxxxx> > Suggested-by: Peter Maydell <peter.maydell@xxxxxxxxxx> Thanks a lot for fixing this! I had no idea the encoding was invalid. Reviewed-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > --- > 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 >
