| Message ID | 313f5f41-1572-aa0e-1112-d606ad5dee9c@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: XSA-298 follow-up | expand |
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
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
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
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
--- 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);
It is wrong for us to check frames beyond the guest specified limit. Signed-off-by: Jan Beulich <jbeulich@suse.com>