| Message ID | 20220427140400.20152-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: Remove unnecessary mfn_valid() call from get_page_from_l1e() | expand |
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. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
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? Jan > @@ -902,13 +902,15 @@ get_page_from_l1e( > return -EINVAL; > } > > - if ( !mfn_valid(_mfn(mfn)) || > + valid = mfn_valid(_mfn(mfn)); > + > + if ( !valid || > (real_pg_owner = page_get_owner_and_reference(page)) == dom_io ) > { > int flip = 0; > > /* Only needed the reference to confirm dom_io ownership. */ > - if ( mfn_valid(_mfn(mfn)) ) > + if ( valid ) > put_page(page); > > /* DOMID_IO reverts to caller for privilege checks. */
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.
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 72dbce43b13a..31b9f96dc0df 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -887,7 +887,7 @@ get_page_from_l1e( uint32_t l1f = l1e_get_flags(l1e); struct vcpu *curr = current; struct domain *real_pg_owner; - bool write; + bool write, valid; if ( unlikely(!(l1f & _PAGE_PRESENT)) ) { @@ -902,13 +902,15 @@ get_page_from_l1e( return -EINVAL; } - if ( !mfn_valid(_mfn(mfn)) || + valid = mfn_valid(_mfn(mfn)); + + if ( !valid || (real_pg_owner = page_get_owner_and_reference(page)) == dom_io ) { int flip = 0; /* Only needed the reference to confirm dom_io ownership. */ - if ( mfn_valid(_mfn(mfn)) ) + if ( valid ) put_page(page); /* DOMID_IO reverts to caller for privilege checks. */
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. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/mm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)