[HVM,v4,1/1] hvm: refactor set param

Commit Message

Norbert Manthey Feb. 18, 2021, 3:01 p.m. UTC

To prevent leaking HVM params via L1TF and similar issues on a
hyperthread pair, let's load values of domains only after performing all
relevant checks, and blocking speculative execution.

For both get and set, the value of the index is already checked in the
outer calling function. The block_speculation calls in hvmop_get_param
and hvmop_set_param are removed, because is_hvm_domain already blocks
speculation.

Furthermore, speculative barriers are re-arranged to make sure we do not
allow guests running on co-located VCPUs to leak hvm parameter values of
other domains.

To improve symmetry between the get and set operations, function
hvmop_set_param is made static.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <[e-mail]>
Reported-by: Hongyan Xia <[e-mail]>
Release-Acked-by: Ian Jackson <[e-mail]>

---
v4: * add 'static' attribute to hvmop_set_param
    * drop introduced bound checks, e.g. in hvm_allow_set_param
    * drop existing bound check from hvm_set_param
    * do not introduce block_speculation in hvmop_set_param,
      as is_hvm_domain already blocks speculation
    * fix comments

 xen/arch/x86/hvm/hvm.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Jan Beulich Feb. 19, 2021, 1:44 p.m. UTC | #1

On 18.02.2021 16:01, Norbert Manthey wrote:
> To prevent leaking HVM params via L1TF and similar issues on a
> hyperthread pair, let's load values of domains only after performing all
> relevant checks, and blocking speculative execution.
> 
> For both get and set, the value of the index is already checked in the
> outer calling function. The block_speculation calls in hvmop_get_param
> and hvmop_set_param are removed, because is_hvm_domain already blocks
> speculation.
> 
> Furthermore, speculative barriers are re-arranged to make sure we do not
> allow guests running on co-located VCPUs to leak hvm parameter values of
> other domains.
> 
> To improve symmetry between the get and set operations, function
> hvmop_set_param is made static.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey <[e-mail]>
> Reported-by: Hongyan Xia <[e-mail]>
> Release-Acked-by: Ian Jackson <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>

Patch

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4060,7 +4060,7 @@  static int hvm_allow_set_param(struct domain *d,
                                uint32_t index,
                                uint64_t new_value)
 {
-    uint64_t value = d->arch.hvm.params[index];
+    uint64_t value;
     int rc;
 
     rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
@@ -4108,6 +4108,10 @@  static int hvm_allow_set_param(struct domain *d,
     if ( rc )
         return rc;
 
+    /* Make sure we evaluate permissions before loading data of domains. */
+    block_speculation();
+
+    value = d->arch.hvm.params[index];
     switch ( index )
     {
     /* The following parameters should only be changed once. */
@@ -4134,13 +4138,13 @@  static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
     struct vcpu *v;
     int rc;
 
-    if ( index >= HVM_NR_PARAMS )
-        return -EINVAL;
-
     rc = hvm_allow_set_param(d, index, value);
     if ( rc )
         return rc;
 
+    /* Make sure we evaluate permissions before loading data of domains. */
+    block_speculation();
+
     switch ( index )
     {
     case HVM_PARAM_CALLBACK_IRQ:
@@ -4305,7 +4309,7 @@  static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
     return rc;
 }
 
-int hvmop_set_param(
+static int hvmop_set_param(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
 {
     struct xen_hvm_param a;
@@ -4318,9 +4322,6 @@  int hvmop_set_param(
     if ( a.index >= HVM_NR_PARAMS )
         return -EINVAL;
 
-    /* Make sure the above bound check is not bypassed during speculation. */
-    block_speculation();
-
     d = rcu_lock_domain_by_any_id(a.domid);
     if ( d == NULL )
         return -ESRCH;
@@ -4388,6 +4389,9 @@  int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value)
     if ( rc )
         return rc;
 
+    /* Make sure the above domain permissions check is respected. */
+    block_speculation();
+
     switch ( index )
     {
     case HVM_PARAM_ACPI_S_STATE:
@@ -4428,9 +4432,6 @@  static int hvmop_get_param(
     if ( a.index >= HVM_NR_PARAMS )
         return -EINVAL;
 
-    /* Make sure the above bound check is not bypassed during speculation. */
-    block_speculation();
-
     d = rcu_lock_domain_by_any_id(a.domid);
     if ( d == NULL )
         return -ESRCH;