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

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

[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 <torbjorn.svensson@foss.st.com>
Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
---
 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 Armv8-M.baseline

[Christophe Lyon]

Hi Torbjörn!

On Thu, 6 Jun 2024 at 18:47, Torbjörn SVENSSON
<torbjorn.svensson@foss.st.com> 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 <torbjorn.svensson@foss.st.com>
> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
> ---
>  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
>

[PATH 0/2] arm: Zero/Sign extends for CMSE security on

[Torbjörn SVENSSON]

Hi,

Updated the patch to also fix the Cortex-M55 issue reported in PR115253 and
updated the commit message to mention the PR number.

Initial issue reported at https://linaro.atlassian.net/browse/GNU-1205.

Ok for these branches?

- releases/gcc-11
- releases/gcc-12
- releases/gcc-13
- releases/gcc-14
- trunk


Kind regards,
Torbjörn and Yvan

[PATCH v2 1/2] arm: Zero/Sign extends for CMSE security on Armv8-M.baseline [PR115253]

[Torbjörn SVENSSON]

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:

	PR target/115253
	* 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 <torbjorn.svensson@foss.st.com>
Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
---
 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..d1bb173c135 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

[PATCH v2 2/2] testsuite: Fix expand-return CMSE test for Armv8.1-M [PR115253]

[Torbjörn SVENSSON]

For Armv8.1-M, the clearing of the registers is handled differently than
for Armv8-M, so update the test case accordingly.

gcc/testsuite/ChangeLog:

	PR target/115253
	* gcc.target/arm/cmse/extend-return.c: Update test case
	condition for Armv8.1-M.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
---
 .../gcc.target/arm/cmse/extend-return.c       | 62 +++++++++++++++++--
 1 file changed, 56 insertions(+), 6 deletions(-)

diff --git a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
index 081de0d699f..2288d166bd3 100644
--- a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
+++ b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
@@ -1,5 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-mcmse -fshort-enums" } */
+/* ARMv8-M expectation with target { ! arm_cmse_clear_ok }.  */
+/* ARMv8.1-M expectation with target arm_cmse_clear_ok.  */
 /* { dg-final { check-function-bodies "**" "" "" } } */
 
 #include <arm_cmse.h>
@@ -20,7 +22,15 @@ typedef enum offset __attribute__ ((cmse_nonsecure_call)) ns_enum_foo_t (void);
 typedef bool __attribute__ ((cmse_nonsecure_call)) ns_bool_foo_t (void);
 
 /*
-**unsignNonsecure0:
+**unsignNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+/*
+**unsignNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxtb	r0, r0
@@ -32,7 +42,15 @@ unsigned char unsignNonsecure0 (ns_unsign_foo_t * ns_foo_p)
 }
 
 /*
-**signNonsecure0:
+**signNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	sxtb	r0, r0
+**	...
+*/
+/*
+**signNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	sxtb	r0, r0
@@ -44,7 +62,15 @@ signed char signNonsecure0 (ns_sign_foo_t * ns_foo_p)
 }
 
 /*
-**shortUnsignNonsecure0:
+**shortUnsignNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxth	r0, r0
+**	...
+*/
+/*
+**shortUnsignNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxth	r0, r0
@@ -56,7 +82,15 @@ unsigned short shortUnsignNonsecure0 (ns_short_unsign_foo_t * ns_foo_p)
 }
 
 /*
-**shortSignNonsecure0:
+**shortSignNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	sxth	r0, r0
+**	...
+*/
+/*
+**shortSignNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	sxth	r0, r0
@@ -68,7 +102,15 @@ signed short shortSignNonsecure0 (ns_short_sign_foo_t * ns_foo_p)
 }
 
 /*
-**enumNonsecure0:
+**enumNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+/*
+**enumNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxtb	r0, r0
@@ -80,7 +122,15 @@ unsigned char __attribute__((noipa)) enumNonsecure0 (ns_enum_foo_t * ns_foo_p)
 }
 
 /*
-**boolNonsecure0:
+**boolNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+/*
+**boolNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxtb	r0, r0
-- 
2.25.1

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

[Andre Vieira (lists)]

Hi Torbjorn,

Thanks for this, I have some comments below.

On 07/06/2024 09:56, Torbjörn SVENSSON wrote:
> 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:
> 
> 	PR target/115253
> 	* 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 <torbjorn.svensson@foss.st.com>
> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
> ---
>   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..d1bb173c135 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);

I'd rename ret_mode and si_mode to ret_reg and si_reg, so its clear they 
are registers and not actually mode types.

>   	      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);
>   	    }

Using gen_rtx_SIGN_EXTEND should work for both, the reason it doesn't is 
because of some weird code in thumb1_extendhisi2, which I'm actually 
gonna look at removing, but I don't think we should block this fix as 
we'd want to backport it ASAP.

But for clearness we should re-order this code so it's clear we only 
need it for that specific case.
Can you maybe do:
if (TYPE_UNSIGNED ..)
{
}
else
{
    /*  Signed-extension is a special case because of 
thumb1_extendhisi2.  */
    if (TARGET_THUMB1
        && known_gt (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))
      {
         //call the gen_thumb1_extendhisi2
      }
     else
      {
         // use gen_RTX_SIGN_EXTEND
      }
}
>   
>   
> @@ -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));
For consistency I'd probably do the same as above here:

