Re: [Xen-devel] [PATCH v2 3/6] hvm/mtrr: use the hardware number of variable ranges for Dom0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.05.18 at 13:53, <roger.pau@xxxxxxxxxx> wrote:
> On Wed, May 16, 2018 at 02:39:26AM -0600, Jan Beulich wrote:
>> >>> On 15.05.18 at 16:36, <roger.pau@xxxxxxxxxx> wrote:
>> > +        for ( i = 0; i < num_var_ranges; i++ )
>> 
>> Following your v1 I had already put together a patch to change just the
>> save and load functions here, as the adjustments are necessary
>> independent of the Dom0 aspect. Should num_var_ranges indeed be
>> below MTRR_VCNT, there's an information leak here (of hypervisor stack
>> data) without pre-initializing hw_mtrr. Here's the hunk from my patch, in
>> case you care to re-use parts of it:
>> 
>> @@ -676,22 +676,22 @@ int hvm_set_mem_pinned_cacheattr(struct
>>  
>>  static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
>>  {
>> -    int i;
>>      struct vcpu *v;
>> -    struct hvm_hw_mtrr hw_mtrr;
>> -    struct mtrr_state *mtrr_state;
>> +
>>      /* save mtrr&pat */
>>      for_each_vcpu(d, v)
>>      {
>> -        mtrr_state = &v->arch.hvm_vcpu.mtrr;
>> +        const struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
>> +        struct hvm_hw_mtrr hw_mtrr = {
>> +            .msr_mtrr_def_type = mtrr_state->def_type |
>> +                                 (mtrr_state->enabled << 10),
>> +            .msr_mtrr_cap      = mtrr_state->mtrr_cap,
>> +        };
>> +        unsigned int i;
>>  
>>          hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
>>  
>> -        hw_mtrr.msr_mtrr_def_type = mtrr_state->def_type
>> -                                | (mtrr_state->enabled << 10);
>> -        hw_mtrr.msr_mtrr_cap = mtrr_state->mtrr_cap;
>> -
>> -        for ( i = 0; i < MTRR_VCNT; i++ )
>> +        for ( i = 0; i < (uint8_t)hw_mtrr.msr_mtrr_cap; i++ )
>>          {
>>              /* save physbase */
>>              hw_mtrr.msr_mtrr_var[i*2] =
>> 
>> (I didn't send it out yet as I'm generally of the opinion that prior to
>> branching focus should be on the code to be released rather than
>> the next following version.)
> 
> Would you be OK if I integrate this as a pre-patch to this one in my
> series?

Sure, but then maybe better use the full one:

x86/HVM: improve MTRR load checks

We should not assume that the incoming set of values contains exactly
MTRR_VCNT variable range MSRs. Permit a smaller amount and reject a
bigger one. As a result the save path then also needs to no longer use
a fixed upper bound, in turn requiring unused space in the save record
to be zeroed up front.

Also slightly refine types where appropriate.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- unstable.orig/xen/arch/x86/hvm/mtrr.c
+++ unstable/xen/arch/x86/hvm/mtrr.c
@@ -676,22 +676,22 @@ int hvm_set_mem_pinned_cacheattr(struct
 
 static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
 {
-    int i;
     struct vcpu *v;
-    struct hvm_hw_mtrr hw_mtrr;
-    struct mtrr_state *mtrr_state;
+
     /* save mtrr&pat */
     for_each_vcpu(d, v)
     {
-        mtrr_state = &v->arch.hvm_vcpu.mtrr;
+        const struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
+        struct hvm_hw_mtrr hw_mtrr = {
+            .msr_mtrr_def_type = mtrr_state->def_type |
+                                 (mtrr_state->enabled << 10),
+            .msr_mtrr_cap      = mtrr_state->mtrr_cap,
+        };
+        unsigned int i;
 
         hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
 
-        hw_mtrr.msr_mtrr_def_type = mtrr_state->def_type
-                                | (mtrr_state->enabled << 10);
-        hw_mtrr.msr_mtrr_cap = mtrr_state->mtrr_cap;
-
-        for ( i = 0; i < MTRR_VCNT; i++ )
+        for ( i = 0; i < (uint8_t)hw_mtrr.msr_mtrr_cap; i++ )
         {
             /* save physbase */
             hw_mtrr.msr_mtrr_var[i*2] =
@@ -729,6 +729,14 @@ static int hvm_load_mtrr_msr(struct doma
     if ( hvm_load_entry(MTRR, h, &hw_mtrr) != 0 )
         return -EINVAL;
 
+    if ( (uint8_t)hw_mtrr.msr_mtrr_cap > MTRR_VCNT )
+    {
+        dprintk(XENLOG_G_ERR,
+                "HVM restore: %pv: too many (%d) variable range MTRRs\n",
+                v, (uint8_t)hw_mtrr.msr_mtrr_cap);
+        return -EINVAL;
+    }
+
     mtrr_state = &v->arch.hvm_vcpu.mtrr;
 
     hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);
@@ -738,7 +746,7 @@ static int hvm_load_mtrr_msr(struct doma
     for ( i = 0; i < NUM_FIXED_MSR; i++ )
         mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
 
-    for ( i = 0; i < MTRR_VCNT; i++ )
+    for ( i = 0; i < (uint8_t)hw_mtrr.msr_mtrr_cap; i++ )
     {
         mtrr_var_range_msr_set(d, mtrr_state,
                                MSR_IA32_MTRR_PHYSBASE(i),

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel