Re: [PATCH v2 3/8] x86/vlapic: Move lapic_load_hidden migration checks to the check hook
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
> >> ---
> >> 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 8a24419c..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, >hw) != 0 )
> >> +if ( hvm_load_entry_zeroextend(LAPIC, h, ) )
> >
> > 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.
Re: [PATCH v2 3/8] x86/vlapic: Move lapic_load_hidden migration checks to the check hook
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
>> ---
>> 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 8a24419c..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, >hw) != 0 )
>> +if ( hvm_load_entry_zeroextend(LAPIC, h, ) )
>
> 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
Re: [PATCH v2 3/8] x86/vlapic: Move lapic_load_hidden migration checks to the check hook
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 8a24419c..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, >hw) != 0 )
> > +if ( hvm_load_entry_zeroextend(LAPIC, h, ) )
> > +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.
Re: [PATCH v2 3/8] x86/vlapic: Move lapic_load_hidden migration checks to the check hook
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 8a24419c..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, >hw) != 0 )
> +if ( hvm_load_entry_zeroextend(LAPIC, h, ) )
> +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
Re: [PATCH v2 3/8] x86/vlapic: Move lapic_load_hidden migration checks to the check hook
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
> ---
> 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 8a24419c..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, >hw) != 0 )
> +if ( hvm_load_entry_zeroextend(LAPIC, h, ) )
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.