if TYPE_UNSIGNED
else
   {
     special-case thumb1_extendhisi2
   }
> +	    }
> +	}
> +    }
> +
>     /* 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.  */

The rest LGTM, but I am not a maintainer You'll need an OK from Richard E.

In the meantime I'll test a patch to simplify thumb1_extendhisi2.

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

[Torbjorn SVENSSON]

Hi Andre,

Thanks for the review!
Please see my questions below.

On 2024-06-10 12:37, Andre Vieira (lists) wrote:
> Hi Torbjorn,
> 
> Thanks for this, I have some comments below.
> 
> On 07/06/2024 09:56, Torbjörn SVENSSON wrote:
>> 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:
>>
>>     PR target/115253
>>     * 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 <torbjorn.svensson@foss.st.com>
>> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
>> ---
>>   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..d1bb173c135 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);
> 
> I'd rename ret_mode and si_mode to ret_reg and si_reg, so its clear they 
> are registers and not actually mode types.

Okay, will be changed before push and/or a V3 of the patches.

>>             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);
>>           }
> 
> Using gen_rtx_SIGN_EXTEND should work for both, the reason it doesn't is 
> because of some weird code in thumb1_extendhisi2, which I'm actually 
> gonna look at removing, but I don't think we should block this fix as 
> we'd want to backport it ASAP.
> 
> But for clearness we should re-order this code so it's clear we only 
> need it for that specific case.
> Can you maybe do:
> if (TYPE_UNSIGNED ..)
> {
> }
> else
> {
>     /*  Signed-extension is a special case because of 
> thumb1_extendhisi2.  */
>     if (TARGET_THUMB1
>         && known_gt (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))
>       {
>          //call the gen_thumb1_extendhisi2
>       }
>      else
>       {
>          // use gen_RTX_SIGN_EXTEND
>       }
> }

So, you talk about gen_thumb1_extendhisi2, but there is also 
gen_thumb1_extendqisi2. Will it actually be cleaner if the block is 
indented one level?
The comment can be added in the "if (TARGET_THUMB1)" block regardless to 
indicate that gen_rtx_SIGN_EXTEND can't be used.

>> @@ -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));
> For consistency I'd probably do the same as above here:
> 
> if TYPE_UNSIGNED
> else
>    {
>      special-case thumb1_extendhisi2
>    }
>> +        }
>> +    }
>> +    }
>> +
>>     /* 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.  */
> 
> The rest LGTM, but I am not a maintainer You'll need an OK from Richard E.
> 
> In the meantime I'll test a patch to simplify thumb1_extendhisi2.

Whit the patch you have in mind, will it be possible to call 
gen_rtx_SIGN_EXTEND  for THUMB1 too? Or do we need to keep calling 
thumb1_extendhisi2 and thumb1_extendqisi2?

Kind regards,
Torbjörn

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

[Andre Vieira (lists)]

Hi,

> So, you talk about gen_thumb1_extendhisi2, but there is also 
> gen_thumb1_extendqisi2. Will it actually be cleaner if the block is 
> indented one level?
> The comment can be added in the "if (TARGET_THUMB1)" block regardless to 
> indicate that gen_rtx_SIGN_EXTEND can't be used.
> 

