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

[Ricardo Koller <ricarkol@google.com>]

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))