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

[Oliver Upton <oupton@google.com>]

On Wed, Sep 8, 2021 at 5:50 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Sep 08, 2021 at 09:32:05PM +0000, Oliver Upton wrote:
> > Hi Ricardo,
> >
> > On Wed, Sep 08, 2021 at 02:03:19PM -0700, 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;
> >
> > Can we drop this check in favor of explicitly comparing rdreg->base with
> > kvm_phys_size()? I believe that would be more readable.
> >
>
> You mean the integer overflow check above? in that case, I would prefer
> to leave it, if you don't mind. It seems that this type of check is used
> in some other places in KVM (like kvm_assign_ioeventfd and
> vgic_v3_alloc_redist_region).

I would expect rdreg->base to exceed kvm_phys_size() before wrapping,
but we do derive rdreg->count from what userspace gives us. In that
case, your addition in combination with this makes sense, so no real
objections here.

> > > +
> > > +           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))
> > > --
> > > 2.33.0.153.gba50c8fa24-goog
> > >
> >
> > --
> > Thanks,
> > Oliver

Reviewed-by: Oliver Upton <oupton@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm