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