gen_rtx_SIGN_EXTEND (I see I used wrong caps above before sorry!) will 
work for the case of QImode -> SImode for Thumb1. The reason it doesn't 
work for HImode -> SImode in thumb1 is because thumb1_extendhisi2 uses a 
scratch register, see:
(define_insn "thumb1_extendhisi2"
   [(set (match_operand:SI 0 "register_operand" "=l,l")
         (sign_extend:SI (match_operand:HI 1 "nonimmediate_operand" "l,m")))
    (clobber (match_scratch:SI 2 "=X,l"))]

  meaning that the pattern generated with gen_rtx_SIGN_EXTEND:
[(set (SImode ...) (sign_extend:SImode (HImode))]
Does not match this and there are no other thumb1 patterns that match 
this either, so the compiler ICEs. For thumb1_extendqisi2 the pattern 
doesn't have the scratch register so a:
  gen_rtx_SET (<rtx:SImode>, gen_rtx_SIGN_EXTEND (SImode, <rtx:QImode>))
will generate a rtl pattern that will match thumb1_extendqisi2.

Hope that makes it clearer.

> 
> Whit the patch you have in mind, will it be possible to call 
> gen_rtx_SIGN_EXTEND  for THUMB1 too? Or do we need to keep calling 
> thumb1_extendhisi2 and thumb1_extendqisi2?

With patch I have in mind, and will post soon, you could use 
gen_rtx_SIGN_EXTEND for HImode in thumb1, like I explained before for 
QImode its already possible.

That series will have another patch that also removes all calls to 
gen_thumb1_extend* and replaces them with gen_rtx_SET + 
gen_rtx_SIGN_EXTEND and renames the "thumb1_extend{hisi2,qisi1} patterns 
to "*thumb1_extend{hisi2,qisi2}".

However, that patch we won't backport, but yours we should hence why 
it's worth having your patch with the thumb1_extendhisi2 workaround.

Thanks,

Andre

[PATCH v3 0/2] arm: Zero/Sign extends for CMSE security on Armv8-M.baseline [PR115253]

[Torbjörn SVENSSON]

Hi,

Changes in v3:
Droped special case for thumb1_extendqisi2 as it's only thumb1_extendhisi2 that
causes problem for gen_rtx_SIGN_EXTEND.

Changes in v2:
Updated the patch to also fix the Cortex-M55 issue reported in PR115253 and
updated the commit message to mention the PR number.

Initial issue reported at https://linaro.atlassian.net/browse/GNU-1205.

Ok for these branches?

- releases/gcc-11
- releases/gcc-12
- releases/gcc-13
- releases/gcc-14
- trunk

Kind regards,
Torbjörn and Yvan

[PATCH v3 1/2] arm: Zero/Sign extends for CMSE security on Armv8-M.baseline [PR115253]

[Torbjörn SVENSSON]

Properly handle zero and sign extension for Armv8-M.baseline as
Cortex-M23 can have the security extension active.
Currently, there is an 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:

	PR target/115253
	* 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 <torbjorn.svensson@foss.st.com>
Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
---
 gcc/config/arm/arm.cc | 71 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 8 deletions(-)

diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index ea0c963a4d6..e7b4caf1083 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -19220,17 +19220,22 @@ 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_reg = gen_rtx_REG (TYPE_MODE (ret_type), R0_REGNUM);
+	      rtx si_reg = 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_reg, gen_rtx_ZERO_EXTEND (SImode,
+								   ret_reg));
 	      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);
-
+		/* Signed-extension is a special case because of
+		   thumb1_extendhisi2.  */
+		if (TARGET_THUMB1
+		    && known_ge (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))
+		  extend = gen_thumb1_extendhisi2 (si_reg, ret_reg);
+		else
+		  extend = gen_rtx_SET (si_reg, gen_rtx_SIGN_EXTEND (SImode,
+								     ret_reg));
+	      emit_insn_after (extend, insn);
 	    }
 
 
