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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 16, 2018 at 06:01:00AM -0600, Jan Beulich wrote:
> >>> 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:

Thanks, I've added this to my MTRR series just after the switch to use
MASK_EXTR to get the VCNT.

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

I've changed this to use MASK_EXTR instead (and switched the cases
below).

>          {
>              /* 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 )

And here I would compare against the VCNT in mtrr_state->mtrr_cat
instead of MTRR_VCNT if you agree.

Thanks, Roger.

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