[PATCH 0/2] KVM: arm64: vgic-v3: Missing check for redist region above the VM IPA size

[PATCH 0/2] KVM: arm64: vgic-v3: Missing check for redist region above the VM IPA size

[Ricardo Koller]

KVM doesn't check for redist regions that extend partially above the
VM-specified IPA (phys_size).  This can happen when using the
KVM_VGIC_V3_ADDR_TYPE_REDIST attribute to set a new region that extends
partially above phys_size (with the base below phys_size).  The issue is that
vcpus can potentially run into a situation where some redistributors are
addressable and others are not.

Patch 1 adds the missing check, and patch 2 adds a test into aarch64/vgic_init.

Ricardo Koller (2):
  KVM: arm64: vgic: check redist region is not above the VM IPA size
  KVM: arm64: selftests: test for vgic redist above the VM IPA size

 arch/arm64/kvm/vgic/vgic-v3.c                 |  4 ++
 .../testing/selftests/kvm/aarch64/vgic_init.c | 44 +++++++++++++++++++
 2 files changed, 48 insertions(+)

-- 
2.33.0.153.gba50c8fa24-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Ricardo Koller]

Extend vgic_v3_check_base() to verify that the redistributor regions
don't go above the VM-specified IPA size (phys_size). This can happen
when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:

  base + size > phys_size AND base < phys_size

vgic_v3_check_base() is used to check the redist regions bases when
setting them (with the vcpus added so far) and when attempting the first
vcpu-run.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 66004f61cd83..5afd9f6f68f6 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
 		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
 			rdreg->base)
 			return false;
+
+		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
+			kvm_phys_size(kvm))
+			return false;
 	}
 
 	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
-- 
2.33.0.153.gba50c8fa24-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Oliver Upton]

Hi Ricardo,

On Wed, Sep 08, 2021 at 02:03:19PM -0700, Ricardo Koller wrote:
> Extend vgic_v3_check_base() to verify that the redistributor regions
> don't go above the VM-specified IPA size (phys_size). This can happen
> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
> 
>   base + size > phys_size AND base < phys_size
> 
> vgic_v3_check_base() is used to check the redist regions bases when
> setting them (with the vcpus added so far) and when attempting the first
> vcpu-run.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 66004f61cd83..5afd9f6f68f6 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>  			rdreg->base)
>  			return false;

Can we drop this check in favor of explicitly comparing rdreg->base with
kvm_phys_size()? I believe that would be more readable.

> +
> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> +			kvm_phys_size(kvm))
> +			return false;
>  	}
>  
>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
> -- 
> 2.33.0.153.gba50c8fa24-goog
> 

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Ricardo Koller]

On Wed, Sep 08, 2021 at 09:32:05PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> On Wed, Sep 08, 2021 at 02:03:19PM -0700, Ricardo Koller wrote:
> > Extend vgic_v3_check_base() to verify that the redistributor regions
> > don't go above the VM-specified IPA size (phys_size). This can happen
> > when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
> > 
> >   base + size > phys_size AND base < phys_size
> > 
> > vgic_v3_check_base() is used to check the redist regions bases when
> > setting them (with the vcpus added so far) and when attempting the first
> > vcpu-run.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> > index 66004f61cd83..5afd9f6f68f6 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> > @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> >  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> >  			rdreg->base)
> >  			return false;
> 
> Can we drop this check in favor of explicitly comparing rdreg->base with
> kvm_phys_size()? I believe that would be more readable.
>

You mean the integer overflow check above? in that case, I would prefer
to leave it, if you don't mind. It seems that this type of check is used
in some other places in KVM (like kvm_assign_ioeventfd and
vgic_v3_alloc_redist_region).

> > +
> > +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> > +			kvm_phys_size(kvm))
> > +			return false;
> >  	}
> >  
> >  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
> > -- 
> > 2.33.0.153.gba50c8fa24-goog
> > 
> 
> --
> Thanks,
> Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Oliver Upton]

On Wed, Sep 8, 2021 at 5:50 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Sep 08, 2021 at 09:32:05PM +0000, Oliver Upton wrote:
> > Hi Ricardo,
> >
> > On Wed, Sep 08, 2021 at 02:03:19PM -0700, Ricardo Koller wrote:
> > > Extend vgic_v3_check_base() to verify that the redistributor regions
> > > don't go above the VM-specified IPA size (phys_size). This can happen
> > > when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
> > >
> > >   base + size > phys_size AND base < phys_size
> > >
> > > vgic_v3_check_base() is used to check the redist regions bases when
> > > setting them (with the vcpus added so far) and when attempting the first
> > > vcpu-run.
> > >
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> > > index 66004f61cd83..5afd9f6f68f6 100644
> > > --- a/arch/arm64/kvm/vgic/vgic-v3.c
> > > +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> > > @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> > >             if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> > >                     rdreg->base)
> > >                     return false;
> >
> > Can we drop this check in favor of explicitly comparing rdreg->base with
> > kvm_phys_size()? I believe that would be more readable.
> >
>
> You mean the integer overflow check above? in that case, I would prefer
> to leave it, if you don't mind. It seems that this type of check is used
> in some other places in KVM (like kvm_assign_ioeventfd and
> vgic_v3_alloc_redist_region).

I would expect rdreg->base to exceed kvm_phys_size() before wrapping,
but we do derive rdreg->count from what userspace gives us. In that
case, your addition in combination with this makes sense, so no real
objections here.

> > > +
> > > +           if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> > > +                   kvm_phys_size(kvm))
> > > +                   return false;
> > >     }
> > >
> > >     if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
> > > --
> > > 2.33.0.153.gba50c8fa24-goog
> > >
> >
> > --
> > Thanks,
> > Oliver

Reviewed-by: Oliver Upton <oupton@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Alexandru Elisei]

Hi Ricardo,

On 9/8/21 10:03 PM, Ricardo Koller wrote:
> Extend vgic_v3_check_base() to verify that the redistributor regions
> don't go above the VM-specified IPA size (phys_size). This can happen
> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
>
>   base + size > phys_size AND base < phys_size
>
> vgic_v3_check_base() is used to check the redist regions bases when
> setting them (with the vcpus added so far) and when attempting the first
> vcpu-run.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 66004f61cd83..5afd9f6f68f6 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>  			rdreg->base)
>  			return false;
> +
> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> +			kvm_phys_size(kvm))
> +			return false;

Looks to me like this same check (and the overflow one before it) is done when
adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
-> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
kvm_vgic_addr() handles both ways of setting the Redistributor address.

Without this patch, did you manage to set a base address such that base + size >
kvm_phys_size()?

Thanks,

Alex

>  	}
>  
>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Eric Auger]

Hi,

On 9/9/21 12:20 PM, Alexandru Elisei wrote:
> Hi Ricardo,
>
> On 9/8/21 10:03 PM, Ricardo Koller wrote:
>> Extend vgic_v3_check_base() to verify that the redistributor regions
>> don't go above the VM-specified IPA size (phys_size). This can happen
>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
>>
>>   base + size > phys_size AND base < phys_size
>>
>> vgic_v3_check_base() is used to check the redist regions bases when
>> setting them (with the vcpus added so far) and when attempting the first
>> vcpu-run.
>>
>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>> ---
>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>> index 66004f61cd83..5afd9f6f68f6 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>>  			rdreg->base)
>>  			return false;
>> +
>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
>> +			kvm_phys_size(kvm))
>> +			return false;
> Looks to me like this same check (and the overflow one before it) is done when
> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
> kvm_vgic_addr() handles both ways of setting the Redistributor address.
To me vgic_check_ioaddr() does check the base addr but not the end addr.
So looks this fix is needed.
As I commented on the selftest patch, I think you should double check
your fix also handles the KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION case.

In vgic_v3_alloc_redist_region(), in this later case, we know the number
of redistributors in the region (count), so it would be easy to check
the end addr. But I think this would be a duplicate of your new check as
vgic_v3_check_base() also gets called in vgic_register_redist_iodev().
But better to check it ;-)

Thanks

Eric
>
> Without this patch, did you manage to set a base address such that base + size >
> kvm_phys_size()?
>
> Thanks,
>
> Alex
>
>>  	}
>>  
>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Ricardo Koller]