@@ -27250,6 +27255,56 @@ 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
+		/* Signed-extension is a special case because of
+		   thumb1_extendhisi2.  */
+		if (known_ge (GET_MODE_SIZE (GET_MODE (arg_rtx)), 2))
+		  emit_insn (gen_thumb1_extendhisi2 (res_reg, arg_rtx));
+		else
+		  emit_set_insn (res_reg,
+				 gen_rtx_SIGN_EXTEND (SImode, 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

[PATCH v3 2/2] testsuite: Fix expand-return CMSE test for Armv8.1-M [PR115253]

[Torbjörn SVENSSON]

For Armv8.1-M, the clearing of the registers is handled differently than
for Armv8-M, so update the test case accordingly.

gcc/testsuite/ChangeLog:

	PR target/115253
	* gcc.target/arm/cmse/extend-return.c: Update test case
	condition for Armv8.1-M.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
---
 .../gcc.target/arm/cmse/extend-return.c       | 62 +++++++++++++++++--
 1 file changed, 56 insertions(+), 6 deletions(-)

diff --git a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
index 081de0d699f..2288d166bd3 100644
--- a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
+++ b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
@@ -1,5 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-mcmse -fshort-enums" } */
+/* ARMv8-M expectation with target { ! arm_cmse_clear_ok }.  */
+/* ARMv8.1-M expectation with target arm_cmse_clear_ok.  */
 /* { dg-final { check-function-bodies "**" "" "" } } */
 
 #include <arm_cmse.h>
@@ -20,7 +22,15 @@ typedef enum offset __attribute__ ((cmse_nonsecure_call)) ns_enum_foo_t (void);
 typedef bool __attribute__ ((cmse_nonsecure_call)) ns_bool_foo_t (void);
 
 /*
-**unsignNonsecure0:
+**unsignNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+/*
+**unsignNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxtb	r0, r0
@@ -32,7 +42,15 @@ unsigned char unsignNonsecure0 (ns_unsign_foo_t * ns_foo_p)
 }
 
 /*
-**signNonsecure0:
+**signNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	sxtb	r0, r0
+**	...
+*/
+/*
+**signNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	sxtb	r0, r0
@@ -44,7 +62,15 @@ signed char signNonsecure0 (ns_sign_foo_t * ns_foo_p)
 }
 
 /*
-**shortUnsignNonsecure0:
+**shortUnsignNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxth	r0, r0
+**	...
+*/
+/*
+**shortUnsignNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxth	r0, r0
@@ -56,7 +82,15 @@ unsigned short shortUnsignNonsecure0 (ns_short_unsign_foo_t * ns_foo_p)
 }
 
 /*
-**shortSignNonsecure0:
+**shortSignNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	sxth	r0, r0
+**	...
+*/
+/*
+**shortSignNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	sxth	r0, r0
@@ -68,7 +102,15 @@ signed short shortSignNonsecure0 (ns_short_sign_foo_t * ns_foo_p)
 }
 
 /*
-**enumNonsecure0:
+**enumNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+/*
+**enumNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxtb	r0, r0
@@ -80,7 +122,15 @@ unsigned char __attribute__((noipa)) enumNonsecure0 (ns_enum_foo_t * ns_foo_p)
 }
 
 /*
-**boolNonsecure0:
+**boolNonsecure0:  { target arm_cmse_clear_ok }
+**	...
+**	blxns	r[0-3]
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+/*
+**boolNonsecure0: { target { ! arm_cmse_clear_ok } }
 **	...
 **	bl	__gnu_cmse_nonsecure_call
 **	uxtb	r0, r0
-- 
2.25.1

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

[Richard Earnshaw (lists)]

On 10/06/2024 15:04, Torbjörn SVENSSON wrote:
> Properly handle zero and sign extension for Armv8-M.baseline as
> Cortex-M23 can have the security extension active.
> Currently, there is an 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:
> 
> 	PR target/115253
> 	* 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 <torbjorn.svensson@foss.st.com>
> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
> ---
>  gcc/config/arm/arm.cc | 71 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 8 deletions(-)
> 
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index ea0c963a4d6..e7b4caf1083 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -19220,17 +19220,22 @@ 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_reg = gen_rtx_REG (TYPE_MODE (ret_type), R0_REGNUM);
> +	      rtx si_reg = 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_reg, gen_rtx_ZERO_EXTEND (SImode,
> +								   ret_reg));
>  	      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);
> -
> +		/* Signed-extension is a special case because of
> +		   thumb1_extendhisi2.  */
> +		if (TARGET_THUMB1

You effectively have an 'else if' split across a comment here, and the indentation looks weird.  Either write 'else if' on one line (and re-indent accordingly) or put this entire block inside braces.

> +		    && known_ge (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))

You can use known_eq here.  We'll never have any value other than 2, given the known_le (4) above and anyway it doesn't make sense to call extendhisi with any other size.

> +		  extend = gen_thumb1_extendhisi2 (si_reg, ret_reg);
> +		else
> +		  extend = gen_rtx_SET (si_reg, gen_rtx_SIGN_EXTEND (SImode,
> +								     ret_reg));
> +	      emit_insn_after (extend, insn);
>  	    }
>  
>  
> @@ -27250,6 +27255,56 @@ thumb1_expand_prologue (void)
>    live_regs_mask = offsets->saved_regs_mask;
>    lr_needs_saving = live_regs_mask & (1 << LR_REGNUM);
>  

Similar comments to above apply to the hunk below.

> +  /* 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
> +		/* Signed-extension is a special case because of
> +		   thumb1_extendhisi2.  */
> +		if (known_ge (GET_MODE_SIZE (GET_MODE (arg_rtx)), 2))
> +		  emit_insn (gen_thumb1_extendhisi2 (res_reg, arg_rtx));
> +		else
> +		  emit_set_insn (res_reg,
> +				 gen_rtx_SIGN_EXTEND (SImode, 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.  */

OK with those changes.

R.

Re: [PATCH v3 2/2] testsuite: Fix expand-return CMSE test for Armv8.1-M [PR115253]

[Richard Earnshaw (lists)]

On 10/06/2024 15:04, Torbjörn SVENSSON wrote:
> For Armv8.1-M, the clearing of the registers is handled differently than
> for Armv8-M, so update the test case accordingly.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/115253
> 	* gcc.target/arm/cmse/extend-return.c: Update test case
> 	condition for Armv8.1-M.
> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
> ---
>  .../gcc.target/arm/cmse/extend-return.c       | 62 +++++++++++++++++--
>  1 file changed, 56 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
> index 081de0d699f..2288d166bd3 100644
> --- a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
> +++ b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
> @@ -1,5 +1,7 @@
>  /* { dg-do compile } */
>  /* { dg-options "-mcmse -fshort-enums" } */
> +/* ARMv8-M expectation with target { ! arm_cmse_clear_ok }.  */
> +/* ARMv8.1-M expectation with target arm_cmse_clear_ok.  */
>  /* { dg-final { check-function-bodies "**" "" "" } } */
>  
>  #include <arm_cmse.h>
> @@ -20,7 +22,15 @@ typedef enum offset __attribute__ ((cmse_nonsecure_call)) ns_enum_foo_t (void);
>  typedef bool __attribute__ ((cmse_nonsecure_call)) ns_bool_foo_t (void);
>  
>  /*
> -**unsignNonsecure0:
> +**unsignNonsecure0:  { target arm_cmse_clear_ok }
> +**	...
> +**	blxns	r[0-3]
> +**	...
> +**	uxtb	r0, r0
> +**	...
> +*/
> +/*
> +**unsignNonsecure0: { target { ! arm_cmse_clear_ok } }
>  **	...
>  **	bl	__gnu_cmse_nonsecure_call
>  **	uxtb	r0, r0
> @@ -32,7 +42,15 @@ unsigned char unsignNonsecure0 (ns_unsign_foo_t * ns_foo_p)
>  }
>  
>  /*
> -**signNonsecure0:
> +**signNonsecure0:  { target arm_cmse_clear_ok }
> +**	...
> +**	blxns	r[0-3]
> +**	...
> +**	sxtb	r0, r0
> +**	...
> +*/
> +/*
> +**signNonsecure0: { target { ! arm_cmse_clear_ok } }
>  **	...
>  **	bl	__gnu_cmse_nonsecure_call
>  **	sxtb	r0, r0
> @@ -44,7 +62,15 @@ signed char signNonsecure0 (ns_sign_foo_t * ns_foo_p)
>  }
>  
>  /*
> -**shortUnsignNonsecure0:
> +**shortUnsignNonsecure0:  { target arm_cmse_clear_ok }
> +**	...
> +**	blxns	r[0-3]
> +**	...
> +**	uxth	r0, r0
> +**	...
> +*/
> +/*
> +**shortUnsignNonsecure0: { target { ! arm_cmse_clear_ok } }
>  **	...
>  **	bl	__gnu_cmse_nonsecure_call
>  **	uxth	r0, r0
> @@ -56,7 +82,15 @@ unsigned short shortUnsignNonsecure0 (ns_short_unsign_foo_t * ns_foo_p)
>  }
>  
>  /*
> -**shortSignNonsecure0:
> +**shortSignNonsecure0:  { target arm_cmse_clear_ok }
> +**	...
> +**	blxns	r[0-3]
> +**	...
> +**	sxth	r0, r0
> +**	...
> +*/
> +/*
> +**shortSignNonsecure0: { target { ! arm_cmse_clear_ok } }
>  **	...
>  **	bl	__gnu_cmse_nonsecure_call
>  **	sxth	r0, r0
> @@ -68,7 +102,15 @@ signed short shortSignNonsecure0 (ns_short_sign_foo_t * ns_foo_p)
>  }
>  
>  /*
> -**enumNonsecure0:
> +**enumNonsecure0:  { target arm_cmse_clear_ok }
> +**	...
> +**	blxns	r[0-3]
> +**	...
> +**	uxtb	r0, r0
> +**	...
> +*/
> +/*
> +**enumNonsecure0: { target { ! arm_cmse_clear_ok } }
>  **	...
>  **	bl	__gnu_cmse_nonsecure_call
>  **	uxtb	r0, r0
> @@ -80,7 +122,15 @@ unsigned char __attribute__((noipa)) enumNonsecure0 (ns_enum_foo_t * ns_foo_p)
>  }
>  
>  /*
> -**boolNonsecure0:
> +**boolNonsecure0:  { target arm_cmse_clear_ok }
> +**	...
> +**	blxns	r[0-3]
> +**	...
> +**	uxtb	r0, r0
> +**	...
> +*/
> +/*
> +**boolNonsecure0: { target { ! arm_cmse_clear_ok } }
>  **	...
>  **	bl	__gnu_cmse_nonsecure_call
>  **	uxtb	r0, r0

OK when the nits in the first patch are sorted.

R.

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

[Andre Vieira (lists)]

On 11/06/2024 14:59, Richard Earnshaw (lists) wrote:
> You effectively have an 'else if' split across a comment here, and the indentation looks weird.  Either write 'else if' on one line (and re-indent accordingly) or put this entire block inside braces.

Apologies here, Torbjorn had this as an else if before, but I found that 
structure slightly more difficult to read because it wasn't entirely 
clear the else if and else were both for the 'SIGNED' handling, so I 
have a small preference for the 'or put this entire block inside braces'.

Kind Regards,
Andre

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

[Torbjorn SVENSSON]

On 2024-06-11 15:59, Richard Earnshaw (lists) wrote:
> On 10/06/2024 15:04, Torbjörn SVENSSON wrote:
>> Properly handle zero and sign extension for Armv8-M.baseline as
>> Cortex-M23 can have the security extension active.
>> Currently, there is an 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:
>>
>> 	PR target/115253
>> 	* 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 <torbjorn.svensson@foss.st.com>
>> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
>> ---
>>   gcc/config/arm/arm.cc | 71 ++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 63 insertions(+), 8 deletions(-)
>>
>> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
>> index ea0c963a4d6..e7b4caf1083 100644
>> --- a/gcc/config/arm/arm.cc
>> +++ b/gcc/config/arm/arm.cc
>> @@ -19220,17 +19220,22 @@ 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_reg = gen_rtx_REG (TYPE_MODE (ret_type), R0_REGNUM);
>> +	      rtx si_reg = 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_reg, gen_rtx_ZERO_EXTEND (SImode,
>> +								   ret_reg));
>>   	      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);
>> -
>> +		/* Signed-extension is a special case because of
>> +		   thumb1_extendhisi2.  */
>> +		if (TARGET_THUMB1
> 
> You effectively have an 'else if' split across a comment here, and the indentation looks weird.  Either write 'else if' on one line (and re-indent accordingly) or put this entire block inside braces.
> 
>> +		    && known_ge (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))
> 
> You can use known_eq here.  We'll never have any value other than 2, given the known_le (4) above and anyway it doesn't make sense to call extendhisi with any other size.
> 
>> +		  extend = gen_thumb1_extendhisi2 (si_reg, ret_reg);
>> +		else
>> +		  extend = gen_rtx_SET (si_reg, gen_rtx_SIGN_EXTEND (SImode,
>> +								     ret_reg));
>> +	      emit_insn_after (extend, insn);
>>   	    }
>>   
>>   
>> @@ -27250,6 +27255,56 @@ thumb1_expand_prologue (void)
>>     live_regs_mask = offsets->saved_regs_mask;
>>     lr_needs_saving = live_regs_mask & (1 << LR_REGNUM);
>>   
> 
> Similar comments to above apply to the hunk below.
> 
>> +  /* 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
>> +		/* Signed-extension is a special case because of
>> +		   thumb1_extendhisi2.  */
>> +		if (known_ge (GET_MODE_SIZE (GET_MODE (arg_rtx)), 2))
>> +		  emit_insn (gen_thumb1_extendhisi2 (res_reg, arg_rtx));
>> +		else
>> +		  emit_set_insn (res_reg,
>> +				 gen_rtx_SIGN_EXTEND (SImode, 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.  */
> 
> OK with those changes.
> 
> R.

Pushed as:
basepoints/gcc-15-1200-g65bd0655ece
releases/gcc-14.1.0-133-ga657148995e
releases/gcc-13.3.0-63-gbf3ffb44355
releases/gcc-12.3.0-1034-g55c1687d542
releases/gcc-11.4.0-649-g319081d614d

Kind regards,
Torbjörn

Re: [PATCH v3 2/2] testsuite: Fix expand-return CMSE test for Armv8.1-M [PR115253]

[Torbjorn SVENSSON]

On 2024-06-11 16:00, Richard Earnshaw (lists) wrote:
> On 10/06/2024 15:04, Torbjörn SVENSSON wrote:
>> For Armv8.1-M, the clearing of the registers is handled differently than
>> for Armv8-M, so update the test case accordingly.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR target/115253
>> 	* gcc.target/arm/cmse/extend-return.c: Update test case
>> 	condition for Armv8.1-M.
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
>> ---
>>   .../gcc.target/arm/cmse/extend-return.c       | 62 +++++++++++++++++--
>>   1 file changed, 56 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
>> index 081de0d699f..2288d166bd3 100644
>> --- a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
>> +++ b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
>> @@ -1,5 +1,7 @@
>>   /* { dg-do compile } */
>>   /* { dg-options "-mcmse -fshort-enums" } */
>> +/* ARMv8-M expectation with target { ! arm_cmse_clear_ok }.  */
>> +/* ARMv8.1-M expectation with target arm_cmse_clear_ok.  */
>>   /* { dg-final { check-function-bodies "**" "" "" } } */
>>   
>>   #include <arm_cmse.h>
>> @@ -20,7 +22,15 @@ typedef enum offset __attribute__ ((cmse_nonsecure_call)) ns_enum_foo_t (void);
>>   typedef bool __attribute__ ((cmse_nonsecure_call)) ns_bool_foo_t (void);
>>   
>>   /*
>> -**unsignNonsecure0:
>> +**unsignNonsecure0:  { target arm_cmse_clear_ok }
>> +**	...
>> +**	blxns	r[0-3]
>> +**	...
>> +**	uxtb	r0, r0
>> +**	...
>> +*/
>> +/*
>> +**unsignNonsecure0: { target { ! arm_cmse_clear_ok } }
>>   **	...
>>   **	bl	__gnu_cmse_nonsecure_call
>>   **	uxtb	r0, r0
>> @@ -32,7 +42,15 @@ unsigned char unsignNonsecure0 (ns_unsign_foo_t * ns_foo_p)
>>   }
>>   
>>   /*
>> -**signNonsecure0:
>> +**signNonsecure0:  { target arm_cmse_clear_ok }
>> +**	...
>> +**	blxns	r[0-3]
>> +**	...
>> +**	sxtb	r0, r0
>> +**	...
>> +*/
>> +/*
>> +**signNonsecure0: { target { ! arm_cmse_clear_ok } }
>>   **	...
>>   **	bl	__gnu_cmse_nonsecure_call
>>   **	sxtb	r0, r0
>> @@ -44,7 +62,15 @@ signed char signNonsecure0 (ns_sign_foo_t * ns_foo_p)
>>   }
>>   
>>   /*
>> -**shortUnsignNonsecure0:
>> +**shortUnsignNonsecure0:  { target arm_cmse_clear_ok }
>> +**	...
>> +**	blxns	r[0-3]
>> +**	...
>> +**	uxth	r0, r0
>> +**	...
>> +*/
>> +/*
>> +**shortUnsignNonsecure0: { target { ! arm_cmse_clear_ok } }
>>   **	...
>>   **	bl	__gnu_cmse_nonsecure_call
>>   **	uxth	r0, r0
>> @@ -56,7 +82,15 @@ unsigned short shortUnsignNonsecure0 (ns_short_unsign_foo_t * ns_foo_p)
>>   }
>>   
>>   /*
>> -**shortSignNonsecure0:
>> +**shortSignNonsecure0:  { target arm_cmse_clear_ok }
>> +**	...
>> +**	blxns	r[0-3]
>> +**	...
>> +**	sxth	r0, r0
>> +**	...
>> +*/
>> +/*
>> +**shortSignNonsecure0: { target { ! arm_cmse_clear_ok } }
>>   **	...
>>   **	bl	__gnu_cmse_nonsecure_call
>>   **	sxth	r0, r0
>> @@ -68,7 +102,15 @@ signed short shortSignNonsecure0 (ns_short_sign_foo_t * ns_foo_p)
>>   }
>>   
>>   /*
>> -**enumNonsecure0:
>> +**enumNonsecure0:  { target arm_cmse_clear_ok }
>> +**	...
>> +**	blxns	r[0-3]
>> +**	...
>> +**	uxtb	r0, r0
>> +**	...
>> +*/
>> +/*
>> +**enumNonsecure0: { target { ! arm_cmse_clear_ok } }
>>   **	...
>>   **	bl	__gnu_cmse_nonsecure_call
>>   **	uxtb	r0, r0
>> @@ -80,7 +122,15 @@ unsigned char __attribute__((noipa)) enumNonsecure0 (ns_enum_foo_t * ns_foo_p)
>>   }
>>   
>>   /*
>> -**boolNonsecure0:
>> +**boolNonsecure0:  { target arm_cmse_clear_ok }
>> +**	...
>> +**	blxns	r[0-3]
>> +**	...
>> +**	uxtb	r0, r0
>> +**	...
>> +*/
>> +/*
>> +**boolNonsecure0: { target { ! arm_cmse_clear_ok } }
>>   **	...
>>   **	bl	__gnu_cmse_nonsecure_call
>>   **	uxtb	r0, r0
> 
> OK when the nits in the first patch are sorted.
> 
> R.

Pushed as:

basepoints/gcc-15-1201-gcf5f9171bae
releases/gcc-14.1.0-134-g9100e78ba28
releases/gcc-13.3.0-64-gdfab6851eb5
releases/gcc-12.3.0-1035-g3d9e4eedb6b
releases/gcc-11.4.0-650-gbf9c877c4c9

Kind regards,
Torbjörn

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

[Richard Sandiford]

"Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> writes:
> On 10/06/2024 15:04, Torbjörn SVENSSON wrote:
>> Properly handle zero and sign extension for Armv8-M.baseline as
>> Cortex-M23 can have the security extension active.
>> Currently, there is an 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:
>> 
>> 	PR target/115253
>> 	* 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 <torbjorn.svensson@foss.st.com>
>> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com>
>> ---
>>  gcc/config/arm/arm.cc | 71 ++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 63 insertions(+), 8 deletions(-)
>> 
>> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
>> index ea0c963a4d6..e7b4caf1083 100644
>> --- a/gcc/config/arm/arm.cc
>> +++ b/gcc/config/arm/arm.cc
>> [...]
>> +		    && known_ge (GET_MODE_SIZE (TYPE_MODE (ret_type)), 2))
>
> You can use known_eq here.  We'll never have any value other than 2, given the known_le (4) above and anyway it doesn't make sense to call extendhisi with any other size.

BTW, I'm surprised we need known_* in arm-specific code.  Is it actually
needed?  Or is this just a conditioned response? ;)  

Richard