A few things stumbled across while doing the investigations. 1: relax GDT check in arch_set_info_guest() 2: relax LDT check in arch_set_info_guest() 3: PV: polish pv_set_gdt() Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
It is wrong for us to check frames beyond the guest specified limit.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- 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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
It is wrong for us to check the base address when there's no LDT in the
first place.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: I also wonder whether we wouldn't better set v->arch.pv.ldt_base to
zero for an empty LDT, just like do_mmuext_op() does.
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -989,8 +989,9 @@ int arch_set_info_guest(
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);
+ if ( v->arch.pv.ldt_ents )
+ fail |= v->arch.pv.ldt_base != c(ldt_base);
if ( fail )
return -EOPNOTSUPP;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 06/12/2019 10:14, Jan Beulich wrote: > It is wrong for us to check the base address when there's no LDT in the > first place. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: I also wonder whether we wouldn't better set v->arch.pv.ldt_base to > zero for an empty LDT, just like do_mmuext_op() does. My query with patch 1 is also applicable here. As for setting it to zero, we should use something non-canonical instead. Doing so would have saved us from XSA-298, which was only a problem in guests because the base falling to 0. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 06.12.2019 11:33, Andrew Cooper wrote: > On 06/12/2019 10:14, Jan Beulich wrote: >> It is wrong for us to check the base address when there's no LDT in the >> first place. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> TBD: I also wonder whether we wouldn't better set v->arch.pv.ldt_base to >> zero for an empty LDT, just like do_mmuext_op() does. > > My query with patch 1 is also applicable here. As is my answer there. > As for setting it to zero, we should use something non-canonical > instead. Doing so would have saved us from XSA-298, which was only a > problem in guests because the base falling to 0. I can certainly do so (in do_mmuext_op() then as well). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
There's no need to invoke get_page_from_gfn(), and there's also no need
to update the passed in frames[]. Invoke get_page_and_type() directly.
Also make the function's frames[] parameter const, change its return
type to int, and drop the bogus casts from two of its invocations.
Finally a little bit of cosmetics.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1089,7 +1089,7 @@ int arch_set_info_guest(
return rc;
if ( !compat )
- rc = (int)pv_set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
+ rc = pv_set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
else
{
unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
@@ -1100,7 +1100,7 @@ int arch_set_info_guest(
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);
+ rc = pv_set_gdt(v, gdt_frames, c.cmp->gdt_ents);
}
if ( rc != 0 )
return rc;
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -96,7 +96,8 @@ void pv_destroy_gdt(struct vcpu *v)
}
}
-long pv_set_gdt(struct vcpu *v, unsigned long *frames, unsigned int entries)
+int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
+ unsigned int entries)
{
struct domain *d = v->domain;
l1_pgentry_t *pl1e;
@@ -110,17 +111,11 @@ long pv_set_gdt(struct vcpu *v, unsigned
/* Check the pages in the new GDT. */
for ( i = 0; i < nr_frames; i++ )
{
- struct page_info *page;
+ mfn_t mfn = _mfn(frames[i]);
- page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
- if ( !page )
+ if ( !mfn_valid(mfn) ||
+ !get_page_and_type(mfn_to_page(mfn), d, PGT_seg_desc_page) )
goto fail;
- if ( !get_page_type(page, PGT_seg_desc_page) )
- {
- put_page(page);
- goto fail;
- }
- frames[i] = mfn_x(page_to_mfn(page));
}
/* Tear down the old GDT. */
@@ -139,9 +134,8 @@ long pv_set_gdt(struct vcpu *v, unsigned
fail:
while ( i-- > 0 )
- {
put_page_and_type(mfn_to_page(_mfn(frames[i])));
- }
+
return -EINVAL;
}
--- a/xen/include/asm-x86/pv/mm.h
+++ b/xen/include/asm-x86/pv/mm.h
@@ -25,7 +25,8 @@
int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs);
-long pv_set_gdt(struct vcpu *v, unsigned long *frames, unsigned int entries);
+int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
+ unsigned int entries);
void pv_destroy_gdt(struct vcpu *v);
bool pv_map_ldt_shadow_page(unsigned int off);
@@ -43,8 +44,8 @@ static inline int pv_ro_page_fault(unsig
return 0;
}
-static inline long pv_set_gdt(struct vcpu *v, unsigned long *frames,
- unsigned int entries)
+static inline int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
+ unsigned int entries)
{ ASSERT_UNREACHABLE(); return -EINVAL; }
static inline void pv_destroy_gdt(struct vcpu *v) { ASSERT_UNREACHABLE(); }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 06/12/2019 10:15, Jan Beulich wrote: > There's no need to invoke get_page_from_gfn(), and there's also no need > to update the passed in frames[]. Invoke get_page_and_type() directly. > > Also make the function's frames[] parameter const, change its return > type to int, and drop the bogus casts from two of its invocations. > > Finally a little bit of cosmetics. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.