On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
> Hi Ricardo,
> 
> On 9/8/21 10:03 PM, Ricardo Koller wrote:
> > Extend vgic_v3_check_base() to verify that the redistributor regions
> > don't go above the VM-specified IPA size (phys_size). This can happen
> > when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
> >
> >   base + size > phys_size AND base < phys_size
> >
> > vgic_v3_check_base() is used to check the redist regions bases when
> > setting them (with the vcpus added so far) and when attempting the first
> > vcpu-run.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> > index 66004f61cd83..5afd9f6f68f6 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> > @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> >  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> >  			rdreg->base)
> >  			return false;
> > +
> > +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> > +			kvm_phys_size(kvm))
> > +			return false;
> 
> Looks to me like this same check (and the overflow one before it) is done when
> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
> kvm_vgic_addr() handles both ways of setting the Redistributor address.
> 
> Without this patch, did you manage to set a base address such that base + size >
> kvm_phys_size()?
> 

Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
to get to this situation is with the selftest in patch 2.  I then tried
an extra experiment: map the first redistributor, run the first vcpu,
and access the redist from inside the guest. KVM didn't complain in any
of these steps.

Thanks,
Ricardo

> Thanks,
> 
> Alex
> 
> >  	}
> >  
> >  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Alexandru Elisei]

Hi Ricardo,

On 9/9/21 5:47 PM, Ricardo Koller wrote:
> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
>> Hi Ricardo,
>>
>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
>>> Extend vgic_v3_check_base() to verify that the redistributor regions
>>> don't go above the VM-specified IPA size (phys_size). This can happen
>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
>>>
>>>   base + size > phys_size AND base < phys_size
>>>
>>> vgic_v3_check_base() is used to check the redist regions bases when
>>> setting them (with the vcpus added so far) and when attempting the first
>>> vcpu-run.
>>>
>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>> index 66004f61cd83..5afd9f6f68f6 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>>>  			rdreg->base)
>>>  			return false;
>>> +
>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
>>> +			kvm_phys_size(kvm))
>>> +			return false;
>> Looks to me like this same check (and the overflow one before it) is done when
>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
>>
>> Without this patch, did you manage to set a base address such that base + size >
>> kvm_phys_size()?
>>
> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
> to get to this situation is with the selftest in patch 2.  I then tried
> an extra experiment: map the first redistributor, run the first vcpu,
> and access the redist from inside the guest. KVM didn't complain in any
> of these steps.

Yes, Eric pointed out that I was mistaken and there is no check being done for
base + size > kvm_phys_size().

What I was trying to say is that this check is better done when the user creates a
Redistributor region, not when a VCPU is first run. We have everything we need to
make the check when a region is created, why wait until the VCPU is run?

For example, vgic_v3_insert_redist_region() is called each time the adds a new
Redistributor region (via either of the two APIs), and already has a check for the
upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
add the check against the maximum IPA size there.

Also, because vgic_v3_insert_redist_region() already checks for overflow, I
believe the overflow check in vgic_v3_check_base() is redundant.

As far as I can tell, vgic_v3_check_base() is there to make sure that the
Distributor doesn't overlap with any of the Redistributors, and because the
Redistributors and the Distributor can be created in any order, we defer the check
until the first VCPU is run. I might be wrong about this, someone please correct
me if I'm wrong.

Also, did you verify that KVM is also doing this check for GICv2? KVM does
something similar and calls vgic_v2_check_base() when mapping the GIC resources,
and I don't see a check for the maximum IPA size in that function either.

Thanks,

Alex

>
> Thanks,
> Ricardo
>
>> Thanks,
>>
>> Alex
>>
>>>  	}
>>>  
>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Eric Auger]

Hi Alexandru,

On 9/10/21 10:28 AM, Alexandru Elisei wrote:
> Hi Ricardo,
>
> On 9/9/21 5:47 PM, Ricardo Koller wrote:
>> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
>>> Hi Ricardo,
>>>
>>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
>>>> Extend vgic_v3_check_base() to verify that the redistributor regions
>>>> don't go above the VM-specified IPA size (phys_size). This can happen
>>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
>>>>
>>>>   base + size > phys_size AND base < phys_size
>>>>
>>>> vgic_v3_check_base() is used to check the redist regions bases when
>>>> setting them (with the vcpus added so far) and when attempting the first
>>>> vcpu-run.
>>>>
>>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>>> ---
>>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>>> index 66004f61cd83..5afd9f6f68f6 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>>>>  			rdreg->base)
>>>>  			return false;
>>>> +
>>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
>>>> +			kvm_phys_size(kvm))
>>>> +			return false;
>>> Looks to me like this same check (and the overflow one before it) is done when
>>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
>>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
>>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
>>>
>>> Without this patch, did you manage to set a base address such that base + size >
>>> kvm_phys_size()?
>>>
>> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
>> to get to this situation is with the selftest in patch 2.  I then tried
>> an extra experiment: map the first redistributor, run the first vcpu,
>> and access the redist from inside the guest. KVM didn't complain in any
>> of these steps.
> Yes, Eric pointed out that I was mistaken and there is no check being done for
> base + size > kvm_phys_size().
>
> What I was trying to say is that this check is better done when the user creates a
> Redistributor region, not when a VCPU is first run. We have everything we need to
> make the check when a region is created, why wait until the VCPU is run?
>
> For example, vgic_v3_insert_redist_region() is called each time the adds a new
> Redistributor region (via either of the two APIs), and already has a check for the
> upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
> add the check against the maximum IPA size there.
you seem to refer to an old kernel as vgic_v3_insert_redist_region was
renamed into  vgic_v3_alloc_redist_region in
e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()

I think in case you use the old rdist API you do not know yet the size
of the redist region at this point (count=0), hence Ricardo's choice to
do the check latter.
>
> Also, because vgic_v3_insert_redist_region() already checks for overflow, I
> believe the overflow check in vgic_v3_check_base() is redundant.
>
> As far as I can tell, vgic_v3_check_base() is there to make sure that the
> Distributor doesn't overlap with any of the Redistributors, and because the
> Redistributors and the Distributor can be created in any order, we defer the check
> until the first VCPU is run. I might be wrong about this, someone please correct
> me if I'm wrong.
>
> Also, did you verify that KVM is also doing this check for GICv2? KVM does
> something similar and calls vgic_v2_check_base() when mapping the GIC resources,
> and I don't see a check for the maximum IPA size in that function either.

I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
checks the base @)

Thanks

Eric
>
> Thanks,
>
> Alex
>
>> Thanks,
>> Ricardo
>>
>>> Thanks,
>>>
>>> Alex
>>>
>>>>  	}
>>>>  
>>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Ricardo Koller]

Hi Alexandru and Eric,

On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote:
> Hi Alexandru,
> 
> On 9/10/21 10:28 AM, Alexandru Elisei wrote:
> > Hi Ricardo,
> >
> > On 9/9/21 5:47 PM, Ricardo Koller wrote:
> >> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
> >>> Hi Ricardo,
> >>>
> >>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
> >>>> Extend vgic_v3_check_base() to verify that the redistributor regions
> >>>> don't go above the VM-specified IPA size (phys_size). This can happen
> >>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
> >>>>
> >>>>   base + size > phys_size AND base < phys_size
> >>>>
> >>>> vgic_v3_check_base() is used to check the redist regions bases when
> >>>> setting them (with the vcpus added so far) and when attempting the first
> >>>> vcpu-run.
> >>>>
> >>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> >>>> ---
> >>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> >>>> index 66004f61cd83..5afd9f6f68f6 100644
> >>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> >>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> >>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> >>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> >>>>  			rdreg->base)
> >>>>  			return false;
> >>>> +
> >>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> >>>> +			kvm_phys_size(kvm))
> >>>> +			return false;
> >>> Looks to me like this same check (and the overflow one before it) is done when
> >>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
> >>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
> >>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
> >>>
> >>> Without this patch, did you manage to set a base address such that base + size >
> >>> kvm_phys_size()?
> >>>
> >> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
> >> to get to this situation is with the selftest in patch 2.  I then tried
> >> an extra experiment: map the first redistributor, run the first vcpu,
> >> and access the redist from inside the guest. KVM didn't complain in any
> >> of these steps.
> > Yes, Eric pointed out that I was mistaken and there is no check being done for
> > base + size > kvm_phys_size().
> >
> > What I was trying to say is that this check is better done when the user creates a
> > Redistributor region, not when a VCPU is first run. We have everything we need to
> > make the check when a region is created, why wait until the VCPU is run?
> >
> > For example, vgic_v3_insert_redist_region() is called each time the adds a new
> > Redistributor region (via either of the two APIs), and already has a check for the
> > upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
> > add the check against the maximum IPA size there.
> you seem to refer to an old kernel as vgic_v3_insert_redist_region was
> renamed into  vgic_v3_alloc_redist_region in
> e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
> 
> I think in case you use the old rdist API you do not know yet the size
> of the redist region at this point (count=0), hence Ricardo's choice to
> do the check latter.

