Re: [PATCH v2 1/3] x86: relax GDT check in arch_set_info_guest()

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

On Fri, Dec 20, 2019 at 02:49:48PM +0100, Jan Beulich wrote:
> It is wrong for us to check frames beyond the guest specified limit
> (in the native case, other than in the compat one).

Wouldn't this result in arch_set_info_guest failing if gdt_ents was
smaller than the maximum? Or all callers always pass gdt_ents set to
the maximum?

> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -840,6 +840,7 @@ int arch_set_info_guest(
>  #ifdef CONFIG_PV
>      mfn_t cr3_mfn;
>      struct page_info *cr3_page = NULL;
> +    unsigned int nr_gdt_frames;
>      int rc = 0;
>  #endif
>  
> @@ -951,6 +952,8 @@ int arch_set_info_guest(
>      /* Ensure real hardware interrupts are enabled. */
>      v->arch.user_regs.eflags |= X86_EFLAGS_IF;
>  
> +    nr_gdt_frames = DIV_ROUND_UP(c(gdt_ents), 512);
> +
>      if ( !v->is_initialised )
>      {
>          if ( !compat && !(flags & VGCF_in_kernel) && !c.nat->ctrlreg[1] )
> @@ -982,9 +985,9 @@ int arch_set_info_guest(
>              fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
>          }
>  
> -        for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); ++i )
> -            fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
>          fail |= v->arch.pv.gdt_ents != c(gdt_ents);
> +        for ( i = 0; !fail && i < nr_gdt_frames; ++i )
> +            fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);

fail doesn't need to be OR'ed anymore here, since you check for it in
the loop condition.

>  
>          fail |= v->arch.pv.ldt_base != c(ldt_base);
>          fail |= v->arch.pv.ldt_ents != c(ldt_ents);
> @@ -1089,12 +1092,11 @@ int arch_set_info_guest(
>      else
>      {
>          unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
> -        unsigned int nr_frames = DIV_ROUND_UP(c.cmp->gdt_ents, 512);
>  
> -        if ( nr_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
> +        if ( nr_gdt_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
>              return -EINVAL;

Shouldn't this check be performed when nr_gdt_frames is initialized
instead of here? (as nr_gdt_frames is already used as a limit to
iterate over gdt_frames).

Thanks, Roger.