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

[Eric Auger <eric.auger@redhat.com>]

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