Re: [PATCH 10/15 V4] arm: Implement cortex-M return signing address codegen

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 06/12/2022 15:46, Andrea Corallo wrote:
> Hi Richard,
> 
> thanks for reviewing.
> 
> Just one clarification before I complete the respin of this patch.
> 
> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:
> 
> [...]
> 
>> Also, I think (out of an abundance of caution) we really need a
>> scheduling barrier placed before calls to gen_aut_nop() pattern is
>> emitted, to ensure that the scheduler never tries to move this
>> instruction away from the position we place it.  Use gen_blockage()
>> for that (see TARGET_SCHED_PROLOG).  Alternatively, we could make the
>> UNSPEC_PAC_NOP an unspec_volatile, which has the same effect (IIRC)
>> without needing an additional insn - if you use this approach, then
>> please make sure this is explained in a comment.
>>
>> +(define_insn "pacbti_nop"
>> +  [(set (reg:SI IP_REGNUM)
>> +	(unspec:SI [(reg:SI SP_REGNUM) (reg:SI LR_REGNUM)]
>> +		   UNSPEC_PACBTI_NOP))]
>> +  "arm_arch8m_main"
>> +  "pacbti\t%|ip, %|lr, %|sp"
>> +  [(set_attr "conds" "unconditional")])
>>
>> The additional side-effect of this being a BTI landing pad means that
>> we mustn't move any other instruction before it.  So I think this
>> needs to be an unspec_volatile as well.
> 
> IIUC from this we want to make all the three (UNSPEC_PAC_NOP,
> UNSPEC_PACBTI_NOP, UNSPEC_AUT_NOP) unspec volatile, correct?

UNSPEC_PAC_NOP doesn't need to be volatile. The register constraints 
will be enough to ensure it is run before any instruction that consumes 
the result it produces.

UNSPEC_PAC_BTI_NOP needs to be volatile, as it's essential that when we 
have an instruction (for example ldr r3, [r3]) early in the program that 
doesn't interact with the prologue then it cannot be migrated before the 
BTI as the BTI is a landing pad and must be the first instruction in the 
function.  This is why UNSPEC_BTI_NOP is volatile.

UNSPEC_AUT_NOP must be volatile because we want to ensure that no 
instruction is moved after this one and before the return as that might 
expose a ROP gadget to hackers.

R.

> 
> IIUC correctly the scheduler should not reorder them as we have
> expressed which register they consume and produce but is for double
> caution correct?
> 
>> On the tests, they are OK as they stand, but we lack anything that
>> will be tested when suitable hardware is unavailable (all tests are
>> "dg-do run").  Can we please have some compile-only tests as well?
> 
> Ack.
> 
> BR
> 
>    Andrea