> On 18 Aug 2020, at 18:34, Julien Grall <jul...@xen.org> wrote: > > Hi Bertrand, > > On 18/08/2020 18:06, Bertrand Marquis wrote: >>> On 18 Aug 2020, at 17:43, Julien Grall <jul...@xen.org> 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 <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 ? >>> >>> 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
