| Message ID | 20210512081211.200025-1-arnd@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: fix gcc-10 thumb2-kernel regression | expand |
On Wed, 12 May 2021 at 10:13, Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > When building the kernel wtih gcc-10 or higher using the > CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y flag, the compiler picks a slightly > different set of registers for the inline assembly in cpu_init() that > subsequently results in a corrupt kernel stack as well as remaining in > FIQ mode. If a banked register is used for the last argument, the wrong > version of that register gets loaded into CPSR_c. When building in Arm > mode, the arguments are passed as immediate values and the bug cannot > happen. > > This got introduced when Daniel reworked the FIQ handling and was > technically always broken, but happened to work with both clang and gcc > before gcc-10 as long as they picked one of the lower registers. > This is probably an indication that still very few people build the > kernel in Thumb2 mode. > > Marek pointed out the problem on IRC, Arnd narrowed it down to this > inline assembly and Russell pinpointed the exact bug. > > Change the constraints to force the final mode switch to use a non-banked > register for the argument to ensure that the correct constant gets loaded. > Another alternative would be to always use registers for the constant > arguments to avoid the #ifdef that has now become more complex. > > Cc: <stable@vger.kernel.org> # v3.18+ > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Reported-by: Marek Vasut <marek.vasut@gmail.com> > Fixes: c0e7f7ee717e ("ARM: 8150/3: fiq: Replace default FIQ handler") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Nice bug! Acked-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm/kernel/setup.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index 1a5edf562e85..73ca7797b92f 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -545,9 +545,11 @@ void notrace cpu_init(void) > * In Thumb-2, msr with an immediate value is not allowed. > */ > #ifdef CONFIG_THUMB2_KERNEL > -#define PLC "r" > +#define PLC_l "l" > +#define PLC_r "r" > #else > -#define PLC "I" > +#define PLC_l "I" > +#define PLC_r "I" > #endif > > /* > @@ -569,15 +571,15 @@ void notrace cpu_init(void) > "msr cpsr_c, %9" > : > : "r" (stk), > - PLC (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), > + PLC_r (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), > "I" (offsetof(struct stack, irq[0])), > - PLC (PSR_F_BIT | PSR_I_BIT | ABT_MODE), > + PLC_r (PSR_F_BIT | PSR_I_BIT | ABT_MODE), > "I" (offsetof(struct stack, abt[0])), > - PLC (PSR_F_BIT | PSR_I_BIT | UND_MODE), > + PLC_r (PSR_F_BIT | PSR_I_BIT | UND_MODE), > "I" (offsetof(struct stack, und[0])), > - PLC (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), > + PLC_r (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), > "I" (offsetof(struct stack, fiq[0])), > - PLC (PSR_F_BIT | PSR_I_BIT | SVC_MODE) > + PLC_l (PSR_F_BIT | PSR_I_BIT | SVC_MODE) > : "r14"); > #endif > } > -- > 2.29.2 >
On Wed, May 12, 2021 at 02:38:36PM +0200, Ard Biesheuvel wrote: > On Wed, 12 May 2021 at 10:13, Arnd Bergmann <arnd@kernel.org> wrote: > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > When building the kernel wtih gcc-10 or higher using the > > CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y flag, the compiler picks a slightly > > different set of registers for the inline assembly in cpu_init() that > > subsequently results in a corrupt kernel stack as well as remaining in > > FIQ mode. If a banked register is used for the last argument, the wrong > > version of that register gets loaded into CPSR_c. When building in Arm > > mode, the arguments are passed as immediate values and the bug cannot > > happen. > > > > This got introduced when Daniel reworked the FIQ handling and was > > technically always broken, but happened to work with both clang and gcc > > before gcc-10 as long as they picked one of the lower registers. > > This is probably an indication that still very few people build the > > kernel in Thumb2 mode. > > > > Marek pointed out the problem on IRC, Arnd narrowed it down to this > > inline assembly and Russell pinpointed the exact bug. > > > > Change the constraints to force the final mode switch to use a non-banked > > register for the argument to ensure that the correct constant gets loaded. > > Another alternative would be to always use registers for the constant > > arguments to avoid the #ifdef that has now become more complex. > > > > Cc: <stable@vger.kernel.org> # v3.18+ > > Cc: Daniel Thompson <daniel.thompson@linaro.org> > > Reported-by: Marek Vasut <marek.vasut@gmail.com> > > Fixes: c0e7f7ee717e ("ARM: 8150/3: fiq: Replace default FIQ handler") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Nice bug! Indeed. Many thanks for those involved with the find and fix! Daniel.
On Wed, May 12, 2021 at 10:13 AM Arnd Bergmann <arnd@kernel.org> wrote: > From: Arnd Bergmann <arnd@arndb.de> > > When building the kernel wtih gcc-10 or higher using the > CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y flag, the compiler picks a slightly > different set of registers for the inline assembly in cpu_init() that > subsequently results in a corrupt kernel stack as well as remaining in > FIQ mode. If a banked register is used for the last argument, the wrong > version of that register gets loaded into CPSR_c. When building in Arm > mode, the arguments are passed as immediate values and the bug cannot > happen. > > This got introduced when Daniel reworked the FIQ handling and was > technically always broken, but happened to work with both clang and gcc > before gcc-10 as long as they picked one of the lower registers. > This is probably an indication that still very few people build the > kernel in Thumb2 mode. > > Marek pointed out the problem on IRC, Arnd narrowed it down to this > inline assembly and Russell pinpointed the exact bug. > > Change the constraints to force the final mode switch to use a non-banked > register for the argument to ensure that the correct constant gets loaded. > Another alternative would be to always use registers for the constant > arguments to avoid the #ifdef that has now become more complex. > > Cc: <stable@vger.kernel.org> # v3.18+ > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Reported-by: Marek Vasut <marek.vasut@gmail.com> > Fixes: c0e7f7ee717e ("ARM: 8150/3: fiq: Replace default FIQ handler") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Wow. Nice bug hunt here, hats off! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index 1a5edf562e85..73ca7797b92f 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -545,9 +545,11 @@ void notrace cpu_init(void) * In Thumb-2, msr with an immediate value is not allowed. */ #ifdef CONFIG_THUMB2_KERNEL -#define PLC "r" +#define PLC_l "l" +#define PLC_r "r" #else -#define PLC "I" +#define PLC_l "I" +#define PLC_r "I" #endif /* @@ -569,15 +571,15 @@ void notrace cpu_init(void) "msr cpsr_c, %9" : : "r" (stk), - PLC (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), "I" (offsetof(struct stack, irq[0])), - PLC (PSR_F_BIT | PSR_I_BIT | ABT_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | ABT_MODE), "I" (offsetof(struct stack, abt[0])), - PLC (PSR_F_BIT | PSR_I_BIT | UND_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | UND_MODE), "I" (offsetof(struct stack, und[0])), - PLC (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), "I" (offsetof(struct stack, fiq[0])), - PLC (PSR_F_BIT | PSR_I_BIT | SVC_MODE) + PLC_l (PSR_F_BIT | PSR_I_BIT | SVC_MODE) : "r14"); #endif }