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 <jul...@xen.org> 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 <jgr...@amazon.com> >>>> >>>> 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 <jgr...@amazon.com> >>> >>> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> >>> >>> 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
