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.
