| Message ID | 20210928184803.2496885-3-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic: Missing checks for REDIST/CPU and ITS regions above the VM IPA size | expand |
Hi Ricardo, On 9/28/21 8:47 PM, Ricardo Koller wrote: > Verify that the redistributor regions do not extend beyond the > VM-specified IPA range (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. Note that using the REDIST_REGIONS API > results in a different check, which already exists, at first vcpu run: > that the number of redist regions is enough for all vcpus. > > Finally, this patch also enables some extra tests in > vgic_v3_alloc_redist_region() by calculating "size" early for the legacy > redist api: like checking that the REDIST region can fit all the already > created vcpus. > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > --- > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++-- > arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++ > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > index a09cdc0b953c..9be02bf7865e 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; This actually fixes the vgic_dist_overlap(kvm, base, size=0) in case the number of online-vcpus at that time is less than the final one (1st run), if count=0 (legacy API) do we ever check that the RDIST (with accumulated vcpu rdists) does not overlap with dist. in other words shouldn't we call vgic_dist_overlap(kvm, base, size) again in vgic_v3_check_base(). Thanks Eric > int ret; > > /* cross the end of memory ? */ > @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index, > > rdreg->base = VGIC_ADDR_UNDEF; > > - ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); > + ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size); > if (ret) > goto free; > > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c > index 21a6207fb2ee..27ee674631b3 100644 > --- a/arch/arm64/kvm/vgic/vgic-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > @@ -486,6 +486,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))
Hi Eric, On Wed, Sep 29, 2021 at 06:23:04PM +0200, Eric Auger wrote: > Hi Ricardo, > > On 9/28/21 8:47 PM, Ricardo Koller wrote: > > Verify that the redistributor regions do not extend beyond the > > VM-specified IPA range (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. Note that using the REDIST_REGIONS API > > results in a different check, which already exists, at first vcpu run: > > that the number of redist regions is enough for all vcpus. > > > > Finally, this patch also enables some extra tests in > > vgic_v3_alloc_redist_region() by calculating "size" early for the legacy > > redist api: like checking that the REDIST region can fit all the already > > created vcpus. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++-- > > arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++ > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > > index a09cdc0b953c..9be02bf7865e 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; > > This actually fixes the vgic_dist_overlap(kvm, base, size=0) > > in case the number of online-vcpus at that time is less than the final > one (1st run), if count=0 (legacy API) do we ever check that the RDIST > (with accumulated vcpu rdists) does not overlap with dist. > in other words shouldn't we call vgic_dist_overlap(kvm, base, size) > again in vgic_v3_check_base(). > I think we're good; that's checked by vgic_v3_rdist_overlap(dist_base) in vgic_v3_check_base(). This function uses the only region (legacy case) using a size based on the online_vcpus (in vgic_v3_rd_region_size()). This exact situation is tested by test_vgic_then_vcpus() in the vgic_init selftest. Thanks, Ricardo > Thanks > > Eric > > > int ret; > > > > /* cross the end of memory ? */ > > @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index, > > > > rdreg->base = VGIC_ADDR_UNDEF; > > > > - ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); > > + ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size); > > if (ret) > > goto free; > > > > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c > > index 21a6207fb2ee..27ee674631b3 100644 > > --- a/arch/arm64/kvm/vgic/vgic-v3.c > > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > > @@ -486,6 +486,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)) >
Hi, On 9/29/21 11:10 PM, Ricardo Koller wrote: > Hi Eric, > > On Wed, Sep 29, 2021 at 06:23:04PM +0200, Eric Auger wrote: >> Hi Ricardo, >> >> On 9/28/21 8:47 PM, Ricardo Koller wrote: >>> Verify that the redistributor regions do not extend beyond the >>> VM-specified IPA range (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. Note that using the REDIST_REGIONS API >>> results in a different check, which already exists, at first vcpu run: >>> that the number of redist regions is enough for all vcpus. >>> >>> Finally, this patch also enables some extra tests in >>> vgic_v3_alloc_redist_region() by calculating "size" early for the legacy >>> redist api: like checking that the REDIST region can fit all the already >>> created vcpus. >>> >>> Signed-off-by: Ricardo Koller <ricarkol@google.com> >>> --- >>> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++-- >>> arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++ >>> 2 files changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c >>> index a09cdc0b953c..9be02bf7865e 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; >> This actually fixes the vgic_dist_overlap(kvm, base, size=0) >> >> in case the number of online-vcpus at that time is less than the final >> one (1st run), if count=0 (legacy API) do we ever check that the RDIST >> (with accumulated vcpu rdists) does not overlap with dist. >> in other words shouldn't we call vgic_dist_overlap(kvm, base, size) >> again in vgic_v3_check_base(). >> > I think we're good; that's checked by vgic_v3_rdist_overlap(dist_base) > in vgic_v3_check_base(). This function uses the only region (legacy > case) using a size based on the online_vcpus (in > vgic_v3_rd_region_size()). This exact situation is tested by > test_vgic_then_vcpus() in the vgic_init selftest. Ah OK so that's fine then. So looks good to me Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > > Thanks, > Ricardo > >> Thanks >> >> Eric >> >>> int ret; >>> >>> /* cross the end of memory ? */ >>> @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index, >>> >>> rdreg->base = VGIC_ADDR_UNDEF; >>> >>> - ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); >>> + ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size); >>> if (ret) >>> goto free; >>> >>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c >>> index 21a6207fb2ee..27ee674631b3 100644 >>> --- a/arch/arm64/kvm/vgic/vgic-v3.c >>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c >>> @@ -486,6 +486,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))
On Tue, 28 Sep 2021 19:47:56 +0100, Ricardo Koller <ricarkol@google.com> wrote: > > Verify that the redistributor regions do not extend beyond the > VM-specified IPA range (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. Note that using the REDIST_REGIONS API > results in a different check, which already exists, at first vcpu run: > that the number of redist regions is enough for all vcpus. > > Finally, this patch also enables some extra tests in > vgic_v3_alloc_redist_region() by calculating "size" early for the legacy > redist api: like checking that the REDIST region can fit all the already > created vcpus. > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > --- > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++-- > arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++ > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > index a09cdc0b953c..9be02bf7865e 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 ? */ > @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index, > > rdreg->base = VGIC_ADDR_UNDEF; > > - ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); > + ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size); > if (ret) > goto free; > > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c > index 21a6207fb2ee..27ee674631b3 100644 > --- a/arch/arm64/kvm/vgic/vgic-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > @@ -486,6 +486,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; Why can't we replace these two checks with a single call to your new fancy helper? Thanks, M.
On Fri, Oct 01, 2021 at 02:14:29PM +0100, Marc Zyngier wrote: > On Tue, 28 Sep 2021 19:47:56 +0100, > Ricardo Koller <ricarkol@google.com> wrote: > > > > Verify that the redistributor regions do not extend beyond the > > VM-specified IPA range (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. Note that using the REDIST_REGIONS API > > results in a different check, which already exists, at first vcpu run: > > that the number of redist regions is enough for all vcpus. > > > > Finally, this patch also enables some extra tests in > > vgic_v3_alloc_redist_region() by calculating "size" early for the legacy > > redist api: like checking that the REDIST region can fit all the already > > created vcpus. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++-- > > arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++ > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > > index a09cdc0b953c..9be02bf7865e 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 ? */ > > @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index, > > > > rdreg->base = VGIC_ADDR_UNDEF; > > > > - ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); > > + ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size); > > if (ret) > > goto free; > > > > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c > > index 21a6207fb2ee..27ee674631b3 100644 > > --- a/arch/arm64/kvm/vgic/vgic-v3.c > > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > > @@ -486,6 +486,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; > > Why can't we replace these two checks with a single call to your new > fancy helper? ACK using the new helper (on rdreg base and size). Thanks, Ricardo > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c index a09cdc0b953c..9be02bf7865e 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 ? */ @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index, rdreg->base = VGIC_ADDR_UNDEF; - ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); + ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size); if (ret) goto free; diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c index 21a6207fb2ee..27ee674631b3 100644 --- a/arch/arm64/kvm/vgic/vgic-v3.c +++ b/arch/arm64/kvm/vgic/vgic-v3.c @@ -486,6 +486,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))
Verify that the redistributor regions do not extend beyond the VM-specified IPA range (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. Note that using the REDIST_REGIONS API results in a different check, which already exists, at first vcpu run: that the number of redist regions is enough for all vcpus. Finally, this patch also enables some extra tests in vgic_v3_alloc_redist_region() by calculating "size" early for the legacy redist api: like checking that the REDIST region can fit all the already created vcpus. Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++-- arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-)