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

Commit Message

Jan Beulich Dec. 20, 2019, 1:49 p.m. UTC

It is wrong for us to check frames beyond the guest specified limit
(in the native case, other than in the compat one).

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Dec. 27, 2019, 3:09 p.m. UTC | #1

On 20/12/2019 13:49, 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).
>
> Signed-off-by: Jan Beulich <[e-mail]>

Just like the restriction on sharing L2's, no guest is ever going to be
able to not zero all of this to operate on older hypervisors.

I agree that it is not ideal that this got into the ABI to begin with,
but as I said before, all you are doing is complicating
arch_set_info_guest() for a relaxation which no guest can use.

~Andrew

Jan Beulich Jan. 3, 2020, 10:24 a.m. UTC | #2

On 27.12.2019 16:09, Andrew Cooper wrote:
> On 20/12/2019 13:49, 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).
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> Just like the restriction on sharing L2's, no guest is ever going to be
> able to not zero all of this to operate on older hypervisors.
> 
> I agree that it is not ideal that this got into the ABI to begin with,
> but as I said before, all you are doing is complicating
> arch_set_info_guest() for a relaxation which no guest can use.

As asked before - would you mind clarifying where I'm complicating
things? I think I'm rather simplifying matters, by
- pulling out a calculation, storing the result into a now common
  (between native and compat cases) local variable,
- as a result making native and compat cases behave consistently,
  eliminating the need for reader to (try to) figure out why there
  is a difference in behavior.

Jan

Roger Pau Monné May 19, 2020, 8:42 a.m. UTC | #3

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 <[e-mail]>
> 
> --- 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.

Jan Beulich May 19, 2020, 9:36 a.m. UTC | #4

On 19.05.2020 10:42, Roger Pau Monné wrote:
> 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?

Since the array is embedded in the struct, I suppose callers simply
start out from a zero-initialized array, in which case the actual
count given doesn't matter. Additionally I think it is uncommon for
the function to get called on a vCPU with v->is_initialised already
set.

>> @@ -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.

Ah yes, changed.

>> @@ -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).

Oh, yes, of course. Thanks for spotting.

Jan

Patch

--- 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 |= 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;
 
-        for ( i = 0; i < nr_frames; ++i )
+        for ( i = 0; i < nr_gdt_frames; ++i )
             gdt_frames[i] = c.cmp->gdt_frames[i];
 
         rc = (int)pv_set_gdt(v, gdt_frames, c.cmp->gdt_ents);