Just wanted to add one more detail. vgic_v3_check_base() is also called
when creating the redistributor region (via vgic_v3_set_redist_base ->
vgic_register_redist_iodev). This patch reuses that check for the old
redist API to also check for "base + size > kvm_phys_size()" with a size
calculated using the vcpus added so far.

> >
> > Also, because vgic_v3_insert_redist_region() already checks for overflow, I
> > believe the overflow check in vgic_v3_check_base() is redundant.
> >

It's redundant for the new redist API, but still needed for the old
redist API.

> > As far as I can tell, vgic_v3_check_base() is there to make sure that the
> > Distributor doesn't overlap with any of the Redistributors, and because the
> > Redistributors and the Distributor can be created in any order, we defer the check
> > until the first VCPU is run. I might be wrong about this, someone please correct
> > me if I'm wrong.
> >
> > Also, did you verify that KVM is also doing this check for GICv2? KVM does
> > something similar and calls vgic_v2_check_base() when mapping the GIC resources,
> > and I don't see a check for the maximum IPA size in that function either.
> 
> I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
> checks the base @)
>

It seems that GICv2 suffers from the same problem. The cpu interface
base is checked but the end can extend above IPA size. Note that the cpu
interface is 8KBs and vgic_check_ioaddr() is only checking that its base
is 4KB aligned and below IPA size. The distributor region is 4KB so
vgic_check_ioaddr() is enough in that case.

What about the following?

I can work on the next version of this patch (v2 has the GICv2 issue)
which adds vgic_check_range(), which is like vgic_check_ioaddr() but
with a size arg.  kvm_vgic_addr() can then call vgic_check_range() and
do all the checks for GICv2 and GICv3. Note that for GICv2, there's no
need to wait until first vcpu run to do the check. Also note that I will
have to keep the change in vgic_v3_check_base() to check for the old v3
redist API at first vcpu run.

Thanks,
Ricardo

> Thanks
> 
> Eric
> >
> > Thanks,
> >
> > Alex
> >
> >> Thanks,
> >> Ricardo
> >>
> >>> Thanks,
> >>>
> >>> Alex
> >>>
> >>>>  	}
> >>>>  
> >>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Eric Auger]

Hi Ricardo,

On 9/10/21 9:32 PM, Ricardo Koller wrote:
> Hi Alexandru and Eric,
>
> On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote:
>> Hi Alexandru,
>>
>> On 9/10/21 10:28 AM, Alexandru Elisei wrote:
>>> Hi Ricardo,
>>>
>>> On 9/9/21 5:47 PM, Ricardo Koller wrote:
>>>> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
>>>>> Hi Ricardo,
>>>>>
>>>>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
>>>>>> Extend vgic_v3_check_base() to verify that the redistributor regions
>>>>>> don't go above the VM-specified IPA size (phys_size). This can happen
>>>>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
>>>>>>
>>>>>>   base + size > phys_size AND base < phys_size
>>>>>>
>>>>>> vgic_v3_check_base() is used to check the redist regions bases when
>>>>>> setting them (with the vcpus added so far) and when attempting the first
>>>>>> vcpu-run.
>>>>>>
>>>>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>>>>> ---
>>>>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>> index 66004f61cd83..5afd9f6f68f6 100644
>>>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>>>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>>>>>>  			rdreg->base)
>>>>>>  			return false;
>>>>>> +
>>>>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
>>>>>> +			kvm_phys_size(kvm))
>>>>>> +			return false;
>>>>> Looks to me like this same check (and the overflow one before it) is done when
>>>>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
>>>>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
>>>>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
>>>>>
>>>>> Without this patch, did you manage to set a base address such that base + size >
>>>>> kvm_phys_size()?
>>>>>
>>>> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
>>>> to get to this situation is with the selftest in patch 2.  I then tried
>>>> an extra experiment: map the first redistributor, run the first vcpu,
>>>> and access the redist from inside the guest. KVM didn't complain in any
>>>> of these steps.
>>> Yes, Eric pointed out that I was mistaken and there is no check being done for
>>> base + size > kvm_phys_size().
>>>
>>> What I was trying to say is that this check is better done when the user creates a
>>> Redistributor region, not when a VCPU is first run. We have everything we need to
>>> make the check when a region is created, why wait until the VCPU is run?
>>>
>>> For example, vgic_v3_insert_redist_region() is called each time the adds a new
>>> Redistributor region (via either of the two APIs), and already has a check for the
>>> upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
>>> add the check against the maximum IPA size there.
>> you seem to refer to an old kernel as vgic_v3_insert_redist_region was
>> renamed into  vgic_v3_alloc_redist_region in
>> e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
>>
>> I think in case you use the old rdist API you do not know yet the size
>> of the redist region at this point (count=0), hence Ricardo's choice to
>> do the check latter.
> Just wanted to add one more detail. vgic_v3_check_base() is also called
> when creating the redistributor region (via vgic_v3_set_redist_base ->
> vgic_register_redist_iodev). This patch reuses that check for the old
> redist API to also check for "base + size > kvm_phys_size()" with a size
> calculated using the vcpus added so far.
>
>>> Also, because vgic_v3_insert_redist_region() already checks for overflow, I
>>> believe the overflow check in vgic_v3_check_base() is redundant.
>>>
> It's redundant for the new redist API, but still needed for the old
> redist API.
>
>>> As far as I can tell, vgic_v3_check_base() is there to make sure that the
>>> Distributor doesn't overlap with any of the Redistributors, and because the
>>> Redistributors and the Distributor can be created in any order, we defer the check
>>> until the first VCPU is run. I might be wrong about this, someone please correct
>>> me if I'm wrong.
>>>
>>> Also, did you verify that KVM is also doing this check for GICv2? KVM does
>>> something similar and calls vgic_v2_check_base() when mapping the GIC resources,
>>> and I don't see a check for the maximum IPA size in that function either.
>> I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
>> checks the base @)
>>
> It seems that GICv2 suffers from the same problem. The cpu interface
> base is checked but the end can extend above IPA size. Note that the cpu
> interface is 8KBs and vgic_check_ioaddr() is only checking that its base
I missed this 8kB thingy.
> is 4KB aligned and below IPA size. The distributor region is 4KB so
> vgic_check_ioaddr() is enough in that case.
>
> What about the following?
>
> I can work on the next version of this patch (v2 has the GICv2 issue)
> which adds vgic_check_range(), which is like vgic_check_ioaddr() but
> with a size arg.  kvm_vgic_addr() can then call vgic_check_range() and
> do all the checks for GICv2 and GICv3. Note that for GICv2, there's no
> need to wait until first vcpu run to do the check. Also note that I will
> have to keep the change in vgic_v3_check_base() to check for the old v3
> redist API at first vcpu run.
Looking forward to reviewing it.

Thanks

Eric
>
> Thanks,
> Ricardo
>
>> Thanks
>>
>> Eric
>>> Thanks,
>>>
>>> Alex
>>>
>>>> Thanks,
>>>> Ricardo
>>>>
>>>>> Thanks,
>>>>>
>>>>> Alex
>>>>>
>>>>>>  	}
>>>>>>  
>>>>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Alexandru Elisei]

Hi Eric, Ricardo,

