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

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

[Andrew Cooper]

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(-)

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. */
-- 
2.11.0

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

[Jan Beulich]

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>

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

[Jan Beulich]

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

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

[Andrew Cooper]

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.