Re: [PATCH] arm: Zero/Sign extends for CMSE security on Armv8-M.baseline
Hi Torbjörn!
On Thu, 6 Jun 2024 at 18:47, Torbjörn SVENSSON
wrote:
>
> I would like to push this patch to the following branches:
>
> - releases/gcc-11
> - releases/gcc-12
> - releases/gcc-13
> - releases/gcc-14
> - trunk
>
> Ok?
>
> The problem was highlighted by https://linaro.atlassian.net/browse/GNU-1239
>
> --
>
> Properly handle zero and sign extension for Armv8-M.baseline as
> Cortex-M23 can have the security extension active.
> Currently, there is a internal compiler error on Cortex-M23 for the
> epilog processing of sign extension.
>
> This patch addresses the following CVE-2024-0151 for Armv8-M.baseline.
>
> gcc/ChangeLog:
>
> * config/arm/arm.cc (cmse_nonsecure_call_inline_register_clear):
> Sign extend for Thumb1.
> (thumb1_expand_prologue): Add zero/sign extend.
Quick nitpicking: I think the ICE you are fixing was reported as
https://linaro.atlassian.net/browse/GNU-1205
(GNU-1239 is about your test improvements failing too, in addition to
the existing ones)
and your patch is actually about fixing GCC bug report 115253.
So your commit title should end with "[PR115253]" (or maybe "PR target/115253")
and your ChangeLog should also contain "PR target/115253".
You can use contrib/git_check_commit.py to check your patch is
correctly formatted (otherwise it will be rejected by the commit hooks
anyway).
I haven't looked into the details of the patch yet :-)
Thanks for looking at this,
Christophe
>
> Signed-off-by: Torbjörn SVENSSON
> Co-authored-by: Yvan ROUX
> ---
> gcc/config/arm/arm.cc | 68 ++-
> 1 file changed, 60 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index ea0c963a4d6..077cb61f42a 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -19220,17 +19220,23 @@ cmse_nonsecure_call_inline_register_clear (void)
> || TREE_CODE (ret_type) == BOOLEAN_TYPE)
> && known_lt (GET_MODE_SIZE (TYPE_MODE (ret_type)), 4))
> {
> - machine_mode ret_mode = TYPE_MODE (ret_type);
> + rtx ret_mode = gen_rtx_REG (TYPE_MODE (ret_type), R0_REGNUM);
> + rtx si_mode = gen_rtx_REG (SImode, R0_REGNUM);
> rtx extend;
> if (TYPE_UNSIGNED (ret_type))
> - extend = gen_rtx_ZERO_EXTEND (SImode,
> - gen_rtx_REG (ret_mode,
> R0_REGNUM));
> + extend = gen_rtx_SET (si_mode, gen_rtx_ZERO_EXTEND (SImode,
> +
> ret_mode));
> + else if (TARGET_THUMB1)
> + {
> + if (known_lt (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))
> + extend = gen_thumb1_extendqisi2 (si_mode, ret_mode);
> + else
> + extend = gen_thumb1_extendhisi2 (si_mode, ret_mode);
> + }
> else
> - extend = gen_rtx_SIGN_EXTEND (SImode,
> - gen_rtx_REG (ret_mode,
> R0_REGNUM));
> - emit_insn_after (gen_rtx_SET (gen_rtx_REG (SImode, R0_REGNUM),
> -extend), insn);
> -
> + extend = gen_rtx_SET (si_mode, gen_rtx_SIGN_EXTEND (SImode,
> +
> ret_mode));
> + emit_insn_after (extend, insn);
> }
>
>
> @@ -27250,6 +27256,52 @@ thumb1_expand_prologue (void)
>live_regs_mask = offsets->saved_regs_mask;
>lr_needs_saving = live_regs_mask & (1 << LR_REGNUM);
>
> + /* The AAPCS requires the callee to widen integral types narrower
> + than 32 bits to the full width of the register; but when handling
> + calls to non-secure space, we cannot trust the callee to have
> + correctly done so. So forcibly re-widen the result here. */
> + if (IS_CMSE_ENTRY (func_type))
> +{
> + function_args_iterator args_iter;
> + CUMULATIVE_ARGS args_so_far_v;
> + cumulative_args_t args_so_far;
> + bool first_param = true;
> + tree arg_type;
> + tree fndecl = current_function_decl;
> + tree fntype = TREE_TYPE (fndecl);
> + arm_init_cumulative_args (&args_so_far_v, fntype, NULL_RTX, fndecl);
> + args_so_far = pack_cumulative_args (&args_so_far_v);
> + FOREACH_FUNCTION_ARGS (fntype, arg_type, args_iter)
> + {
> + rtx arg_rtx;
> +
> + if (VOID_TYPE_P (arg_type))
> + break;
> +
> + function_arg_info arg (arg_type, /*named=*/true);
> + if (!first_param)
> + /* We should advance after processing the argument and pass
> + the argument we're advancing past. */
> + arm_function_arg_advance (args_so_far, arg);
> + first_param = false;
> + arg_rtx = arm_function_arg (args_so_far, arg);
> + gcc_assert (REG_P (arg_rtx));
> + if ((
[PATCH] arm: Zero/Sign extends for CMSE security on Armv8-M.baseline
I would like to push this patch to the following branches:
- releases/gcc-11
- releases/gcc-12
- releases/gcc-13
- releases/gcc-14
- trunk
Ok?
The problem was highlighted by https://linaro.atlassian.net/browse/GNU-1239
--
Properly handle zero and sign extension for Armv8-M.baseline as
Cortex-M23 can have the security extension active.
Currently, there is a internal compiler error on Cortex-M23 for the
epilog processing of sign extension.
This patch addresses the following CVE-2024-0151 for Armv8-M.baseline.
gcc/ChangeLog:
* config/arm/arm.cc (cmse_nonsecure_call_inline_register_clear):
Sign extend for Thumb1.
(thumb1_expand_prologue): Add zero/sign extend.
Signed-off-by: Torbjörn SVENSSON
Co-authored-by: Yvan ROUX
---
gcc/config/arm/arm.cc | 68 ++-
1 file changed, 60 insertions(+), 8 deletions(-)
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index ea0c963a4d6..077cb61f42a 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -19220,17 +19220,23 @@ cmse_nonsecure_call_inline_register_clear (void)
|| TREE_CODE (ret_type) == BOOLEAN_TYPE)
&& known_lt (GET_MODE_SIZE (TYPE_MODE (ret_type)), 4))
{
- machine_mode ret_mode = TYPE_MODE (ret_type);
+ rtx ret_mode = gen_rtx_REG (TYPE_MODE (ret_type), R0_REGNUM);
+ rtx si_mode = gen_rtx_REG (SImode, R0_REGNUM);
rtx extend;
if (TYPE_UNSIGNED (ret_type))
- extend = gen_rtx_ZERO_EXTEND (SImode,
- gen_rtx_REG (ret_mode,
R0_REGNUM));
+ extend = gen_rtx_SET (si_mode, gen_rtx_ZERO_EXTEND (SImode,
+ ret_mode));
+ else if (TARGET_THUMB1)
+ {
+ if (known_lt (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))
+ extend = gen_thumb1_extendqisi2 (si_mode, ret_mode);
+ else
+ extend = gen_thumb1_extendhisi2 (si_mode, ret_mode);
+ }
else
- extend = gen_rtx_SIGN_EXTEND (SImode,
- gen_rtx_REG (ret_mode,
R0_REGNUM));
- emit_insn_after (gen_rtx_SET (gen_rtx_REG (SImode, R0_REGNUM),
-extend), insn);
-
+ extend = gen_rtx_SET (si_mode, gen_rtx_SIGN_EXTEND (SImode,
+ ret_mode));
+ emit_insn_after (extend, insn);
}
@@ -27250,6 +27256,52 @@ thumb1_expand_prologue (void)
live_regs_mask = offsets->saved_regs_mask;
lr_needs_saving = live_regs_mask & (1 << LR_REGNUM);
+ /* The AAPCS requires the callee to widen integral types narrower
+ than 32 bits to the full width of the register; but when handling
+ calls to non-secure space, we cannot trust the callee to have
+ correctly done so. So forcibly re-widen the result here. */
+ if (IS_CMSE_ENTRY (func_type))
+{
+ function_args_iterator args_iter;
+ CUMULATIVE_ARGS args_so_far_v;
+ cumulative_args_t args_so_far;
+ bool first_param = true;
+ tree arg_type;
+ tree fndecl = current_function_decl;
+ tree fntype = TREE_TYPE (fndecl);
+ arm_init_cumulative_args (&args_so_far_v, fntype, NULL_RTX, fndecl);
+ args_so_far = pack_cumulative_args (&args_so_far_v);
+ FOREACH_FUNCTION_ARGS (fntype, arg_type, args_iter)
+ {
+ rtx arg_rtx;
+
+ if (VOID_TYPE_P (arg_type))
+ break;
+
+ function_arg_info arg (arg_type, /*named=*/true);
+ if (!first_param)
+ /* We should advance after processing the argument and pass
+ the argument we're advancing past. */
+ arm_function_arg_advance (args_so_far, arg);
+ first_param = false;
+ arg_rtx = arm_function_arg (args_so_far, arg);
+ gcc_assert (REG_P (arg_rtx));
+ if ((TREE_CODE (arg_type) == INTEGER_TYPE
+ || TREE_CODE (arg_type) == ENUMERAL_TYPE
+ || TREE_CODE (arg_type) == BOOLEAN_TYPE)
+ && known_lt (GET_MODE_SIZE (GET_MODE (arg_rtx)), 4))
+ {
+ rtx res_reg = gen_rtx_REG (SImode, REGNO(arg_rtx));
+ if (TYPE_UNSIGNED (arg_type))
+ emit_set_insn (res_reg, gen_rtx_ZERO_EXTEND (SImode, arg_rtx));
+ else if (known_lt (GET_MODE_SIZE (GET_MODE (arg_rtx)), 2))
+ emit_insn (gen_thumb1_extendqisi2 (res_reg, arg_rtx));
+ else
+ emit_insn (gen_thumb1_extendhisi2 (res_reg, arg_rtx));
+ }
+ }
+}
+
/* Extract a mask of the ones we can give to the Thumb's push instruction.
*/
l_mask = live_regs_mask & 0x40ff;
/* Then count how many other high registers will need to be pushed. */
--
2.25.1
Re: [PATCH] arm: Zero/Sign extends for CMSE security
On 26/04/2024 09:39, Torbjorn SVENSSON wrote: > Hi, > > On 2024-04-25 16:25, Richard Ball wrote: >> Hi Torbjorn, >> >> Thanks very much for the comments. >> I think given that the code that handles this, is within a >> FOREACH_FUNCTION_ARGS loop. >> It seems a fairly safe assumption that if the code works for one that it >> will work for all. >> To go back and add extra tests to me seems a little overkill. > > For verifying that the implementation does the right thing now, no, but for > verifying against future regressions, then yes. > > So, from a regression point of view, I think it makes sense to have the check > that more than the first argument is managed properly. > > Kind regards, > Torbjörn Feel free to post some additional tests, Torbjorn. R.
Re: [PATCH] arm: Zero/Sign extends for CMSE security
Hi, On 2024-04-25 16:25, Richard Ball wrote: Hi Torbjorn, Thanks very much for the comments. I think given that the code that handles this, is within a FOREACH_FUNCTION_ARGS loop. It seems a fairly safe assumption that if the code works for one that it will work for all. To go back and add extra tests to me seems a little overkill. For verifying that the implementation does the right thing now, no, but for verifying against future regressions, then yes. So, from a regression point of view, I think it makes sense to have the check that more than the first argument is managed properly. Kind regards, Torbjörn
Re: [PATCH] arm: Zero/Sign extends for CMSE security
Hi Torbjorn, Thanks very much for the comments. I think given that the code that handles this, is within a FOREACH_FUNCTION_ARGS loop. It seems a fairly safe assumption that if the code works for one that it will work for all. To go back and add extra tests to me seems a little overkill. Kind Regards, Richard Ball From: Torbjorn SVENSSON Sent: 25 April 2024 12:47 To: Richard Ball ; gcc-patches@gcc.gnu.org ; Richard Earnshaw ; Richard Sandiford ; Marcus Shawcroft ; Kyrylo Tkachov Subject: Re: [PATCH] arm: Zero/Sign extends for CMSE security Hi, On 2024-04-24 17:55, Richard Ball wrote: > This patch makes the following changes: > > 1) When calling a secure function from non-secure code then any arguments > smaller than 32-bits that are passed in registers are zero- or > sign-extended. > 2) After a non-secure function returns into secure code then any return value > smaller than 32-bits that is passed in a register is zero- or > sign-extended. > > This patch addresses the following CVE-2024-0151. > > gcc/ChangeLog: > PR target/114837 > * config/arm/arm.cc (cmse_nonsecure_call_inline_register_clear): >Add zero/sign extend. > (arm_expand_prologue): Add zero/sign extend. > > gcc/testsuite/ChangeLog: > > * gcc.target/arm/cmse/extend-param.c: New test. > * gcc.target/arm/cmse/extend-return.c: New test. I think it would make sense that there is at least one test case that takes 2 or more arguments to ensure that not only the first argument is extended. WDYT? Kind regards, Torbjörn
Re: [PATCH] arm: Zero/Sign extends for CMSE security
Hi, On 2024-04-24 17:55, Richard Ball wrote: This patch makes the following changes: 1) When calling a secure function from non-secure code then any arguments smaller than 32-bits that are passed in registers are zero- or sign-extended. 2) After a non-secure function returns into secure code then any return value smaller than 32-bits that is passed in a register is zero- or sign-extended. This patch addresses the following CVE-2024-0151. gcc/ChangeLog: PR target/114837 * config/arm/arm.cc (cmse_nonsecure_call_inline_register_clear): Add zero/sign extend. (arm_expand_prologue): Add zero/sign extend. gcc/testsuite/ChangeLog: * gcc.target/arm/cmse/extend-param.c: New test. * gcc.target/arm/cmse/extend-return.c: New test. I think it would make sense that there is at least one test case that takes 2 or more arguments to ensure that not only the first argument is extended. WDYT? Kind regards, Torbjörn
Re: [PATCH] arm: Zero/Sign extends for CMSE security
On 24/04/2024 16:55, Richard Ball wrote: > This patch makes the following changes: > > 1) When calling a secure function from non-secure code then any arguments >smaller than 32-bits that are passed in registers are zero- or > sign-extended. > 2) After a non-secure function returns into secure code then any return value >smaller than 32-bits that is passed in a register is zero- or > sign-extended. > > This patch addresses the following CVE-2024-0151. > > gcc/ChangeLog: > PR target/114837 > * config/arm/arm.cc (cmse_nonsecure_call_inline_register_clear): > Add zero/sign extend. > (arm_expand_prologue): Add zero/sign extend. > > gcc/testsuite/ChangeLog: > > * gcc.target/arm/cmse/extend-param.c: New test. > * gcc.target/arm/cmse/extend-return.c: New test. OK. And OK to backport to active branches. R.
[PATCH] arm: Zero/Sign extends for CMSE security
This patch makes the following changes:
1) When calling a secure function from non-secure code then any arguments
smaller than 32-bits that are passed in registers are zero- or sign-extended.
2) After a non-secure function returns into secure code then any return value
smaller than 32-bits that is passed in a register is zero- or sign-extended.
This patch addresses the following CVE-2024-0151.
gcc/ChangeLog:
PR target/114837
* config/arm/arm.cc (cmse_nonsecure_call_inline_register_clear):
Add zero/sign extend.
(arm_expand_prologue): Add zero/sign extend.
gcc/testsuite/ChangeLog:
* gcc.target/arm/cmse/extend-param.c: New test.
* gcc.target/arm/cmse/extend-return.c: New test.diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index 0217abc218d60956ce727e6d008d46b9176dddc5..ea0c963a4d67ecd70e1571624e84dfe46d757df9 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -19210,6 +19210,30 @@ cmse_nonsecure_call_inline_register_clear (void)
end_sequence ();
emit_insn_before (seq, insn);
+ /* The AAPCS requires the callee to widen integral types narrower
+ than 32 bits to the full width of the register; but when handling
+ calls to non-secure space, we cannot trust the callee to have
+ correctly done so. So forcibly re-widen the result here. */
+ tree ret_type = TREE_TYPE (fntype);
+ if ((TREE_CODE (ret_type) == INTEGER_TYPE
+ || TREE_CODE (ret_type) == ENUMERAL_TYPE
+ || TREE_CODE (ret_type) == BOOLEAN_TYPE)
+ && known_lt (GET_MODE_SIZE (TYPE_MODE (ret_type)), 4))
+ {
+ machine_mode ret_mode = TYPE_MODE (ret_type);
+ rtx extend;
+ if (TYPE_UNSIGNED (ret_type))
+ extend = gen_rtx_ZERO_EXTEND (SImode,
+ gen_rtx_REG (ret_mode, R0_REGNUM));
+ else
+ extend = gen_rtx_SIGN_EXTEND (SImode,
+ gen_rtx_REG (ret_mode, R0_REGNUM));
+ emit_insn_after (gen_rtx_SET (gen_rtx_REG (SImode, R0_REGNUM),
+ extend), insn);
+
+ }
+
+
if (TARGET_HAVE_FPCXT_CMSE)
{
rtx_insn *last, *pop_insn, *after = insn;
@@ -23652,6 +23676,51 @@ arm_expand_prologue (void)
ip_rtx = gen_rtx_REG (SImode, IP_REGNUM);
+ /* The AAPCS requires the callee to widen integral types narrower
+ than 32 bits to the full width of the register; but when handling
+ calls to non-secure space, we cannot trust the callee to have
+ correctly done so. So forcibly re-widen the result here. */
+ if (IS_CMSE_ENTRY (func_type))
+{
+ function_args_iterator args_iter;
+ CUMULATIVE_ARGS args_so_far_v;
+ cumulative_args_t args_so_far;
+ bool first_param = true;
+ tree arg_type;
+ tree fndecl = current_function_decl;
+ tree fntype = TREE_TYPE (fndecl);
+ arm_init_cumulative_args (&args_so_far_v, fntype, NULL_RTX, fndecl);
+ args_so_far = pack_cumulative_args (&args_so_far_v);
+ FOREACH_FUNCTION_ARGS (fntype, arg_type, args_iter)
+ {
+ rtx arg_rtx;
+
+ if (VOID_TYPE_P (arg_type))
+ break;
+
+ function_arg_info arg (arg_type, /*named=*/true);
+ if (!first_param)
+ /* We should advance after processing the argument and pass
+ the argument we're advancing past. */
+ arm_function_arg_advance (args_so_far, arg);
+ first_param = false;
+ arg_rtx = arm_function_arg (args_so_far, arg);
+ gcc_assert (REG_P (arg_rtx));
+ if ((TREE_CODE (arg_type) == INTEGER_TYPE
+ || TREE_CODE (arg_type) == ENUMERAL_TYPE
+ || TREE_CODE (arg_type) == BOOLEAN_TYPE)
+ && known_lt (GET_MODE_SIZE (GET_MODE (arg_rtx)), 4))
+ {
+ if (TYPE_UNSIGNED (arg_type))
+ emit_set_insn (gen_rtx_REG (SImode, REGNO (arg_rtx)),
+ gen_rtx_ZERO_EXTEND (SImode, arg_rtx));
+ else
+ emit_set_insn (gen_rtx_REG (SImode, REGNO (arg_rtx)),
+ gen_rtx_SIGN_EXTEND (SImode, arg_rtx));
+ }
+ }
+}
+
if (IS_STACKALIGN (func_type))
{
rtx r0, r1;
diff --git a/gcc/testsuite/gcc.target/arm/cmse/extend-param.c b/gcc/testsuite/gcc.target/arm/cmse/extend-param.c
new file mode 100644
index ..01fac7862385f871f3ecc246ede95eea180be025
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/cmse/extend-param.c
@@ -0,0 +1,96 @@
+/* { dg-do compile } */
+/* { dg-options "-mcmse" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+#include
+#include
+
+#define ARRAY_SIZE (256)
+char array[ARRAY_SIZE];
+
+enum offset
+{
+zero = 0,
+one = 1,
+two = 2
+};
+
+/*
+**__acle_se_unsignSecureFunc:
+** ...
+** uxtb r0, r0
+** ...
+*/
+__attribute__((cmse_nonsecure_entry)) char unsignSecureFunc (unsigned char index) {
+if (index >= ARRAY_SIZE)
+ return 0;
+return array[index];
+}
+
+/*
+**__acle_se_signSecureFunc:
+** ...
+** sxtb r0, r0
+** ...
+*/
+__attribute__((cmse_nonsecure_entry)) char signSecureFunc (signed char index) {
+if (index >= ARRAY_SIZE)
+ return 0;
+return array[index]
