Re: [PATCH 2/4] rs6000: Emit ROP-protect instructions in prologue and epilogue

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Sun, Apr 25, 2021 at 08:50:16PM -0500, Bill Schmidt wrote:
> Insert the hashst and hashchk instructions when -mrop-protect has been
> selected.  The encrypted save slot for ROP mitigation is placed
> between the parameter save area and the alloca space (if any;
> otherwise the local variable space).
> 
> Note that ROP-mitigation instructions are currently only provided for
> the ELFv2 ABI.

The good thing about that is we will gain some experience with it before
we have to implement this for other ABIs, heh.

> 	(rs6000_emit_prologue): Assert if WORLD_SAVE used with
> 	-mrop-protect;

WORLD_SAVE is only used for Darwin, so this is pretty useless.  Use full
stops in changelogs please, not semicolons?

> 	* config/rs6000/rs6000.md (unspec): Add UNSPEC_HASHST[P] and
> 	UNSPEC_HASHCHK[P].
> 	(hashst): New define_insn.
> 	(hashstp): Likewise.
> 	(hashchk): Likewise.
> 	(hashchkp): Likewise.

> +  int rop_check_save_offset;	/* offset to save ROP check from initial SP */

Offset to store the hash, right?  It isn't a "Rop check" that is stored
there :-)

> +  if (TARGET_POWER10 && info->calls_p
> +      && DEFAULT_ABI == ABI_ELFv2 && rs6000_rop_protect)
> +    info->rop_check_size = 8;

One && per line please (if it doesn't fit on one line).  It of course
depends what is most readable, but in this case the four clauses are
all completely different, so each && should start a new line.

> +  else if (rs6000_rop_protect && DEFAULT_ABI != ABI_ELFv2)
> +    {
> +      /* We can't check this in rs6000_option_override_internal since
> +	 DEFAULT_ABI isn't established yet.  */
> +      error ("%qs requires the ELFv2 ABI", "-mrop-protect");
> +      info->rop_check_size = 0;
> +    }
> +  else
> +    info->rop_check_size = 0;

Hrm.  It would be nicer if you set rop_check_size to 0 before everything
else, and then overwrite it in the one case you need something else.

> @@ -806,8 +821,14 @@ rs6000_stack_info (void)
>  	  gcc_assert (info->altivec_size == 0
>  		      || info->altivec_save_offset % 16 == 0);
>  
> -	  /* Adjust for AltiVec case.  */
> -	  info->ehrd_offset = info->altivec_save_offset - ehrd_size;
> +	  if (info->rop_check_size != 0)
> +	    {
> +	      info->rop_check_save_offset
> +		= info->altivec_save_offset - info->rop_check_size;
> +	      info->ehrd_offset = info->rop_check_save_offset - ehrd_size;
> +	    }
> +	  else
> +	    info->ehrd_offset = info->altivec_save_offset - ehrd_size;
>  	}

Put the comment back here?  Also maybe easier if you do

	  /* Adjust for AltiVec case.  */
	  info->ehrd_offset = info->altivec_save_offset - ehrd_size;

	  /* Adjust for ROP protection.  */
	  if (info->rop_check_size != 0)
	    {
	      info->rop_check_save_offset
		= info->altivec_save_offset - info->rop_check_size;
	      info->ehrd_offset -= info->rop_check_size;
	    }

(or even make that unconditional, the "if" isn't needed anymore then).

> @@ -3044,7 +3073,6 @@ rs6000_emit_prologue (void)
>  	cfun->machine->r2_setup_needed = true;
>      }
>  
> -
>    if (flag_stack_usage_info)
>      current_function_static_stack_size = info->total_size;
>  

I don' think this improves the code, and it is an unrelated change, but
heh, *shrug*.

> +  /* The ROP hash store must occur before a stack frame is created.  */

Why?  (Explain in the code, not to me :-) )

> +  /* The ROP hash check must occur after the stack pointer is restored,
> +     and is not performed for a sibcall.  */

Same here.

> +   UNSPEC_HASHST
> +   UNSPEC_HASHSTP
> +   UNSPEC_HASHCHK
> +   UNSPEC_HASHCHKP

You don't need a separate unspec (or insns patterns) for privileged
mode: you can just sprintf the template, or maybe you can use
print_operand_punct_valid if you think this 'p' will happen more often
(there exist more insns like this already, we just never generate them).

> +;; ROP mitigation instructions.
> +
> +(define_insn "hashst"
> +  [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m")
> +        (unspec:DI [(match_operand:DI 1 "int_reg_operand" "r")]
> +	           UNSPEC_HASHST))]

This needs unspec_volatile in principle.  There will be only one per
function, so it is kind of moot, but still.

Thanks,


Segher