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

[Julien Grall <julien@xxxxxxx>]

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

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

--
Julien Grall