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

[Ricardo Koller <ricarkol@google.com>]

On Mon, Sep 20, 2021 at 01:30:40PM +0100, Marc Zyngier wrote:
> Hi Ricardo,
> 
> On Fri, 10 Sep 2021 01:49:18 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > Verify that the redistributor regions do not extend beyond the
> > VM-specified IPA size (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. Finally, this patch also enables some extra
> > tests in vgic_v3_alloc_redist_region() by calculating "size" early for
> > the legacy redist api.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++++++-
> >  arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > index a09cdc0b953c..055671bede85 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;
> >  	int ret;
> >  
> >  	/* cross the end of memory ? */
> > @@ -834,6 +836,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
> >  	if (vgic_v3_rdist_overlap(kvm, base, size))
> >  		return -EINVAL;
> >  
> > +	if (base + size > kvm_phys_size(kvm))
> > +		return -E2BIG;
> > +
> >  	rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
> >  	if (!rdreg)
> >  		return -ENOMEM;
> > 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))
> 
> How about vgic-v2? From what I can see, the placement of the
> distributor and CPU interface should be subjected to the same checks
> (see vgic_v2_check_base()).
> 

Yes, gic-v2 has the same problem. Alexandru first mentioned it here:

  https://lore.kernel.org/kvmarm/5eb41efd-2ff2-d25b-5801-f4a56457a09f@arm.com/

I'm now working on the next version of the patch which applies to gic-v2
as well.

Thanks,
Ricardo

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.