On 9/10/21 20:32, Ricardo Koller wrote:
> Hi Alexandru and Eric,
>
> On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote:
>> Hi Alexandru,
>>
>> On 9/10/21 10:28 AM, Alexandru Elisei wrote:
>>> Hi Ricardo,
>>>
>>> On 9/9/21 5:47 PM, Ricardo Koller wrote:
>>>> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
>>>>> Hi Ricardo,
>>>>>
>>>>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
>>>>>> Extend vgic_v3_check_base() to verify that the redistributor regions
>>>>>> don't go above the VM-specified IPA size (phys_size). This can happen
>>>>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
>>>>>>
>>>>>>   base + size > phys_size AND base < phys_size
>>>>>>
>>>>>> vgic_v3_check_base() is used to check the redist regions bases when
>>>>>> setting them (with the vcpus added so far) and when attempting the first
>>>>>> vcpu-run.
>>>>>>
>>>>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>>>>> ---
>>>>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>> index 66004f61cd83..5afd9f6f68f6 100644
>>>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>>>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>>>>>>  			rdreg->base)
>>>>>>  			return false;
>>>>>> +
>>>>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
>>>>>> +			kvm_phys_size(kvm))
>>>>>> +			return false;
>>>>> Looks to me like this same check (and the overflow one before it) is done when
>>>>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
>>>>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
>>>>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
>>>>>
>>>>> Without this patch, did you manage to set a base address such that base + size >
>>>>> kvm_phys_size()?
>>>>>
>>>> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
>>>> to get to this situation is with the selftest in patch 2.  I then tried
>>>> an extra experiment: map the first redistributor, run the first vcpu,
>>>> and access the redist from inside the guest. KVM didn't complain in any
>>>> of these steps.
>>> Yes, Eric pointed out that I was mistaken and there is no check being done for
>>> base + size > kvm_phys_size().
>>>
>>> What I was trying to say is that this check is better done when the user creates a
>>> Redistributor region, not when a VCPU is first run. We have everything we need to
>>> make the check when a region is created, why wait until the VCPU is run?
>>>
>>> For example, vgic_v3_insert_redist_region() is called each time the adds a new
>>> Redistributor region (via either of the two APIs), and already has a check for the
>>> upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
>>> add the check against the maximum IPA size there.
>> you seem to refer to an old kernel as vgic_v3_insert_redist_region was
>> renamed into� vgic_v3_alloc_redist_region in
>> e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
>>
>> I think in case you use the old rdist API you do not know yet the size
>> of the redist region at this point (count=0), hence Ricardo's choice to
>> do the check latter.
> Just wanted to add one more detail. vgic_v3_check_base() is also called
> when creating the redistributor region (via vgic_v3_set_redist_base ->
> vgic_register_redist_iodev). This patch reuses that check for the old
> redist API to also check for "base + size > kvm_phys_size()" with a size
> calculated using the vcpus added so far.

@Eric: Indeed I was looking at an older kernel by mistake, thank you for pointing
that out!

Thank you both for the explanations, the piece I was missing was the fact that
KVM_VGIC_V3_ADDR_TYPE_REDIST specifies only the base address and the limit for the
region is the number of VCPUs * (KVM_VGIC_V3_REDIST_SIZE = 128K), which makes it
necessary to have the check when each VCPU is first run (as far as I can tell,
VCPUs can be created at any time).

>
>>> Also, because vgic_v3_insert_redist_region() already checks for overflow, I
>>> believe the overflow check in vgic_v3_check_base() is redundant.
>>>
> It's redundant for the new redist API, but still needed for the old
> redist API.

Indeed.

>
>>> As far as I can tell, vgic_v3_check_base() is there to make sure that the
>>> Distributor doesn't overlap with any of the Redistributors, and because the
>>> Redistributors and the Distributor can be created in any order, we defer the check
>>> until the first VCPU is run. I might be wrong about this, someone please correct
>>> me if I'm wrong.
>>>
>>> Also, did you verify that KVM is also doing this check for GICv2? KVM does
>>> something similar and calls vgic_v2_check_base() when mapping the GIC resources,
>>> and I don't see a check for the maximum IPA size in that function either.
>> I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
>> checks the base @)
>>
> It seems that GICv2 suffers from the same problem. The cpu interface
> base is checked but the end can extend above IPA size. Note that the cpu
> interface is 8KBs and vgic_check_ioaddr() is only checking that its base

... except that the doc for KVM_VGIC_V2_ADDR_TYPE_CPU says that the CPU interface
region is 4K, while the check in vgic_v2_check_base() is done against
KVM_VGIC_V2_CPU_SIZE, which is 8K. I suppose that the CPU interface region is 8K
because ARM IHI 0048B.b strongly recommends that the virtual CPU interface control
registers are in a separate 4KB region, and KVM wants to emulate a GICv2 as close
to the real thing as possible?

> is 4KB aligned and below IPA size. The distributor region is 4KB so
> vgic_check_ioaddr() is enough in that case.
>
> What about the following?
>
> I can work on the next version of this patch (v2 has the GICv2 issue)
> which adds vgic_check_range(), which is like vgic_check_ioaddr() but
> with a size arg.  kvm_vgic_addr() can then call vgic_check_range() and
> do all the checks for GICv2 and GICv3. Note that for GICv2, there's no
> need to wait until first vcpu run to do the check. Also note that I will
> have to keep the change in vgic_v3_check_base() to check for the old v3
> redist API at first vcpu run.

Sounds good.

Thanks,

Alex

>
> Thanks,
> Ricardo
>
>> Thanks
>>
>> Eric
>>> Thanks,
>>>
>>> Alex
>>>
>>>> Thanks,
>>>> Ricardo
>>>>
>>>>> Thanks,
>>>>>
>>>>> Alex
>>>>>
>>>>>>  	}
>>>>>>  
>>>>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Ricardo Koller]

Hi Alexandru, Eric,

On Mon, Sep 13, 2021 at 11:15:33AM +0100, Alexandru Elisei wrote:
> Hi Eric, Ricardo,
> 
> On 9/10/21 20:32, Ricardo Koller wrote:
> > Hi Alexandru and Eric,
> >
> > On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote:
> >> Hi Alexandru,
> >>
> >> On 9/10/21 10:28 AM, Alexandru Elisei wrote:
> >>> Hi Ricardo,
> >>>
> >>> On 9/9/21 5:47 PM, Ricardo Koller wrote:
> >>>> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
> >>>>> Hi Ricardo,
> >>>>>
> >>>>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
> >>>>>> Extend vgic_v3_check_base() to verify that the redistributor regions
> >>>>>> don't go above the VM-specified IPA size (phys_size). This can happen
> >>>>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
> >>>>>>
> >>>>>>   base + size > phys_size AND base < phys_size
> >>>>>>
> >>>>>> vgic_v3_check_base() is used to check the redist regions bases when
> >>>>>> setting them (with the vcpus added so far) and when attempting the first
> >>>>>> vcpu-run.
> >>>>>>
> >>>>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> >>>>>> ---
> >>>>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
> >>>>>>  1 file changed, 4 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> >>>>>> index 66004f61cd83..5afd9f6f68f6 100644
> >>>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> >>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> >>>>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> >>>>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> >>>>>>  			rdreg->base)
> >>>>>>  			return false;
> >>>>>> +
> >>>>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> >>>>>> +			kvm_phys_size(kvm))
> >>>>>> +			return false;
> >>>>> Looks to me like this same check (and the overflow one before it) is done when
> >>>>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
> >>>>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
> >>>>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
> >>>>>
> >>>>> Without this patch, did you manage to set a base address such that base + size >
> >>>>> kvm_phys_size()?
> >>>>>
> >>>> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
> >>>> to get to this situation is with the selftest in patch 2.  I then tried
> >>>> an extra experiment: map the first redistributor, run the first vcpu,
> >>>> and access the redist from inside the guest. KVM didn't complain in any
> >>>> of these steps.
> >>> Yes, Eric pointed out that I was mistaken and there is no check being done for
> >>> base + size > kvm_phys_size().
> >>>
> >>> What I was trying to say is that this check is better done when the user creates a
> >>> Redistributor region, not when a VCPU is first run. We have everything we need to
> >>> make the check when a region is created, why wait until the VCPU is run?
> >>>
> >>> For example, vgic_v3_insert_redist_region() is called each time the adds a new
> >>> Redistributor region (via either of the two APIs), and already has a check for the
> >>> upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
> >>> add the check against the maximum IPA size there.
> >> you seem to refer to an old kernel as vgic_v3_insert_redist_region was
> >> renamed into� vgic_v3_alloc_redist_region in
> >> e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
> >>
> >> I think in case you use the old rdist API you do not know yet the size
> >> of the redist region at this point (count=0), hence Ricardo's choice to
> >> do the check latter.
> > Just wanted to add one more detail. vgic_v3_check_base() is also called
> > when creating the redistributor region (via vgic_v3_set_redist_base ->
> > vgic_register_redist_iodev). This patch reuses that check for the old
> > redist API to also check for "base + size > kvm_phys_size()" with a size
> > calculated using the vcpus added so far.
> 
> @Eric: Indeed I was looking at an older kernel by mistake, thank you for pointing
> that out!
> 
> Thank you both for the explanations, the piece I was missing was the fact that
> KVM_VGIC_V3_ADDR_TYPE_REDIST specifies only the base address and the limit for the
> region is the number of VCPUs * (KVM_VGIC_V3_REDIST_SIZE = 128K), which makes it
> necessary to have the check when each VCPU is first run (as far as I can tell,
> VCPUs can be created at any time).
> 
> >
> >>> Also, because vgic_v3_insert_redist_region() already checks for overflow, I
> >>> believe the overflow check in vgic_v3_check_base() is redundant.
> >>>
> > It's redundant for the new redist API, but still needed for the old
> > redist API.
> 
> Indeed.
> 
> >
> >>> As far as I can tell, vgic_v3_check_base() is there to make sure that the
> >>> Distributor doesn't overlap with any of the Redistributors, and because the
> >>> Redistributors and the Distributor can be created in any order, we defer the check
> >>> until the first VCPU is run. I might be wrong about this, someone please correct
> >>> me if I'm wrong.
> >>>
> >>> Also, did you verify that KVM is also doing this check for GICv2? KVM does
> >>> something similar and calls vgic_v2_check_base() when mapping the GIC resources,
> >>> and I don't see a check for the maximum IPA size in that function either.
> >> I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
> >> checks the base @)
> >>
> > It seems that GICv2 suffers from the same problem. The cpu interface
> > base is checked but the end can extend above IPA size. Note that the cpu
> > interface is 8KBs and vgic_check_ioaddr() is only checking that its base
> 
> ... except that the doc for KVM_VGIC_V2_ADDR_TYPE_CPU says that the CPU interface
> region is 4K, while the check in vgic_v2_check_base() is done against
> KVM_VGIC_V2_CPU_SIZE, which is 8K.

