On Wed, May 31, 2023 at 11:58:12PM +0000, Jon Kohler wrote:
> > The goal of this barrier_nospec() is to prevent speculative execution
> > from bypassing the SPEC_CTRL write (due to misprediction of the
> > conditional branch, Spectre v1 style). Otherwise the next indirect
> > branch or unbalanced RET could be an attack target.
> >
> > So any previous LFENCEs before that conditional branch won't help here.
>
> Ah interesting. Ok, to be clear, thats a guest -> host attack, correct? And such
> an attack would not at all be thwarted by the first CALL retire + LFENCE that
> was added on commit 2b1299322016 ("x86/speculation: Add RSB VM Exit
> protections”)?
Right.
> Sorry to be long winded, just wanting to triple check because
> the aforementioned commit was added slightly after the original one, and I
> want to make extra sure that they aren’t solving the same thing.
>
> If that is indeed the case, does that commit need to be revisited at all?
>
> Or are we saying that this Intel vulnerability needs *two* LFENCE’s to keep
> the host secure?
The first LFENCE (FILL_RETURN_BUFFER) forces the CALL to retire so the
RSB stuff is guaranteed to take effect before the next unbalanced RET
can be speculatively executed.
The second LFENCE (vmx_spec_ctrl_restore_host) forces the conditional
branch to retire so the SPEC_CTRL write (potential IBRS/eIBRS
enablement) is guaranteed to take effect before the next indirect branch
and/or unbalanced RET can be speculatively executed.
So each LFENCE has a distinct purpose. That said, there are no indirect
branches or unbalanced RETs between them. So it should be fine to
combine them into a single LFENCE after both.
You could for example just remove the first LFENCE. But only for that
usage site, i.e. not for other users of FILL_RETURN_BUFFER.
Or, remove them both and add an LFENCE in vmx_vmexit() right after the
call to vmx_spec_ctrl_restore_host(). That might be clearer. Then it
could have a comment describing its dual purposes.
--
Josh
