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.
