Re: [PATCH] x86/mm: Remove unnecessary mfn_valid() call from get_page_from_l1e()

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 26/05/2022 16:31, Jan Beulich wrote:
> On 27.04.2022 16:04, Andrew Cooper wrote:
>> mfn_valid() is not a trivially simple, and contains an evaluate_nospec() for
>> speculative defence.  Avoid calling it redundantly, and just store the result
>> of the first call.
> Since it took quite some time for this to actually be committed, I did
> notice it among more recent commits, and I've grown a question: Isn't
> the latching of the result in a local variable undermining the supposed
> speculative defense? It's not as if I could point out a particular
> gadget here, but it feels like the adjustment should have specifically
> justified the speculative safety ... But I guess my understanding of
> all of this might still be somewhat flaky?

The eval_nospec() in mfn_valid() is to avoid a speculative over-read of
pdx_group_valid[].

It does not provide any protection for any other logic.

In particular, it does not provide any safety whatsoever in
get_page_from_l1e() because the lfence is the wrong side of the
conditional jump.  i.e. you've got:

    ...
    call mfn_valid // lfence in here
    test %al, %al
    je 1f
    ...                    // lfence missing from here
1:
    ...                    // and here


The change I've made is simply CSE that a compiler could do
automatically. if it could be persuaded that __mfn_valid() was pure
(which it logically is.)

If logic in get_page_from_l1e() needs Spectre protections for any
reason, it needs its own eval_nospec()/array_access_nospec()/etc because
it is specifically not safe to rely on incidental lfence's from
unrelated protections.

~Andrew

P.S. I need to add this to the list of reasons of why we need a "coding
& speculation safety" doc in the tree.