| Message ID | 499e029a7d2fce4fb9118b1e508313f369b37c79.1715102098.git.alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Expose consistent topology to guests | expand |
On Wed, May 08, 2024 at 01:39:22PM +0100, Alejandro Vallejo wrote: > While at it, add a check for the reserved field in the hidden save area. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > --- > v2: > * New patch. Addresses the missing check for rsvd_zero in v1. Oh, it would be better if this was done at the time when rsvd_zero is introduced. I think this should be moved ahead of the series, so that the patch that introduces rsvd_zero can add the check in lapic_check_hidden(). > --- > xen/arch/x86/hvm/vlapic.c | 41 ++++++++++++++++++++++++++++----------- > 1 file changed, 30 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > index 8a244100009c..2f06bff1b2cc 100644 > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -1573,35 +1573,54 @@ static void lapic_load_fixup(struct vlapic *vlapic) > v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); > } > > -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) > +static int cf_check lapic_check_hidden(const struct domain *d, > + hvm_domain_context_t *h) > { > unsigned int vcpuid = hvm_load_instance(h); > - struct vcpu *v; > - struct vlapic *s; > + struct hvm_hw_lapic s; > > if ( !has_vlapic(d) ) > return -ENODEV; > > /* Which vlapic to load? */ > - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > + if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ) > { > dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", > d->domain_id, vcpuid); > return -EINVAL; > } > - s = vcpu_vlapic(v); > > - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) > + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) ) Can't you use hvm_get_entry() to perform the sanity checks: const struct hvm_hw_lapic *s = hvm_get_entry(LAPIC, h); Thanks, Roger.
On 08/05/2024 1:39 pm, Alejandro Vallejo wrote: > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > index 8a244100009c..2f06bff1b2cc 100644 > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -1573,35 +1573,54 @@ static void lapic_load_fixup(struct vlapic *vlapic) > v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); > } > > -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) > +static int cf_check lapic_check_hidden(const struct domain *d, > + hvm_domain_context_t *h) > { > unsigned int vcpuid = hvm_load_instance(h); > - struct vcpu *v; > - struct vlapic *s; > + struct hvm_hw_lapic s; > > if ( !has_vlapic(d) ) > return -ENODEV; > > /* Which vlapic to load? */ > - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > + if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ) As you're editing this anyway, swap for if ( !domain_vcpu(d, vcpuid) ) please. > { > dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", > d->domain_id, vcpuid); > return -EINVAL; > } > - s = vcpu_vlapic(v); > > - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) > + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) ) > + return -ENODATA; > + > + /* EN=0 with EXTD=1 is illegal */ > + if ( (s.apic_base_msr & (APIC_BASE_ENABLE | APIC_BASE_EXTD)) == > + APIC_BASE_EXTD ) > + return -EINVAL; This is very insufficient auditing for the incoming value, but it turns out that there's no nice logic for this at all. As it's just a less obfuscated form of the logic from lapic_load_hidden(), it's probably fine to stay as it is for now. The major changes since this logic was written originally are that the CPU policy correct (so we can reject EXTD on VMs which can't see x2apic), and that we now prohibit VMs moving the xAPIC MMIO window away from its default location (as this would require per-vCPU P2Ms in order to virtualise properly.) ~Andrew
On Thu, May 23, 2024 at 07:58:57PM +0100, Andrew Cooper wrote: > On 08/05/2024 1:39 pm, Alejandro Vallejo wrote: > > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > > index 8a244100009c..2f06bff1b2cc 100644 > > --- a/xen/arch/x86/hvm/vlapic.c > > +++ b/xen/arch/x86/hvm/vlapic.c > > @@ -1573,35 +1573,54 @@ static void lapic_load_fixup(struct vlapic *vlapic) > > v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); > > } > > > > -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) > > +static int cf_check lapic_check_hidden(const struct domain *d, > > + hvm_domain_context_t *h) > > { > > unsigned int vcpuid = hvm_load_instance(h); > > - struct vcpu *v; > > - struct vlapic *s; > > + struct hvm_hw_lapic s; > > > > if ( !has_vlapic(d) ) > > return -ENODEV; > > > > /* Which vlapic to load? */ > > - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > > + if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ) > > As you're editing this anyway, swap for > > if ( !domain_vcpu(d, vcpuid) ) > > please. > > > { > > dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", > > d->domain_id, vcpuid); > > return -EINVAL; > > } > > - s = vcpu_vlapic(v); > > > > - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) > > + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) ) > > + return -ENODATA; > > + > > + /* EN=0 with EXTD=1 is illegal */ > > + if ( (s.apic_base_msr & (APIC_BASE_ENABLE | APIC_BASE_EXTD)) == > > + APIC_BASE_EXTD ) > > + return -EINVAL; > > This is very insufficient auditing for the incoming value, but it turns > out that there's no nice logic for this at all. > > As it's just a less obfuscated form of the logic from > lapic_load_hidden(), it's probably fine to stay as it is for now. > > The major changes since this logic was written originally are that the > CPU policy correct (so we can reject EXTD on VMs which can't see > x2apic), and that we now prohibit VMs moving the xAPIC MMIO window away > from its default location (as this would require per-vCPU P2Ms in order > to virtualise properly.) Since this is just migration of the existing checks I think keeping them as-is is best. Adding new checks should be done in a followup patch. Thanks, Roger.
On 23/05/2024 15:50, Roger Pau Monné wrote: > On Wed, May 08, 2024 at 01:39:22PM +0100, Alejandro Vallejo wrote: >> While at it, add a check for the reserved field in the hidden save area. >> >> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> >> --- >> v2: >> * New patch. Addresses the missing check for rsvd_zero in v1. > > Oh, it would be better if this was done at the time when rsvd_zero is > introduced. I think this should be moved ahead of the series, so that > the patch that introduces rsvd_zero can add the check in > lapic_check_hidden(). I'll give that a whirl. > >> --- >> xen/arch/x86/hvm/vlapic.c | 41 ++++++++++++++++++++++++++++----------- >> 1 file changed, 30 insertions(+), 11 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c >> index 8a244100009c..2f06bff1b2cc 100644 >> --- a/xen/arch/x86/hvm/vlapic.c >> +++ b/xen/arch/x86/hvm/vlapic.c >> @@ -1573,35 +1573,54 @@ static void lapic_load_fixup(struct vlapic *vlapic) >> v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); >> } >> >> -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) >> +static int cf_check lapic_check_hidden(const struct domain *d, >> + hvm_domain_context_t *h) >> { >> unsigned int vcpuid = hvm_load_instance(h); >> - struct vcpu *v; >> - struct vlapic *s; >> + struct hvm_hw_lapic s; >> >> if ( !has_vlapic(d) ) >> return -ENODEV; >> >> /* Which vlapic to load? */ >> - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) >> + if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ) >> { >> dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", >> d->domain_id, vcpuid); >> return -EINVAL; >> } >> - s = vcpu_vlapic(v); >> >> - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) >> + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) ) > > Can't you use hvm_get_entry() to perform the sanity checks: > > const struct hvm_hw_lapic *s = hvm_get_entry(LAPIC, h); > > Thanks, Roger. I don't think I can. Because the last field (rsvd_zero) might or might not be there, so it needs to be zero-extended. Unless I misunderstood what hvm_get_entry() is meant to do. It seems to check for exact sizes. Cheers, Alejandro
On Fri, May 24, 2024 at 12:16:00PM +0100, Alejandro Vallejo wrote: > On 23/05/2024 15:50, Roger Pau Monné wrote: > > On Wed, May 08, 2024 at 01:39:22PM +0100, Alejandro Vallejo wrote: > >> While at it, add a check for the reserved field in the hidden save area. > >> > >> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > >> --- > >> v2: > >> * New patch. Addresses the missing check for rsvd_zero in v1. > > > > Oh, it would be better if this was done at the time when rsvd_zero is > > introduced. I think this should be moved ahead of the series, so that > > the patch that introduces rsvd_zero can add the check in > > lapic_check_hidden(). > > I'll give that a whirl. > > > > >> --- > >> xen/arch/x86/hvm/vlapic.c | 41 ++++++++++++++++++++++++++++----------- > >> 1 file changed, 30 insertions(+), 11 deletions(-) > >> > >> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > >> index 8a244100009c..2f06bff1b2cc 100644 > >> --- a/xen/arch/x86/hvm/vlapic.c > >> +++ b/xen/arch/x86/hvm/vlapic.c > >> @@ -1573,35 +1573,54 @@ static void lapic_load_fixup(struct vlapic *vlapic) > >> v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); > >> } > >> > >> -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) > >> +static int cf_check lapic_check_hidden(const struct domain *d, > >> + hvm_domain_context_t *h) > >> { > >> unsigned int vcpuid = hvm_load_instance(h); > >> - struct vcpu *v; > >> - struct vlapic *s; > >> + struct hvm_hw_lapic s; > >> > >> if ( !has_vlapic(d) ) > >> return -ENODEV; > >> > >> /* Which vlapic to load? */ > >> - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > >> + if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ) > >> { > >> dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", > >> d->domain_id, vcpuid); > >> return -EINVAL; > >> } > >> - s = vcpu_vlapic(v); > >> > >> - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) > >> + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) ) > > > > Can't you use hvm_get_entry() to perform the sanity checks: > > > > const struct hvm_hw_lapic *s = hvm_get_entry(LAPIC, h); > > > > Thanks, Roger. > > I don't think I can. Because the last field (rsvd_zero) might or might > not be there, so it needs to be zero-extended. Unless I misunderstood > what hvm_get_entry() is meant to do. It seems to check for exact sizes. Oh, indeed, hvm_get_entry() uses strict checking and will refuse to return the entry if sizes don't match. There seems to be no way to avoid the copy if we want to do this in a sane way. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 8a244100009c..2f06bff1b2cc 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1573,35 +1573,54 @@ static void lapic_load_fixup(struct vlapic *vlapic) v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); } -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) +static int cf_check lapic_check_hidden(const struct domain *d, + hvm_domain_context_t *h) { unsigned int vcpuid = hvm_load_instance(h); - struct vcpu *v; - struct vlapic *s; + struct hvm_hw_lapic s; if ( !has_vlapic(d) ) return -ENODEV; /* Which vlapic to load? */ - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) + if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ) { dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", d->domain_id, vcpuid); return -EINVAL; } - s = vcpu_vlapic(v); - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) ) + return -ENODATA; + + /* EN=0 with EXTD=1 is illegal */ + if ( (s.apic_base_msr & (APIC_BASE_ENABLE | APIC_BASE_EXTD)) == + APIC_BASE_EXTD ) + return -EINVAL; + + /* + * Fail migrations from newer versions of Xen where + * rsvd_zero is interpreted as something else. + */ + if ( s.rsvd_zero ) return -EINVAL; + return 0; +} + +static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) +{ + unsigned int vcpuid = hvm_load_instance(h); + struct vcpu *v = d->vcpu[vcpuid]; + struct vlapic *s = vcpu_vlapic(v); + + if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) + BUG(); + s->loaded.hw = 1; if ( s->loaded.regs ) lapic_load_fixup(s); - if ( !(s->hw.apic_base_msr & APIC_BASE_ENABLE) && - unlikely(vlapic_x2apic_mode(s)) ) - return -EINVAL; - hvm_update_vlapic_mode(v); return 0; @@ -1643,7 +1662,7 @@ static int cf_check lapic_load_regs(struct domain *d, hvm_domain_context_t *h) return 0; } -HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, NULL, +HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, lapic_check_hidden, lapic_load_hidden, 1, HVMSR_PER_VCPU); HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, NULL, lapic_load_regs, 1, HVMSR_PER_VCPU);
While at it, add a check for the reserved field in the hidden save area. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v2: * New patch. Addresses the missing check for rsvd_zero in v1. --- xen/arch/x86/hvm/vlapic.c | 41 ++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-)