| Message ID | c36cac91-49ae-6bb2-b057-195031979d21@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: XSA-298 follow-up | expand |
On 20/12/2019 13:50, Jan Beulich wrote: > It is wrong for us to check the base address when there's no LDT in the > first place. Once we don't do this check anymore we can also set the > base address to a non-canonical value when the LDT is empty. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I've only just spotted, but this is a semantic change to the guest. Previously, base with ents=0 would be preserved via arch_get_info_guest(). This is likely not interesting from a guests point of view, so is probably fine to change in the ABI. As for the change itself, do you realise that you've not actually relaxed anything? There are checks earlier in arch_set_info_guest() which you haven't altered. Finally, a similar concern about changes which a guest can't actually make use of, even if this one seems rather more minor. ~Andrew
On 27.12.2019 16:26, Andrew Cooper wrote: > On 20/12/2019 13:50, Jan Beulich wrote: >> It is wrong for us to check the base address when there's no LDT in the >> first place. Once we don't do this check anymore we can also set the >> base address to a non-canonical value when the LDT is empty. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I've only just spotted, but this is a semantic change to the guest. > Previously, base with ents=0 would be preserved via arch_get_info_guest(). I've done (extended from v1) this upon your request; I did notice this side effect of the change. This is (partly) why I've made an adjustment to arch_get_info_guest() in the first place. > Finally, a similar concern about changes which a guest can't actually > make use of, even if this one seems rather more minor. Like for the GDT case, the goal isn't so much to allow guests more relaxed behavior (albeit for ones not caring about being compatible with older Xen this is still a secondary goal), but to get behavior in Xen into an overall more consistent shape. Jan
On Fri, Dec 20, 2019 at 02:50:06PM +0100, Jan Beulich wrote: > It is wrong for us to check the base address when there's no LDT in the > first place. Once we don't do this check anymore we can also set the > base address to a non-canonical value when the LDT is empty. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> I wonder if a ldt_ents check should also be added to pv_map_ldt_shadow_page in order to avoid trying to get the mapping of the LDT. Thanks, Roger.
On 19.05.2020 11:02, Roger Pau Monné wrote: > On Fri, Dec 20, 2019 at 02:50:06PM +0100, Jan Beulich wrote: >> It is wrong for us to check the base address when there's no LDT in the >> first place. Once we don't do this check anymore we can also set the >> base address to a non-canonical value when the LDT is empty. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > I wonder if a ldt_ents check should also be added to > pv_map_ldt_shadow_page in order to avoid trying to get the mapping of > the LDT. We already have if ( unlikely((offset >> 3) >= curr->arch.pv.ldt_ents) ) { ASSERT_UNREACHABLE(); return false; } What else do you mean? Jan
On Tue, May 19, 2020 at 11:12:49AM +0200, Jan Beulich wrote: > On 19.05.2020 11:02, Roger Pau Monné wrote: > > On Fri, Dec 20, 2019 at 02:50:06PM +0100, Jan Beulich wrote: > >> It is wrong for us to check the base address when there's no LDT in the > >> first place. Once we don't do this check anymore we can also set the > >> base address to a non-canonical value when the LDT is empty. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > > I wonder if a ldt_ents check should also be added to > > pv_map_ldt_shadow_page in order to avoid trying to get the mapping of > > the LDT. > > We already have > > if ( unlikely((offset >> 3) >= curr->arch.pv.ldt_ents) ) > { > ASSERT_UNREACHABLE(); > return false; > } > > What else do you mean? Oh, I've missed that. I was searching for a check before accessing ldt_base, which is done at initialization time. That's indeed fine. Roger.
--- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -959,8 +959,10 @@ int arch_set_info_guest( if ( !compat && !(flags & VGCF_in_kernel) && !c.nat->ctrlreg[1] ) return -EINVAL; - v->arch.pv.ldt_base = c(ldt_base); v->arch.pv.ldt_ents = c(ldt_ents); + v->arch.pv.ldt_base = v->arch.pv.ldt_ents + ? c(ldt_base) + : (unsigned long)ZERO_BLOCK_PTR; } else { @@ -989,8 +991,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; --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1563,7 +1563,7 @@ void arch_get_info_guest(struct vcpu *v, } else { - c(ldt_base = v->arch.pv.ldt_base); + c(ldt_base = v->arch.pv.ldt_ents ? v->arch.pv.ldt_base : 0); c(ldt_ents = v->arch.pv.ldt_ents); for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); ++i ) c(gdt_frames[i] = v->arch.pv.gdt_frames[i]); --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -3702,14 +3702,15 @@ long do_mmuext_op( case MMUEXT_SET_LDT: { unsigned int ents = op.arg2.nr_ents; - unsigned long ptr = ents ? op.arg1.linear_addr : 0; + unsigned long ptr = ents ? op.arg1.linear_addr + : (unsigned long)ZERO_BLOCK_PTR; if ( unlikely(currd != pg_owner) ) rc = -EPERM; else if ( paging_mode_external(currd) ) rc = -EINVAL; - else if ( ((ptr & (PAGE_SIZE - 1)) != 0) || !__addr_ok(ptr) || - (ents > 8192) ) + else if ( (ents > 8192) || + (ents && ((ptr & (PAGE_SIZE - 1)) || !__addr_ok(ptr))) ) { gdprintk(XENLOG_WARNING, "Bad args to SET_LDT: ptr=%lx, ents=%x\n", ptr, ents);
It is wrong for us to check the base address when there's no LDT in the first place. Once we don't do this check anymore we can also set the base address to a non-canonical value when the LDT is empty. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Set v->arch.pv.ldt_base to non-canonical for an empty LDT, plus related necessary adjustments.