Re: [PATCH 1/2] xen/arm: entry: Place a speculation barrier following an ret instruction

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 18 Aug 2020, at 18:34, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 18/08/2020 18:06, Bertrand Marquis wrote:
>>> On 18 Aug 2020, at 17:43, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> 
>>> 
>>> On 18/08/2020 17:35, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>> Somehow we stopped on this thread and you did already most of the work so 
>>>> I think we should try to finish what you started
>>> 
>>> Sorry this fell-through the cracks. I have a new version for patch #1, but 
>>> not yet patch #2.
>> No problem this came back while trying to reduce my todolist stack :-)
>>> 
>>> I am still debating with myself where the speculation barrier should be 
>>> added after the SMC :).
>> I think that we should unless the SMC is in the context switch path (as all 
>> other calls should not have a performance impact).
> I will introduce *_unsafe() helpers that will not contain the speculation 
> barrier. It could then be used in place where we think the barrier is 
> unnecessary.

great idea, this will make it clear by reading the code reducing the need of 
documentation.

> 
>>> 
>>>>> On 4 Jul 2020, at 17:07, Julien Grall <julien@xxxxxxx> wrote:
>>>>> 
>>>>> On 17/06/2020 17:23, Julien Grall wrote:
>>>>>> Hi,
>>>>>> On 16/06/2020 22:24, Stefano Stabellini wrote:
>>>>>>> On Tue, 16 Jun 2020, Julien Grall wrote:
>>>>>>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>>>>>> 
>>>>>>>> Some CPUs can speculate past a RET instruction and potentially perform
>>>>>>>> speculative accesses to memory before processing the return.
>>>>>>>> 
>>>>>>>> There is no known gadget available after the RET instruction today.
>>>>>>>> However some of the registers (such as in check_pending_guest_serror())
>>>>>>>> may contain a value provided the guest.
>>>>>>>                                ^ by
>>>>>>> 
>>>>>>> 
>>>>>>>> In order to harden the code, it would be better to add a speculation
>>>>>>>> barrier after each RET instruction. The performance is meant to be
>>>>>>>> negligeable as the speculation barrier is not meant to be archicturally
>>>>>>>> executed.
>>>>>>>> 
>>>>>>>> Note that on arm32, the ldmia instruction will act as a return from the
>>>>>>>> function __context_switch(). While the whitepaper doesn't suggest it is
>>>>>>>> possible to speculate after the instruction, add preventively a
>>>>>>>> speculation barrier after it as well.
>>>>>>>> 
>>>>>>>> This is part of the work to mitigate straight-line speculation.
>>>>>>>> 
>>>>>>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>>>>> 
>>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>>>>> 
>>>>>>> I did a compile-test on the patch too.
>>>>>>> 
>>>>>>> 
>>>>>>>> ---
>>>>>>>> 
>>>>>>>> I am still unsure whether we preventively should add a speculation 
>>>>>>>> barrier
>>>>>>>> preventively after all the RET instructions in arm*/lib/. The smc call 
>>>>>>>> be
>>>>>>>> taken care in a follow-up patch.
>>>>>>> 
>>>>>>> SMC is great to have but it seems to be overkill to do the ones under
>>>>>>> lib/.
>>>>>> From my understanding, the compiler will add a speculation barrier 
>>>>>> preventively after each 'ret' when the mitigation are turned on.So it 
>>>>>> feels to me we want to follow the same approach.
>>>>>> Obviously, we can avoid them but I would like to have a justification 
>>>>>> for not adding them (nothing is overkilled against speculation ;)).
>>>>> 
>>>>> I finally found some time to look at arm*/lib in more details. Some of 
>>>>> the helpers can definitely be called with guest inputs.
>>>>> 
>>>>> For instance, memchr() is called from hypfs_get_path_user() with the 3rd 
>>>>> argument controlled by the guest. In both 32-bit and 64-bit 
>>>>> implementation, you will reach the end of the function memchr() with 
>>>>> r2/w2 and r3/w3 (it contains a character from the buffer) controlled by 
>>>>> the guest.
>>>>> 
>>>>> As this is the only function in the unit, we don't know what will be the 
>>>>> instructions right after RET. So it would be safer to add a speculation 
>>>>> barrier there too.
>>>> How about adding a speculation barrier directly in the ENDPROC macro ?
>>> 
>>> This would unfortunately not cover all the cases because you can return in 
>>> the middle of the function. I will have a look to see if we can leverage it.
>> I agree that it would not solve all of them but a big part would be solved 
>> by it.
>> An other solution might be to have a RETURN macro encoded as "mov pc,lr; sb" 
>> and "ret; sb”.
> 
> This is a bit messy on Arm32 because not all the return are using "mov 
> pc,lr".  Anyway, I will explore the two approaches.

ack.

> 
>> The patch sounds good, i just need to find a way to analyse if you missed a 
>> ret or not which is not easy with such a patch :-)
> 
> I did struggle to find all the instances. The directory lib/ is actually 
> quite difficult to go through on 32-bit because they are multiple way to
> implement a return.

some part of the assembler code might benefit from a few branch instead of 
middle return to make the code clearer also but this might impact
a bit the performances.

> 
> Finding a way to reduce manual speculation barrier would definitely be 
> helpful. I will try to revise the patch during this week.

ok i will on my side list the returns to be able to review the final patch more 
easily.

Cheers
Bertrand