diff mbox series

ARM: fix gcc-10 thumb2-kernel regression

Message ID 20210512081211.200025-1-arnd@kernel.org
State Superseded
Headers show
Series ARM: fix gcc-10 thumb2-kernel regression | expand

Commit Message

Arnd Bergmann May 12, 2021, 8:12 a.m. UTC
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>

---
 arch/arm/kernel/setup.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

-- 
2.29.2

Comments

Ard Biesheuvel May 12, 2021, 12:38 p.m. UTC | #1
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

>
Daniel Thompson May 13, 2021, 11:07 a.m. UTC | #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.
Linus Walleij May 17, 2021, 10:18 a.m. UTC | #3
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 mbox series

Patch

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
 }