From: Ard Biesheuvel <ardb@kernel.org> To: Andre Przywara <andre.przywara@arm.com> Cc: Russell King <linux@armlinux.org.uk>, linux-efi <linux-efi@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Adam Lackorzynski <adam@l4re.org>, Peter Maydell <peter.maydell@linaro.org> Subject: Re: [PATCH] ARM: decompressor: Avoid UNPREDICTABLE NOP encoding Date: Thu, 9 Sep 2021 12:48:00 +0200 [thread overview] Message-ID: <CAMj1kXGWtNuE1P2O7=EMg8-RkAn_XYZOxQr5J3iN1fohzyqW4A@mail.gmail.com> (raw) In-Reply-To: <20210908162617.104962-1-andre.przywara@arm.com> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Ard Biesheuvel <ardb@kernel.org> To: Andre Przywara <andre.przywara@arm.com> Cc: Russell King <linux@armlinux.org.uk>, linux-efi <linux-efi@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Adam Lackorzynski <adam@l4re.org>, Peter Maydell <peter.maydell@linaro.org> Subject: Re: [PATCH] ARM: decompressor: Avoid UNPREDICTABLE NOP encoding Date: Thu, 9 Sep 2021 12:48:00 +0200 [thread overview] Message-ID: <CAMj1kXGWtNuE1P2O7=EMg8-RkAn_XYZOxQr5J3iN1fohzyqW4A@mail.gmail.com> (raw) In-Reply-To: <20210908162617.104962-1-andre.przywara@arm.com> 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-09-09 10:48 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-08 16:26 [PATCH] ARM: decompressor: Avoid UNPREDICTABLE NOP encoding Andre Przywara 2021-09-08 16:26 ` Andre Przywara 2021-09-09 10:48 ` Ard Biesheuvel [this message] 2021-09-09 10:48 ` Ard Biesheuvel 2021-09-13 22:43 ` Linus Walleij 2021-09-13 22:43 ` Linus Walleij
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAMj1kXGWtNuE1P2O7=EMg8-RkAn_XYZOxQr5J3iN1fohzyqW4A@mail.gmail.com' \ --to=ardb@kernel.org \ --cc=adam@l4re.org \ --cc=andre.przywara@arm.com \ --cc=catalin.marinas@arm.com \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=peter.maydell@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.