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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

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


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

> 
> Note that hypfs is currently only accessibly by Dom0. Yet, I still think we 
> should try to harden any code if we can :).

Agree with that.

Cheers (and sorry for the delay)
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall