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

[Andrea Corallo <andrea.corallo@arm.com>]

Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:

> 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.

Understood now, thanks.

  Andrea