[1/3] x86: relax GDT check in arch_set_info_guest()
diff mbox series

Message ID 313f5f41-1572-aa0e-1112-d606ad5dee9c@suse.com
State Superseded
Headers show
Series
  • x86: XSA-298 follow-up
Related show

Commit Message

Jan Beulich Dec. 6, 2019, 10:14 a.m. UTC
It is wrong for us to check frames beyond the guest specified limit.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Andrew Cooper Dec. 6, 2019, 10:25 a.m. UTC | #1
On 06/12/2019 10:14, Jan Beulich wrote:
> It is wrong for us to check frames beyond the guest specified limit.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I don't completely agree.  The code has been like this since it was
introduced, and is used to check data from the domain builder (inc
migration), and from the guests.

At the moment, every caller is required not to pass junk in unused
frames, and I don't see an issue with keeping this behaviour.

~Andrew
Jan Beulich Dec. 6, 2019, 11:32 a.m. UTC | #2
On 06.12.2019 11:25, Andrew Cooper wrote:
> On 06/12/2019 10:14, Jan Beulich wrote:
>> It is wrong for us to check frames beyond the guest specified limit.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I don't completely agree.  The code has been like this since it was
> introduced, and is used to check data from the domain builder (inc
> migration), and from the guests.
> 
> At the moment, every caller is required not to pass junk in unused
> frames, and I don't see an issue with keeping this behaviour.

Keeping the behavior isn't going to break anything, yes, but it
shouldn't have been this way to begin with. I simply don't see
the value of validating data we're not consuming anyway. Perhaps
I could say "not helpful" or "pointless" instead of "wrong" ...

Jan
Andrew Cooper Dec. 6, 2019, 7:51 p.m. UTC | #3
On 06/12/2019 11:32, Jan Beulich wrote:
> On 06.12.2019 11:25, Andrew Cooper wrote:
>> On 06/12/2019 10:14, Jan Beulich wrote:
>>> It is wrong for us to check frames beyond the guest specified limit.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> I don't completely agree.  The code has been like this since it was
>> introduced, and is used to check data from the domain builder (inc
>> migration), and from the guests.
>>
>> At the moment, every caller is required not to pass junk in unused
>> frames, and I don't see an issue with keeping this behaviour.
> Keeping the behavior isn't going to break anything, yes, but it
> shouldn't have been this way to begin with. I simply don't see
> the value of validating data we're not consuming anyway. Perhaps
> I could say "not helpful" or "pointless" instead of "wrong" ...

But in other cases we go out of our way to check parameters (especially
reserved fields) even when they aren't presently consumed.

i.e. what do we gain (other than more complicated code) by relaxing a
restriction we know is obeyed by every caller?

~Andrew
Jan Beulich Dec. 9, 2019, 12:05 p.m. UTC | #4
On 06.12.2019 20:51, Andrew Cooper wrote:
> On 06/12/2019 11:32, Jan Beulich wrote:
>> On 06.12.2019 11:25, Andrew Cooper wrote:
>>> On 06/12/2019 10:14, Jan Beulich wrote:
>>>> It is wrong for us to check frames beyond the guest specified limit.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> I don't completely agree.  The code has been like this since it was
>>> introduced, and is used to check data from the domain builder (inc
>>> migration), and from the guests.
>>>
>>> At the moment, every caller is required not to pass junk in unused
>>> frames, and I don't see an issue with keeping this behaviour.
>> Keeping the behavior isn't going to break anything, yes, but it
>> shouldn't have been this way to begin with. I simply don't see
>> the value of validating data we're not consuming anyway. Perhaps
>> I could say "not helpful" or "pointless" instead of "wrong" ...
> 
> But in other cases we go out of our way to check parameters (especially
> reserved fields) even when they aren't presently consumed.

Which we do to make sure we could use the fields down the road
without breaking existing callers. That's quite different from
the overzealous checking we do here.

> i.e. what do we gain (other than more complicated code) by relaxing a
> restriction we know is obeyed by every caller?

First - I don't think the code gets more complicated by this
change (nor the LDT counterpart). If anything I'm seeing a
really minor simplification (by consistently using a now
common variable). Further, if you look closely, you'll note
that the compat path is already only checking the specified
number of frames. Hence I'm bringing the non-compat one in
line, i.e. an improvement in consistency.

Jan

Patch
diff mbox series

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