xsm/dummy: harden against speculative abuse

Commit Message

Jan Beulich Dec. 17, 2020, 11:57 a.m. UTC

First of all don't open-code is_control_domain(), which is already
suitably using evaluate_nospec(). Then also apply this construct to the
other paths of xsm_default_action(). Also guard two paths not using this
function.

Signed-off-by: Jan Beulich <[e-mail]>
---
While the functions are always_inline I'm not entirely certain we can
get away with doing this inside of them, rather than in the callers. It
will certainly take more to also guard builds with non-dummy XSM.

Comments

Jan Beulich Dec. 21, 2020, 1:15 p.m. UTC | #1

On 17.12.2020 12:57, Jan Beulich wrote:
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -76,20 +76,20 @@ static always_inline int xsm_default_act
>      case XSM_HOOK:
>          return 0;
>      case XSM_TARGET:
> -        if ( src == target )
> +        if ( evaluate_nospec(src == target) )
>          {
>              return 0;
>      case XSM_XS_PRIV:
> -            if ( is_xenstore_domain(src) )
> +            if ( evaluate_nospec(is_xenstore_domain(src)) )
>                  return 0;
>          }
>          /* fall through */
>      case XSM_DM_PRIV:
> -        if ( target && src->target == target )
> +        if ( target && evaluate_nospec(src->target == target) )
>              return 0;
>          /* fall through */
>      case XSM_PRIV:
> -        if ( src->is_privileged )
> +        if ( !is_control_domain(src) )
>              return 0;
>          return -EPERM;

And a stray ! slipped in here. Now fixed.

Jan

Wei Liu Jan. 5, 2021, 12:26 p.m. UTC | #2

On Mon, Dec 21, 2020 at 02:15:55PM +0100, Jan Beulich wrote:
> On 17.12.2020 12:57, Jan Beulich wrote:
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -76,20 +76,20 @@ static always_inline int xsm_default_act
> >      case XSM_HOOK:
> >          return 0;
> >      case XSM_TARGET:
> > -        if ( src == target )
> > +        if ( evaluate_nospec(src == target) )
> >          {
> >              return 0;
> >      case XSM_XS_PRIV:
> > -            if ( is_xenstore_domain(src) )
> > +            if ( evaluate_nospec(is_xenstore_domain(src)) )
> >                  return 0;
> >          }
> >          /* fall through */
> >      case XSM_DM_PRIV:
> > -        if ( target && src->target == target )
> > +        if ( target && evaluate_nospec(src->target == target) )
> >              return 0;
> >          /* fall through */
> >      case XSM_PRIV:
> > -        if ( src->is_privileged )
> > +        if ( !is_control_domain(src) )
> >              return 0;
> >          return -EPERM;
> 
> And a stray ! slipped in here. Now fixed.

FWIW:

Reviewed-by: Wei Liu <[e-mail]>

> 
> Jan

Patch

--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -76,20 +76,20 @@  static always_inline int xsm_default_act
     case XSM_HOOK:
         return 0;
     case XSM_TARGET:
-        if ( src == target )
+        if ( evaluate_nospec(src == target) )
         {
             return 0;
     case XSM_XS_PRIV:
-            if ( is_xenstore_domain(src) )
+            if ( evaluate_nospec(is_xenstore_domain(src)) )
                 return 0;
         }
         /* fall through */
     case XSM_DM_PRIV:
-        if ( target && src->target == target )
+        if ( target && evaluate_nospec(src->target == target) )
             return 0;
         /* fall through */
     case XSM_PRIV:
-        if ( src->is_privileged )
+        if ( !is_control_domain(src) )
             return 0;
         return -EPERM;
     default:
@@ -656,7 +656,7 @@  static XSM_INLINE int xsm_mmu_update(XSM
     XSM_ASSERT_ACTION(XSM_TARGET);
     if ( f != dom_io )
         rc = xsm_default_action(action, d, f);
-    if ( t && !rc )
+    if ( evaluate_nospec(t) && !rc )
         rc = xsm_default_action(action, d, t);
     return rc;
 }
@@ -750,6 +750,7 @@  static XSM_INLINE int xsm_xen_version (X
     case XENVER_platform_parameters:
     case XENVER_get_features:
         /* These sub-ops ignore the permission checks and return data. */
+        block_speculation();
         return 0;
     case XENVER_extraversion:
     case XENVER_compile_info: