* [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
* [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
* 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
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).