diff mbox series

arm: Zero/Sign extends for CMSE security

Message ID 33b2e8aa-9aa6-48e3-acef-0bab99676595@arm.com
State Committed
Commit ad45086178d833254d66fab518b14234418f002b
Headers
Series arm: Zero/Sign extends for CMSE security |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed

Commit Message

Richard Ball April 24, 2024, 3:55 p.m. UTC 
  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.

Comments

Richard Earnshaw (lists) April 25, 2024, 10:01 a.m. UTC | #1 
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.
Torbjörn SVENSSON April 25, 2024, 11:47 a.m. UTC | #2 
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
Richard Ball April 25, 2024, 2:25 p.m. UTC | #3 
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
Torbjörn SVENSSON April 26, 2024, 8:39 a.m. UTC | #4 
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
Richard Earnshaw (lists) April 26, 2024, 9:19 a.m. UTC | #5 
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.
diff mbox series

Patch

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 0000000000000000000000000000000000000000..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 <arm_cmse.h>
+#include <stdbool.h>
+
+#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];
+}
+
+/*
+**__acle_se_shortUnsignSecureFunc:
+**	...
+**	uxth	r0, r0
+**	...
+*/
+__attribute__((cmse_nonsecure_entry)) char shortUnsignSecureFunc (unsigned short index) {
+    if (index >= ARRAY_SIZE)
+      return 0;
+    return array[index];
+}
+
+/*
+**__acle_se_shortSignSecureFunc:
+**	...
+**	sxth	r0, r0
+**	...
+*/
+__attribute__((cmse_nonsecure_entry)) char shortSignSecureFunc (signed short index) {
+    if (index >= ARRAY_SIZE)
+      return 0;
+    return array[index];
+}
+
+/*
+**__acle_se_enumSecureFunc:
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+__attribute__((cmse_nonsecure_entry)) char enumSecureFunc (enum offset index) {
+
+  // Compiler may optimize away bounds check as value is an unsigned char.
+
+  // According to AAPCS caller will zero extend to ensure value is < 256.
+
+  if (index >= ARRAY_SIZE)
+    return 0;
+  return array[index];
+
+}
+
+/*
+**__acle_se_boolSecureFunc:
+**	...
+**	uxtb	r0, r0
+**	...
+*/
+__attribute__((cmse_nonsecure_entry)) char boolSecureFunc (bool index) {
+
+  if (index >= ARRAY_SIZE)
+    return 0;
+  return array[index];
+
+}
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/arm/cmse/extend-return.c b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
new file mode 100644
index 0000000000000000000000000000000000000000..cf731ed33df7e6dc101320c1970016f01b14c59a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/cmse/extend-return.c
@@ -0,0 +1,92 @@ 
+/* { dg-do compile } */
+/* { dg-options "-mcmse" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+#include <arm_cmse.h>
+#include <stdbool.h>
+
+enum offset
+{
+    zero = 0,
+    one = 1,
+    two = 2
+};
+
+typedef unsigned char __attribute__ ((cmse_nonsecure_call)) ns_unsign_foo_t (void);
+typedef signed char __attribute__ ((cmse_nonsecure_call)) ns_sign_foo_t (void);
+typedef unsigned short __attribute__ ((cmse_nonsecure_call)) ns_short_unsign_foo_t (void);
+typedef signed short __attribute__ ((cmse_nonsecure_call)) ns_short_sign_foo_t (void);
+typedef enum offset __attribute__ ((cmse_nonsecure_call)) ns_enum_foo_t (void);
+typedef bool __attribute__ ((cmse_nonsecure_call)) ns_bool_foo_t (void);
+
+/*
+**unsignNonsecure0:
+**	...
+**	bl	__gnu_cmse_nonsecure_call
+**	uxtb	r0, r0
+**	...
+*/
+unsigned char unsignNonsecure0 (ns_unsign_foo_t * ns_foo_p)
+{
+  return ns_foo_p ();
+}
+
+/*
+**signNonsecure0:
+**	...
+**	bl	__gnu_cmse_nonsecure_call
+**	sxtb	r0, r0
+**	...
+*/
+signed char signNonsecure0 (ns_sign_foo_t * ns_foo_p)
+{
+  return ns_foo_p ();
+}
+
+/*
+**shortUnsignNonsecure0:
+**	...
+**	bl	__gnu_cmse_nonsecure_call
+**	uxth	r0, r0
+**	...
+*/
+unsigned short shortUnsignNonsecure0 (ns_short_unsign_foo_t * ns_foo_p)
+{
+  return ns_foo_p ();
+}
+
+/*
+**shortSignNonsecure0:
+**	...
+**	bl	__gnu_cmse_nonsecure_call
+**	sxth	r0, r0
+**	...
+*/
+signed short shortSignNonsecure0 (ns_short_sign_foo_t * ns_foo_p)
+{
+  return ns_foo_p ();
+}
+
+/*
+**enumNonsecure0:
+**	...
+**	bl	__gnu_cmse_nonsecure_call
+**	uxtb	r0, r0
+**	...
+*/
+unsigned char __attribute__((noipa)) enumNonsecure0 (ns_enum_foo_t * ns_foo_p)
+{
+  return ns_foo_p ();
+}
+
+/*
+**boolNonsecure0:
+**	...
+**	bl	__gnu_cmse_nonsecure_call
+**	uxtb	r0, r0
+**	...
+*/
+unsigned char boolNonsecure0 (ns_bool_foo_t * ns_foo_p)
+{
+  return ns_foo_p ();
+}
\ No newline at end of file