The "GIC virtual CPU interface" alone is slightly more than 4K: GICV_DIR
is at 0x1000. The documentation might need to be updated.

> I suppose that the CPU interface region is 8K
> because ARM IHI 0048B.b strongly recommends that the virtual CPU interface control
> registers are in a separate 4KB region, and KVM wants to emulate a GICv2 as close
> to the real thing as possible?

Are the "virtual CPU interface control" registers the ones starting with
GICH_? If yes, then I'm a bit confused, as those are not exposed to the
guest (to my knowledge).

> 
> > is 4KB aligned and below IPA size. The distributor region is 4KB so
> > vgic_check_ioaddr() is enough in that case.
> >
> > What about the following?
> >
> > I can work on the next version of this patch (v2 has the GICv2 issue)
> > which adds vgic_check_range(), which is like vgic_check_ioaddr() but
> > with a size arg.  kvm_vgic_addr() can then call vgic_check_range() and
> > do all the checks for GICv2 and GICv3. Note that for GICv2, there's no
> > need to wait until first vcpu run to do the check. Also note that I will
> > have to keep the change in vgic_v3_check_base() to check for the old v3
> > redist API at first vcpu run.
> 
> Sounds good.
> 
> Thanks,
> 
> Alex
> 
> >
> > Thanks,
> > Ricardo
> >
> >> Thanks
> >>
> >> Eric

Will do, thank you both.

Ricardo

> >>> Thanks,
> >>>
> >>> Alex
> >>>
> >>>> Thanks,
> >>>> Ricardo
> >>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Alex
> >>>>>
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Alexandru Elisei]

Hi Ricardo,

(adding kvm@vger.kernel.org to CC because the email this is a reply to got
rejected because of html content)

On 9/14/21 04:20, Ricardo Koller wrote:
> Hi Alexandru, Eric,
>
> On Mon, Sep 13, 2021 at 11:15:33AM +0100, Alexandru Elisei wrote:
>> Hi Eric, Ricardo,
>>
>> On 9/10/21 20:32, Ricardo Koller wrote:
>>> Hi Alexandru and Eric,
>>>
>>> On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote:
>>>> Hi Alexandru,
>>>>
>>>> On 9/10/21 10:28 AM, Alexandru Elisei wrote:
>>>>> Hi Ricardo,
>>>>>
>>>>> On 9/9/21 5:47 PM, Ricardo Koller wrote:
>>>>>> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
>>>>>>> Hi Ricardo,
>>>>>>>
>>>>>>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
>>>>>>>> Extend vgic_v3_check_base() to verify that the redistributor regions
>>>>>>>> don't go above the VM-specified IPA size (phys_size). This can happen
>>>>>>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
>>>>>>>>
>>>>>>>>   base + size > phys_size AND base < phys_size
>>>>>>>>
>>>>>>>> vgic_v3_check_base() is used to check the redist regions bases when
>>>>>>>> setting them (with the vcpus added so far) and when attempting the first
>>>>>>>> vcpu-run.
>>>>>>>>
>>>>>>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>>>>>>> ---
>>>>>>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>>>> index 66004f61cd83..5afd9f6f68f6 100644
>>>>>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>>>>>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>>>>>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>>>>>>>>  			rdreg->base)
>>>>>>>>  			return false;
>>>>>>>> +
>>>>>>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
>>>>>>>> +			kvm_phys_size(kvm))
>>>>>>>> +			return false;
>>>>>>> Looks to me like this same check (and the overflow one before it) is done when
>>>>>>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
>>>>>>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
>>>>>>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
>>>>>>>
>>>>>>> Without this patch, did you manage to set a base address such that base + size >
>>>>>>> kvm_phys_size()?
>>>>>>>
>>>>>> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
>>>>>> to get to this situation is with the selftest in patch 2.  I then tried
>>>>>> an extra experiment: map the first redistributor, run the first vcpu,
>>>>>> and access the redist from inside the guest. KVM didn't complain in any
>>>>>> of these steps.
>>>>> Yes, Eric pointed out that I was mistaken and there is no check being done for
>>>>> base + size > kvm_phys_size().
>>>>>
>>>>> What I was trying to say is that this check is better done when the user creates a
>>>>> Redistributor region, not when a VCPU is first run. We have everything we need to
>>>>> make the check when a region is created, why wait until the VCPU is run?
>>>>>
>>>>> For example, vgic_v3_insert_redist_region() is called each time the adds a new
>>>>> Redistributor region (via either of the two APIs), and already has a check for the
>>>>> upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
>>>>> add the check against the maximum IPA size there.
>>>> you seem to refer to an old kernel as vgic_v3_insert_redist_region was
>>>> renamed into� vgic_v3_alloc_redist_region in
>>>> e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
>>>>
>>>> I think in case you use the old rdist API you do not know yet the size
>>>> of the redist region at this point (count=0), hence Ricardo's choice to
>>>> do the check latter.
>>> Just wanted to add one more detail. vgic_v3_check_base() is also called
>>> when creating the redistributor region (via vgic_v3_set_redist_base ->
>>> vgic_register_redist_iodev). This patch reuses that check for the old
>>> redist API to also check for "base + size > kvm_phys_size()" with a size
>>> calculated using the vcpus added so far.
>> @Eric: Indeed I was looking at an older kernel by mistake, thank you for pointing
>> that out!
>>
>> Thank you both for the explanations, the piece I was missing was the fact that
>> KVM_VGIC_V3_ADDR_TYPE_REDIST specifies only the base address and the limit for the
>> region is the number of VCPUs * (KVM_VGIC_V3_REDIST_SIZE = 128K), which makes it
>> necessary to have the check when each VCPU is first run (as far as I can tell,
>> VCPUs can be created at any time).
>>
>>>>> Also, because vgic_v3_insert_redist_region() already checks for overflow, I
>>>>> believe the overflow check in vgic_v3_check_base() is redundant.
>>>>>
>>> It's redundant for the new redist API, but still needed for the old
>>> redist API.
>> Indeed.
>>
>>>>> As far as I can tell, vgic_v3_check_base() is there to make sure that the
>>>>> Distributor doesn't overlap with any of the Redistributors, and because the
>>>>> Redistributors and the Distributor can be created in any order, we defer the check
>>>>> until the first VCPU is run. I might be wrong about this, someone please correct
>>>>> me if I'm wrong.
>>>>>
>>>>> Also, did you verify that KVM is also doing this check for GICv2? KVM does
>>>>> something similar and calls vgic_v2_check_base() when mapping the GIC resources,
>>>>> and I don't see a check for the maximum IPA size in that function either.
>>>> I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
>>>> checks the base @)
>>>>
>>> It seems that GICv2 suffers from the same problem. The cpu interface
>>> base is checked but the end can extend above IPA size. Note that the cpu
>>> interface is 8KBs and vgic_check_ioaddr() is only checking that its base
>> ... except that the doc for KVM_VGIC_V2_ADDR_TYPE_CPU says that the CPU interface
>> region is 4K, while the check in vgic_v2_check_base() is done against
>> KVM_VGIC_V2_CPU_SIZE, which is 8K.
> The "GIC virtual CPU interface" alone is slightly more than 4K: GICV_DIR
> is at 0x1000. The documentation might need to be updated.
>
>> I suppose that the CPU interface region is 8K
>> because ARM IHI 0048B.b strongly recommends that the virtual CPU interface control
>> registers are in a separate 4KB region, and KVM wants to emulate a GICv2 as close
>> to the real thing as possible?
> Are the "virtual CPU interface control" registers the ones starting with
> GICH_? If yes, then I'm a bit confused, as those are not exposed to the
> guest (to my knowledge).

