Re: [PATCH v2 3/8] x86/vlapic: Move lapic_load_hidden migration checks to the check hook

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.