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

[Jan Beulich <jbeulich@xxxxxxxx>]

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