| Message ID | 20240610140437.966245-2-torbjorn.svensson@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Zero/Sign extends for CMSE security on Armv8-M.baseline [PR115253] | expand |
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.
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
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
"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
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. */
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(-)