Yes, those are the ones, and I also did find that they are not exposed to the guest.

Comparing the KVM documentation with what KVM actually does, I assumed that the
8KB was a forward looking decision, in case nested virtualization will support
GICv2, which means that the GICH_* registers would also have to be exposed. Making
the CPU interface for a CPU 8KB from the start would avoid changes or additions to
the API if that happens.

However, after further digging through the spec, I found that the virtual CPU
interface is specified to be 8KB (Table 5-10 of ARM IHI 0048B.b). I think that's
the reason KVM treats it as 8KB.

Thanks,

Alex

>
>>> is 4KB aligned and below IPA size. The distributor region is 4KB so
>>> vgic_check_ioaddr() is enough in that case.
>>>
>>> What about the following?
>>>
>>> I can work on the next version of this patch (v2 has the GICv2 issue)
>>> which adds vgic_check_range(), which is like vgic_check_ioaddr() but
>>> with a size arg.  kvm_vgic_addr() can then call vgic_check_range() and
>>> do all the checks for GICv2 and GICv3. Note that for GICv2, there's no
>>> need to wait until first vcpu run to do the check. Also note that I will
>>> have to keep the change in vgic_v3_check_base() to check for the old v3
>>> redist API at first vcpu run.
>> Sounds good.
>>
>> Thanks,
>>
>> Alex
>>
>>> Thanks,
>>> Ricardo
>>>
>>>> Thanks
>>>>
>>>> Eric
> Will do, thank you both.
>
> Ricardo
>
>>>>> Thanks,
>>>>>
>>>>> Alex
>>>>>
>>>>>> Thanks,
>>>>>> Ricardo
>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Alex
>>>>>>>
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

[Ricardo Koller]

On Tue, Sep 14, 2021 at 12:00:35PM +0100, Alexandru Elisei wrote:
> Hi Ricardo,
> 
> (adding kvm@vger.kernel.org to CC because the email this is a reply to got
> rejected because of html content)
> 
> On 9/14/21 04:20, Ricardo Koller wrote:
> > Hi Alexandru, Eric,
> >
> > On Mon, Sep 13, 2021 at 11:15:33AM +0100, Alexandru Elisei wrote:
> >> Hi Eric, Ricardo,
> >>
> >> On 9/10/21 20:32, Ricardo Koller wrote:
> >>> Hi Alexandru and Eric,
> >>>
> >>> On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote:
> >>>> Hi Alexandru,
> >>>>
> >>>> On 9/10/21 10:28 AM, Alexandru Elisei wrote:
> >>>>> Hi Ricardo,
> >>>>>
> >>>>> On 9/9/21 5:47 PM, Ricardo Koller wrote:
> >>>>>> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:
> >>>>>>> Hi Ricardo,
> >>>>>>>
> >>>>>>> On 9/8/21 10:03 PM, Ricardo Koller wrote:
> >>>>>>>> Extend vgic_v3_check_base() to verify that the redistributor regions
> >>>>>>>> don't go above the VM-specified IPA size (phys_size). This can happen
> >>>>>>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:
> >>>>>>>>
> >>>>>>>>   base + size > phys_size AND base < phys_size
> >>>>>>>>
> >>>>>>>> vgic_v3_check_base() is used to check the redist regions bases when
> >>>>>>>> setting them (with the vcpus added so far) and when attempting the first
> >>>>>>>> vcpu-run.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> >>>>>>>> ---
> >>>>>>>>  arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
> >>>>>>>>  1 file changed, 4 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> >>>>>>>> index 66004f61cd83..5afd9f6f68f6 100644
> >>>>>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> >>>>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> >>>>>>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> >>>>>>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> >>>>>>>>  			rdreg->base)
> >>>>>>>>  			return false;
> >>>>>>>> +
> >>>>>>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> >>>>>>>> +			kvm_phys_size(kvm))
> >>>>>>>> +			return false;
> >>>>>>> Looks to me like this same check (and the overflow one before it) is done when
> >>>>>>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
> >>>>>>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
> >>>>>>> kvm_vgic_addr() handles both ways of setting the Redistributor address.
> >>>>>>>
> >>>>>>> Without this patch, did you manage to set a base address such that base + size >
> >>>>>>> kvm_phys_size()?
> >>>>>>>
> >>>>>> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
> >>>>>> to get to this situation is with the selftest in patch 2.  I then tried
> >>>>>> an extra experiment: map the first redistributor, run the first vcpu,
> >>>>>> and access the redist from inside the guest. KVM didn't complain in any
> >>>>>> of these steps.
> >>>>> Yes, Eric pointed out that I was mistaken and there is no check being done for
> >>>>> base + size > kvm_phys_size().
> >>>>>
> >>>>> What I was trying to say is that this check is better done when the user creates a
> >>>>> Redistributor region, not when a VCPU is first run. We have everything we need to
> >>>>> make the check when a region is created, why wait until the VCPU is run?
> >>>>>
> >>>>> For example, vgic_v3_insert_redist_region() is called each time the adds a new
> >>>>> Redistributor region (via either of the two APIs), and already has a check for the
> >>>>> upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
> >>>>> add the check against the maximum IPA size there.
> >>>> you seem to refer to an old kernel as vgic_v3_insert_redist_region was
> >>>> renamed into� vgic_v3_alloc_redist_region in
> >>>> e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()
> >>>>
> >>>> I think in case you use the old rdist API you do not know yet the size
> >>>> of the redist region at this point (count=0), hence Ricardo's choice to
> >>>> do the check latter.
> >>> Just wanted to add one more detail. vgic_v3_check_base() is also called
> >>> when creating the redistributor region (via vgic_v3_set_redist_base ->
> >>> vgic_register_redist_iodev). This patch reuses that check for the old
> >>> redist API to also check for "base + size > kvm_phys_size()" with a size
> >>> calculated using the vcpus added so far.
> >> @Eric: Indeed I was looking at an older kernel by mistake, thank you for pointing
> >> that out!
> >>
> >> Thank you both for the explanations, the piece I was missing was the fact that
> >> KVM_VGIC_V3_ADDR_TYPE_REDIST specifies only the base address and the limit for the
> >> region is the number of VCPUs * (KVM_VGIC_V3_REDIST_SIZE = 128K), which makes it
> >> necessary to have the check when each VCPU is first run (as far as I can tell,
> >> VCPUs can be created at any time).
> >>
> >>>>> Also, because vgic_v3_insert_redist_region() already checks for overflow, I
> >>>>> believe the overflow check in vgic_v3_check_base() is redundant.
> >>>>>
> >>> It's redundant for the new redist API, but still needed for the old
> >>> redist API.
> >> Indeed.
> >>
> >>>>> As far as I can tell, vgic_v3_check_base() is there to make sure that the
> >>>>> Distributor doesn't overlap with any of the Redistributors, and because the
> >>>>> Redistributors and the Distributor can be created in any order, we defer the check
> >>>>> until the first VCPU is run. I might be wrong about this, someone please correct
> >>>>> me if I'm wrong.
> >>>>>
> >>>>> Also, did you verify that KVM is also doing this check for GICv2? KVM does
> >>>>> something similar and calls vgic_v2_check_base() when mapping the GIC resources,
> >>>>> and I don't see a check for the maximum IPA size in that function either.
> >>>> I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
> >>>> checks the base @)
> >>>>
> >>> It seems that GICv2 suffers from the same problem. The cpu interface
> >>> base is checked but the end can extend above IPA size. Note that the cpu
> >>> interface is 8KBs and vgic_check_ioaddr() is only checking that its base
> >> ... except that the doc for KVM_VGIC_V2_ADDR_TYPE_CPU says that the CPU interface
> >> region is 4K, while the check in vgic_v2_check_base() is done against
> >> KVM_VGIC_V2_CPU_SIZE, which is 8K.
> > The "GIC virtual CPU interface" alone is slightly more than 4K: GICV_DIR
> > is at 0x1000. The documentation might need to be updated.
> >
> >> I suppose that the CPU interface region is 8K
> >> because ARM IHI 0048B.b strongly recommends that the virtual CPU interface control
> >> registers are in a separate 4KB region, and KVM wants to emulate a GICv2 as close
> >> to the real thing as possible?
> > Are the "virtual CPU interface control" registers the ones starting with
> > GICH_? If yes, then I'm a bit confused, as those are not exposed to the
> > guest (to my knowledge).
> 
> Yes, those are the ones, and I also did find that they are not exposed to the guest.
> 
> Comparing the KVM documentation with what KVM actually does, I assumed that the
> 8KB was a forward looking decision, in case nested virtualization will support
> GICv2, which means that the GICH_* registers would also have to be exposed. Making
> the CPU interface for a CPU 8KB from the start would avoid changes or additions to
> the API if that happens.
> 
> However, after further digging through the spec, I found that the virtual CPU
> interface is specified to be 8KB (Table 5-10 of ARM IHI 0048B.b). I think that's
> the reason KVM treats it as 8KB.

