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

From: Eric Auger <eric.auger@redhat.com>
To: Alexandru Elisei <alexandru.elisei@arm.com>,
	Ricardo Koller <ricarkol@google.com>,
	kvm@vger.kernel.org, maz@kernel.org,
	kvmarm@lists.cs.columbia.edu, drjones@redhat.com
Cc: pshier@google.com, Paolo Bonzini <pbonzini@redhat.com>, shuah@kernel.org
Subject: Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size
Date: Thu, 9 Sep 2021 16:43:11 +0200	[thread overview]
Message-ID: <bd905ebe-f786-9d5b-d19d-03ff5fa1ba14@redhat.com> (raw)
In-Reply-To: <b368e9cf-ec28-1768-edf9-dfdc7fa108f8@arm.com>

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