Re: [PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size

From: Eric Auger <eric.auger@redhat.com>
To: Ricardo Koller <ricarkol@google.com>
Cc: kvm@vger.kernel.org, maz@kernel.org, shuah@kernel.org,
	pshier@google.com, Paolo Bonzini <pbonzini@redhat.com>,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size
Date: Thu, 30 Sep 2021 09:05:47 +0200	[thread overview]
Message-ID: <dac39cc1-16d7-caed-833f-03943b0e6211@redhat.com> (raw)
In-Reply-To: <YVTWVf26yYNUUx2L@google.com>

Hi,

On 9/29/21 11:10 PM, Ricardo Koller wrote:
> Hi Eric,
>
> On Wed, Sep 29, 2021 at 06:23:04PM +0200, Eric Auger wrote:
>> Hi Ricardo,
>>
>> On 9/28/21 8:47 PM, Ricardo Koller wrote:
>>> Verify that the redistributor regions do not extend beyond the
>>> VM-specified IPA range (phys_size). This can happen when using
>>> KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS
>>> with:
>>>
>>>   base + size > phys_size AND base < phys_size
>>>
>>> Add the missing check into vgic_v3_alloc_redist_region() which is called
>>> when setting the regions, and into vgic_v3_check_base() which is called
>>> when attempting the first vcpu-run. The vcpu-run check does not apply to
>>> KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS because the regions size is known
>>> before the first vcpu-run. Note that using the REDIST_REGIONS API
>>> results in a different check, which already exists, at first vcpu run:
>>> that the number of redist regions is enough for all vcpus.
>>>
>>> Finally, this patch also enables some extra tests in
>>> vgic_v3_alloc_redist_region() by calculating "size" early for the legacy
>>> redist api: like checking that the REDIST region can fit all the already
>>> created vcpus.
>>>
>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++--
>>>  arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> index a09cdc0b953c..9be02bf7865e 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> @@ -796,7 +796,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>>>  	struct vgic_dist *d = &kvm->arch.vgic;
>>>  	struct vgic_redist_region *rdreg;
>>>  	struct list_head *rd_regions = &d->rd_regions;
>>> -	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
>>> +	int nr_vcpus = atomic_read(&kvm->online_vcpus);
>>> +	size_t size = count ? count * KVM_VGIC_V3_REDIST_SIZE
>>> +			    : nr_vcpus * KVM_VGIC_V3_REDIST_SIZE;
>> This actually fixes the  vgic_dist_overlap(kvm, base, size=0)
>>
>> in case the number of online-vcpus at that time is less than the final
>> one (1st run), if count=0 (legacy API) do we ever check that the RDIST
>> (with accumulated vcpu rdists) does not overlap with dist.
>> in other words shouldn't we call vgic_dist_overlap(kvm, base, size)
>> again in vgic_v3_check_base().
>>
> I think we're good; that's checked by vgic_v3_rdist_overlap(dist_base)
> in vgic_v3_check_base(). This function uses the only region (legacy
> case) using a size based on the online_vcpus (in
> vgic_v3_rd_region_size()).  This exact situation is tested by
> test_vgic_then_vcpus() in the vgic_init selftest.
Ah OK so that's fine then.

So looks good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric


>
> Thanks,
> Ricardo
>
>> Thanks
>>
>> Eric
>>
>>>  	int ret;
>>>  
>>>  	/* cross the end of memory ? */
>>> @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>>>  
>>>  	rdreg->base = VGIC_ADDR_UNDEF;
>>>  
>>> -	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
>>> +	ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size);
>>>  	if (ret)
>>>  		goto free;
>>>  
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>> index 21a6207fb2ee..27ee674631b3 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>> @@ -486,6 +486,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))

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