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

Commit Message

Norbert Manthey Feb. 8, 2021, 8 p.m. UTC

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

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.

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]>

---
v3: * rephrased commit message to better explain code relocation
    * added release-acked


 xen/arch/x86/hvm/hvm.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Jan Beulich Feb. 9, 2021, 10:06 a.m. UTC | #1

On 08.02.2021 21:00, Norbert Manthey wrote:
> To prevent leaking HVM params via L1TF and similar issues on a
> hyperthread pair, let's load values of domains after performing all
> relevant checks, and blocking speculative execution.

I'd like to suggest "..., let's load values of domains only
after ...".

But there are other points open from v2; I'd like to further
suggest that you allow discussion on a prior version to first
settle, before sending a new one. Unless of course discussion
appears to have stalled.

Jan

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,13 @@  static int hvm_allow_set_param(struct domain *d,
     if ( rc )
         return rc;
 
+    if ( index >= HVM_NR_PARAMS )
+        return -EINVAL;
+
+    /* 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. */
@@ -4141,6 +4148,9 @@  static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
     if ( rc )
         return rc;
 
+    /* Make sure we evaluate permissions before loading data of domains. */
+    block_speculation();
+
     switch ( index )
     {
     case HVM_PARAM_CALLBACK_IRQ:
@@ -4388,6 +4398,10 @@  int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value)
     if ( rc )
         return rc;
 
+    /* Make sure the index bound check in hvm_get_param is respected, as well as
+       the above domain permissions. */
+    block_speculation();
+
     switch ( index )
     {
     case HVM_PARAM_ACPI_S_STATE:
@@ -4428,9 +4442,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;