* [PATCH v2 0/2] KVM: arm64: vgic-v3: Missing check for redist region above the VM IPA size @ 2021-09-10 0:49 Ricardo Koller 2021-09-10 0:49 ` [PATCH v2 1/2] KVM: arm64: vgic: check redist region is not " Ricardo Koller 2021-09-10 0:49 ` [PATCH v2 2/2] KVM: arm64: selftests: tests for vgic redist regions " Ricardo Koller 0 siblings, 2 replies; 5+ messages in thread From: Ricardo Koller @ 2021-09-10 0:49 UTC (permalink / raw) To: kvm, maz, kvmarm, drjones, eric.auger, alexandru.elisei Cc: pshier, Paolo Bonzini, shuah KVM doesn't check for redist regions that extend partially above the VM-specified IPA (phys_size). This can happen when using the KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute to set a new region that extends partially above phys_size (with the base below phys_size). The issue is that vcpus can potentially run into a situation where some redistributors are addressable and others are not. Patch 1 adds the missing checks, and patch 2 adds a test into aarch64/vgic_init. Changes: v2: adding a test for KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, and returning E2BIG instead of EINVAL (thanks Alexandru and Eric). Ricardo Koller (2): KVM: arm64: vgic: check redist region is not above the VM IPA size KVM: arm64: selftests: tests for vgic redist regions above the VM IPA size arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++- arch/arm64/kvm/vgic/vgic-v3.c | 4 ++ .../testing/selftests/kvm/aarch64/vgic_init.c | 52 +++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) -- 2.33.0.309.g3052b89438-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size 2021-09-10 0:49 [PATCH v2 0/2] KVM: arm64: vgic-v3: Missing check for redist region above the VM IPA size Ricardo Koller @ 2021-09-10 0:49 ` Ricardo Koller 2021-09-20 12:30 ` Marc Zyngier 2021-09-10 0:49 ` [PATCH v2 2/2] KVM: arm64: selftests: tests for vgic redist regions " Ricardo Koller 1 sibling, 1 reply; 5+ messages in thread From: Ricardo Koller @ 2021-09-10 0:49 UTC (permalink / raw) To: kvm, maz, kvmarm, drjones, eric.auger, alexandru.elisei Cc: pshier, Paolo Bonzini, shuah 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)) -- 2.33.0.309.g3052b89438-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size 2021-09-10 0:49 ` [PATCH v2 1/2] KVM: arm64: vgic: check redist region is not " Ricardo Koller @ 2021-09-20 12:30 ` Marc Zyngier 2021-09-20 21:06 ` Ricardo Koller 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2021-09-20 12:30 UTC (permalink / raw) To: Ricardo Koller; +Cc: kvm, shuah, pshier, Paolo Bonzini, kvmarm 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()). Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size 2021-09-20 12:30 ` Marc Zyngier @ 2021-09-20 21:06 ` Ricardo Koller 0 siblings, 0 replies; 5+ messages in thread From: Ricardo Koller @ 2021-09-20 21:06 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, shuah, pshier, Paolo Bonzini, kvmarm 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. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] KVM: arm64: selftests: tests for vgic redist regions above the VM IPA size 2021-09-10 0:49 [PATCH v2 0/2] KVM: arm64: vgic-v3: Missing check for redist region above the VM IPA size Ricardo Koller 2021-09-10 0:49 ` [PATCH v2 1/2] KVM: arm64: vgic: check redist region is not " Ricardo Koller @ 2021-09-10 0:49 ` Ricardo Koller 1 sibling, 0 replies; 5+ messages in thread From: Ricardo Koller @ 2021-09-10 0:49 UTC (permalink / raw) To: kvm, maz, kvmarm, drjones, eric.auger, alexandru.elisei Cc: pshier, Paolo Bonzini, shuah These new tests attempt (and fail) to set a redist region using the KVM_VGIC_V3_ADDR_TYPE_REDIST and KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS with regions partially above the VM-specified IPA size (phys_size). Signed-off-by: Ricardo Koller <ricarkol@google.com> --- .../testing/selftests/kvm/aarch64/vgic_init.c | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c index 623f31a14326..15aa01dc6a4d 100644 --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c @@ -285,6 +285,57 @@ static void test_vcpus_then_vgic(void) vm_gic_destroy(&v); } +static void test_redist_above_vm_pa_bits(enum vm_guest_mode mode) +{ + struct vm_gic v; + int ret, i; + uint32_t vcpuids[] = { 1, 2, 3, 4, }; + int pa_bits = vm_guest_mode_params[mode].pa_bits; + uint64_t addr, psize = 1ULL << pa_bits; + + /* Add vcpu 1 */ + v.vm = vm_create_with_vcpus(mode, 1, DEFAULT_GUEST_PHY_PAGES, + 0, 0, guest_code, vcpuids); + v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false); + + /* The redist end is above the IPA size, so this fails. */ + addr = REDIST_REGION_ATTR_ADDR(1, psize - 0x10000, 0, 0); + ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, + KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true); + TEST_ASSERT(ret && errno == E2BIG, + "register redist region with end address beyond IPA range"); + + /* Set space for half a redist, we have 1 vcpu, so this fails. */ + addr = psize - 0x10000; + ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, + KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true); + TEST_ASSERT(ret && errno == E2BIG, + "register redist (legacy) with end address beyond IPA range"); + + /* Set space for 3 redists, we have 1 vcpu, so this succeeds. */ + addr = psize - (3 * 2 * 0x10000); + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, + KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true); + + addr = 0x00000; + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, + KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true); + + /* Add the rest of the VCPUs */ + for (i = 1; i < 4; ++i) + vm_vcpu_add_default(v.vm, vcpuids[i], guest_code); + + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, + KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true); + + /* Attempt to run a vcpu without enough redist space. */ + ret = run_vcpu(v.vm, vcpuids[3]); + TEST_ASSERT(ret && errno == EINVAL, + "redist base+size above IPA detected on 1st vcpu run"); + + vm_gic_destroy(&v); +} + static void test_new_redist_regions(void) { void *dummy = NULL; @@ -542,6 +593,7 @@ int main(int ac, char **av) test_kvm_device(); test_vcpus_then_vgic(); test_vgic_then_vcpus(); + test_redist_above_vm_pa_bits(VM_MODE_DEFAULT); test_new_redist_regions(); test_typer_accesses(); test_last_bit_redist_regions(); -- 2.33.0.309.g3052b89438-goog _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-20 21:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-10 0:49 [PATCH v2 0/2] KVM: arm64: vgic-v3: Missing check for redist region above the VM IPA size Ricardo Koller 2021-09-10 0:49 ` [PATCH v2 1/2] KVM: arm64: vgic: check redist region is not " Ricardo Koller 2021-09-20 12:30 ` Marc Zyngier 2021-09-20 21:06 ` Ricardo Koller 2021-09-10 0:49 ` [PATCH v2 2/2] KVM: arm64: selftests: tests for vgic redist regions " Ricardo Koller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).