Re: [PATCH] arm: Zero/Sign extends for CMSE security on Armv8-M.baseline

2024-06-06 Thread Christophe Lyon
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

2024-06-06 Thread Torbjörn SVENSSON
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

2024-04-26 Thread Richard Earnshaw (lists)
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

2024-04-26 Thread Torbjorn SVENSSON

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

2024-04-25 Thread Richard Ball
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

2024-04-25 Thread Torbjorn SVENSSON

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

2024-04-25 Thread Richard Earnshaw (lists)
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

2024-04-24 Thread Richard Ball
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]