Thanks for checking Alexandru. Will send a commit to update the
documentation as well then (v3).

Regards,
Ricardo

> 
> Thanks,
> 
> Alex
> 
> >
> >>> is 4KB aligned and below IPA size. The distributor region is 4KB so
> >>> vgic_check_ioaddr() is enough in that case.
> >>>
> >>> What about the following?
> >>>
> >>> I can work on the next version of this patch (v2 has the GICv2 issue)
> >>> which adds vgic_check_range(), which is like vgic_check_ioaddr() but
> >>> with a size arg.  kvm_vgic_addr() can then call vgic_check_range() and
> >>> do all the checks for GICv2 and GICv3. Note that for GICv2, there's no
> >>> need to wait until first vcpu run to do the check. Also note that I will
> >>> have to keep the change in vgic_v3_check_base() to check for the old v3
> >>> redist API at first vcpu run.
> >> Sounds good.
> >>
> >> Thanks,
> >>
> >> Alex
> >>
> >>> Thanks,
> >>> Ricardo
> >>>
> >>>> Thanks
> >>>>
> >>>> Eric
> > Will do, thank you both.
> >
> > Ricardo
> >
> >>>>> Thanks,
> >>>>>
> >>>>> Alex
> >>>>>
> >>>>>> Thanks,
> >>>>>> Ricardo
> >>>>>>
> >>>>>>> Thanks,
> >>>>>>>
> >>>>>>> Alex
> >>>>>>>
> >>>>>>>>  	}
> >>>>>>>>  
> >>>>>>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/2] KVM: arm64: selftests: test for vgic redist above the VM IPA size

[Ricardo Koller]

This test attempts (and fails) to set a redistributor region using the
legacy KVM_VGIC_V3_ADDR_TYPE_REDIST that's partially above the
VM-specified IPA size.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index 623f31a14326..6dd7b5e91421 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -285,6 +285,49 @@ static void test_vcpus_then_vgic(void)
 	vm_gic_destroy(&v);
 }
 
+static void test_redist_above_vm_pa_bits(enum vm_guest_mode mode)
+{
+	struct vm_gic v;
+	int ret, i;
+	uint32_t vcpuids[] = { 1, 2, 3, 4, };
+	int pa_bits = vm_guest_mode_params[mode].pa_bits;
+	uint64_t addr, psize = 1ULL << pa_bits;
+
+	/* Add vcpu 1 */
+	v.vm = vm_create_with_vcpus(mode, 1, DEFAULT_GUEST_PHY_PAGES,
+				    0, 0, guest_code, vcpuids);
+	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+
+	/* Set space for half a redist, we have 1 vcpu, so this fails. */
+	addr = psize - 0x10000;
+	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+	TEST_ASSERT(ret && errno == EINVAL, "not enough space for one redist");
+
+	/* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
+	addr = psize - (3 * 2 * 0x10000);
+	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+
+	addr = 0x00000;
+	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
+
+	/* Add three vcpus (2, 3, 4). */
+	for (i = 1; i < 4; ++i)
+		vm_vcpu_add_default(v.vm, vcpuids[i], guest_code);
+
+	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+
+	/* Attempt to run a vcpu without enough redist space. */
+	ret = run_vcpu(v.vm, vcpuids[3]);
+	TEST_ASSERT(ret && errno == EINVAL,
+			"redist base+size above IPA detected on 1st vcpu run");
+
+	vm_gic_destroy(&v);
+}
+
 static void test_new_redist_regions(void)
 {
 	void *dummy = NULL;
@@ -542,6 +585,7 @@ int main(int ac, char **av)
 	test_kvm_device();
 	test_vcpus_then_vgic();
 	test_vgic_then_vcpus();
+	test_redist_above_vm_pa_bits(VM_MODE_DEFAULT);
 	test_new_redist_regions();
 	test_typer_accesses();
 	test_last_bit_redist_regions();
-- 
2.33.0.153.gba50c8fa24-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: selftests: test for vgic redist above the VM IPA size

[Eric Auger]

Hi Ricardo,

On 9/8/21 11:03 PM, Ricardo Koller wrote:
> This test attempts (and fails) to set a redistributor region using the
> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST that's partially above the
> VM-specified IPA size.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index 623f31a14326..6dd7b5e91421 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -285,6 +285,49 @@ static void test_vcpus_then_vgic(void)
>  	vm_gic_destroy(&v);
>  }
>  
> +static void test_redist_above_vm_pa_bits(enum vm_guest_mode mode)
> +{
> +	struct vm_gic v;
> +	int ret, i;
> +	uint32_t vcpuids[] = { 1, 2, 3, 4, };
> +	int pa_bits = vm_guest_mode_params[mode].pa_bits;
> +	uint64_t addr, psize = 1ULL << pa_bits;
> +
> +	/* Add vcpu 1 */
> +	v.vm = vm_create_with_vcpus(mode, 1, DEFAULT_GUEST_PHY_PAGES,
> +				    0, 0, guest_code, vcpuids);
> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +
> +	/* Set space for half a redist, we have 1 vcpu, so this fails. */
> +	addr = psize - 0x10000;
> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +	TEST_ASSERT(ret && errno == EINVAL, "not enough space for one redist");
> +
> +	/* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
> +	addr = psize - (3 * 2 * 0x10000);
> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);

I think you need to test both the old API (KVM_VGIC_V3_ADDR_TYPE_REDIST)
and the new one (KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION).

Can't you integrate those new checks in existing tests,
subtest_redist_regions() and subtest_dist_rdist() which already tests
base addr beyond IPA limit (but not range end unfortunately). look for
E2BIG.

Thanks

Eric
> +
> +	addr = 0x00000;
> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> +
> +	/* Add three vcpus (2, 3, 4). */
> +	for (i = 1; i < 4; ++i)
> +		vm_vcpu_add_default(v.vm, vcpuids[i], guest_code);
> +
> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +
> +	/* Attempt to run a vcpu without enough redist space. */
> +	ret = run_vcpu(v.vm, vcpuids[3]);
> +	TEST_ASSERT(ret && errno == EINVAL,
> +			"redist base+size above IPA detected on 1st vcpu run");
> +
> +	vm_gic_destroy(&v);
> +}
> +
>  static void test_new_redist_regions(void)
>  {
>  	void *dummy = NULL;
> @@ -542,6 +585,7 @@ int main(int ac, char **av)
>  	test_kvm_device();
>  	test_vcpus_then_vgic();
>  	test_vgic_then_vcpus();
> +	test_redist_above_vm_pa_bits(VM_MODE_DEFAULT);
>  	test_new_redist_regions();
>  	test_typer_accesses();
>  	test_last_bit_redist_regions();

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: selftests: test for vgic redist above the VM IPA size

[Ricardo Koller]

On Thu, Sep 09, 2021 at 03:54:31PM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 9/8/21 11:03 PM, Ricardo Koller wrote:
> > This test attempts (and fails) to set a redistributor region using the
> > legacy KVM_VGIC_V3_ADDR_TYPE_REDIST that's partially above the
> > VM-specified IPA size.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/vgic_init.c | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > index 623f31a14326..6dd7b5e91421 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > @@ -285,6 +285,49 @@ static void test_vcpus_then_vgic(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > +static void test_redist_above_vm_pa_bits(enum vm_guest_mode mode)
> > +{
> > +	struct vm_gic v;
> > +	int ret, i;
> > +	uint32_t vcpuids[] = { 1, 2, 3, 4, };
> > +	int pa_bits = vm_guest_mode_params[mode].pa_bits;
> > +	uint64_t addr, psize = 1ULL << pa_bits;
> > +
> > +	/* Add vcpu 1 */
> > +	v.vm = vm_create_with_vcpus(mode, 1, DEFAULT_GUEST_PHY_PAGES,
> > +				    0, 0, guest_code, vcpuids);
> > +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> > +
> > +	/* Set space for half a redist, we have 1 vcpu, so this fails. */
> > +	addr = psize - 0x10000;
> > +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> > +	TEST_ASSERT(ret && errno == EINVAL, "not enough space for one redist");
> > +
> > +	/* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
> > +	addr = psize - (3 * 2 * 0x10000);
> > +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> 
> I think you need to test both the old API (KVM_VGIC_V3_ADDR_TYPE_REDIST)
> and the new one (KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION).
> 
> Can't you integrate those new checks in existing tests,
> subtest_redist_regions() and subtest_dist_rdist() which already tests
> base addr beyond IPA limit (but not range end unfortunately). look for
> E2BIG.
> 

Had some issues adapting subtest_dist_rdist() as the IPA range check for
ADDR_TYPE_REDIST is done at 1st vcpu run.  subtest_dist_rdist() is
already used to set overlapping dist/redist regions, which is then
checked to generate EINVAL on 1st vcpu run. If subtest_dist_rdist() is
also used to set the redist region above phys_size, then there won't be
a way of checking that the vcpu run fails because of both the overlap
and IPA issue.  It was simpler and cleaner to just add a new function
for the ADDR_TYPE_REDIST IPA range test.  Will adapt
subtest_redist_regions() as the check for ADDR_TYPE_REDIST_REGION can be
done when setting the regions.

Related Question:

Both the KVM_VGIC_V3_ADDR_TYPE_REDIST and KVM_RUN currently return
EINVAL with my proposed change (not E2BIG). I will change
KVM_VGIC_V3_ADDR_TYPE_REDIST to fail with E2BIG, but will leave KVM_RUN
failing with EINVAL.  Would you say that's the correct behavior?

Thanks,
Ricardo

> Thanks
> 
> Eric
> > +
> > +	addr = 0x00000;
> > +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> > +
> > +	/* Add three vcpus (2, 3, 4). */
> > +	for (i = 1; i < 4; ++i)
> > +		vm_vcpu_add_default(v.vm, vcpuids[i], guest_code);
> > +
> > +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> > +			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> > +
> > +	/* Attempt to run a vcpu without enough redist space. */
> > +	ret = run_vcpu(v.vm, vcpuids[3]);
> > +	TEST_ASSERT(ret && errno == EINVAL,
> > +			"redist base+size above IPA detected on 1st vcpu run");
> > +
> > +	vm_gic_destroy(&v);
> > +}
> > +
> >  static void test_new_redist_regions(void)
> >  {
> >  	void *dummy = NULL;
> > @@ -542,6 +585,7 @@ int main(int ac, char **av)
> >  	test_kvm_device();
> >  	test_vcpus_then_vgic();
> >  	test_vgic_then_vcpus();
> > +	test_redist_above_vm_pa_bits(VM_MODE_DEFAULT);
> >  	test_new_redist_regions();
> >  	test_typer_accesses();
> >  	test_last_bit_redist_regions();
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: selftests: test for vgic redist above the VM IPA size

[Eric Auger]

Hi Ricardo,

On 9/9/21 8:22 PM, Ricardo Koller wrote:
> On Thu, Sep 09, 2021 at 03:54:31PM +0200, Eric Auger wrote:
>> Hi Ricardo,
>>
>> On 9/8/21 11:03 PM, Ricardo Koller wrote:
>>> This test attempts (and fails) to set a redistributor region using the
>>> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST that's partially above the
>>> VM-specified IPA size.
>>>
>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>> ---
>>>  .../testing/selftests/kvm/aarch64/vgic_init.c | 44 +++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> index 623f31a14326..6dd7b5e91421 100644
>>> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> @@ -285,6 +285,49 @@ static void test_vcpus_then_vgic(void)
>>>  	vm_gic_destroy(&v);
>>>  }
>>>  
>>> +static void test_redist_above_vm_pa_bits(enum vm_guest_mode mode)
>>> +{
>>> +	struct vm_gic v;
>>> +	int ret, i;
>>> +	uint32_t vcpuids[] = { 1, 2, 3, 4, };
>>> +	int pa_bits = vm_guest_mode_params[mode].pa_bits;
>>> +	uint64_t addr, psize = 1ULL << pa_bits;
>>> +
>>> +	/* Add vcpu 1 */
>>> +	v.vm = vm_create_with_vcpus(mode, 1, DEFAULT_GUEST_PHY_PAGES,
>>> +				    0, 0, guest_code, vcpuids);
>>> +	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>>> +
>>> +	/* Set space for half a redist, we have 1 vcpu, so this fails. */
>>> +	addr = psize - 0x10000;
>>> +	ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>>> +	TEST_ASSERT(ret && errno == EINVAL, "not enough space for one redist");
>>> +
>>> +	/* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
>>> +	addr = psize - (3 * 2 * 0x10000);
>>> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> I think you need to test both the old API (KVM_VGIC_V3_ADDR_TYPE_REDIST)
>> and the new one (KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION).
>>
>> Can't you integrate those new checks in existing tests,
>> subtest_redist_regions() and subtest_dist_rdist() which already tests
>> base addr beyond IPA limit (but not range end unfortunately). look for
>> E2BIG.
>>
> Had some issues adapting subtest_dist_rdist() as the IPA range check for
> ADDR_TYPE_REDIST is done at 1st vcpu run.  subtest_dist_rdist() is
> already used to set overlapping dist/redist regions, which is then
> checked to generate EINVAL on 1st vcpu run. If subtest_dist_rdist() is
> also used to set the redist region above phys_size, then there won't be
> a way of checking that the vcpu run fails because of both the overlap
> and IPA issue.  It was simpler and cleaner to just add a new function
> for the ADDR_TYPE_REDIST IPA range test.  Will adapt
OK I see, then effectively adding a new test was more straightforward.
> subtest_redist_regions() as the check for ADDR_TYPE_REDIST_REGION can be
> done when setting the regions.
OK
>
> Related Question:
>
> Both the KVM_VGIC_V3_ADDR_TYPE_REDIST and KVM_RUN currently return
> EINVAL with my proposed change (not E2BIG). I will change
> KVM_VGIC_V3_ADDR_TYPE_REDIST to fail with E2BIG, but will leave KVM_RUN
> failing with EINVAL.  Would you say that's the correct behavior?
This looks OK to me, as long as the KVM uapi doc documents is aligned.

Thanks

Eric
>
> Thanks,
> Ricardo
>
>> Thanks
>>
>> Eric
>>> +
>>> +	addr = 0x00000;
>>> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +			KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
>>> +
>>> +	/* Add three vcpus (2, 3, 4). */
>>> +	for (i = 1; i < 4; ++i)
>>> +		vm_vcpu_add_default(v.vm, vcpuids[i], guest_code);
>>> +
>>> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>>> +			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>>> +
>>> +	/* Attempt to run a vcpu without enough redist space. */
>>> +	ret = run_vcpu(v.vm, vcpuids[3]);
>>> +	TEST_ASSERT(ret && errno == EINVAL,
>>> +			"redist base+size above IPA detected on 1st vcpu run");
>>> +
>>> +	vm_gic_destroy(&v);
>>> +}
>>> +
>>>  static void test_new_redist_regions(void)
>>>  {
>>>  	void *dummy = NULL;
>>> @@ -542,6 +585,7 @@ int main(int ac, char **av)
>>>  	test_kvm_device();
>>>  	test_vcpus_then_vgic();
>>>  	test_vgic_then_vcpus();
>>> +	test_redist_above_vm_pa_bits(VM_MODE_DEFAULT);
>>>  	test_new_redist_regions();
>>>  	test_typer_accesses();
>>>  	test_last_bit_redist_regions();

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm