* [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors
@ 2024-04-24 17:39 Oliver Upton
2024-04-24 17:39 ` [PATCH 1/2] KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr() Oliver Upton
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Oliver Upton @ 2024-04-24 17:39 UTC (permalink / raw)
To: kvmarm
Cc: Marc Zyngier, James Morse, Suzuki K Poulose, Zenghui Yu, kvm,
Alexander Potapenko, Dmitry Vyukov, Oliver Upton
Alex reports that it is possible to trigger a NULL dereference via the
vgic-v2 device attribute accessors, stemming from a lack of sanitization
of user input...
Here's a fix + regression test for the bug. Obviously, I intend to take
these as a fix ASAP.
Oliver Upton (2):
KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr()
KVM: selftests: Add test for uaccesses to non-existent vgic-v2 CPUIF
arch/arm64/kvm/vgic/vgic-kvm-device.c | 8 +--
.../testing/selftests/kvm/aarch64/vgic_init.c | 49 +++++++++++++++++++
2 files changed, 53 insertions(+), 4 deletions(-)
base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr()
2024-04-24 17:39 [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Oliver Upton
@ 2024-04-24 17:39 ` Oliver Upton
2024-04-24 17:39 ` [PATCH 2/2] KVM: selftests: Add test for uaccesses to non-existent vgic-v2 CPUIF Oliver Upton
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Oliver Upton @ 2024-04-24 17:39 UTC (permalink / raw)
To: kvmarm
Cc: Marc Zyngier, James Morse, Suzuki K Poulose, Zenghui Yu, kvm,
Alexander Potapenko, Dmitry Vyukov, Oliver Upton, stable
vgic_v2_parse_attr() is responsible for finding the vCPU that matches
the user-provided CPUID, which (of course) may not be valid. If the ID
is invalid, kvm_get_vcpu_by_id() returns NULL, which isn't handled
gracefully.
Similar to the GICv3 uaccess flow, check that kvm_get_vcpu_by_id()
actually returns something and fail the ioctl if not.
Cc: stable@vger.kernel.org
Fixes: 7d450e282171 ("KVM: arm/arm64: vgic-new: Add userland access to VGIC dist registers")
Reported-by: Alexander Potapenko <glider@google.com>
Tested-by: Alexander Potapenko <glider@google.com>
Reviewed-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
arch/arm64/kvm/vgic/vgic-kvm-device.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index f48b8dab8b3d..1d26bb5b02f4 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -338,12 +338,12 @@ int kvm_register_vgic_device(unsigned long type)
int vgic_v2_parse_attr(struct kvm_device *dev, struct kvm_device_attr *attr,
struct vgic_reg_attr *reg_attr)
{
- int cpuid;
+ int cpuid = FIELD_GET(KVM_DEV_ARM_VGIC_CPUID_MASK, attr->attr);
- cpuid = FIELD_GET(KVM_DEV_ARM_VGIC_CPUID_MASK, attr->attr);
-
- reg_attr->vcpu = kvm_get_vcpu_by_id(dev->kvm, cpuid);
reg_attr->addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+ reg_attr->vcpu = kvm_get_vcpu_by_id(dev->kvm, cpuid);
+ if (!reg_attr->vcpu)
+ return -EINVAL;
return 0;
}
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] KVM: selftests: Add test for uaccesses to non-existent vgic-v2 CPUIF
2024-04-24 17:39 [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Oliver Upton
2024-04-24 17:39 ` [PATCH 1/2] KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr() Oliver Upton
@ 2024-04-24 17:39 ` Oliver Upton
2024-04-24 18:06 ` [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Marc Zyngier
2024-04-24 19:15 ` Oliver Upton
3 siblings, 0 replies; 5+ messages in thread
From: Oliver Upton @ 2024-04-24 17:39 UTC (permalink / raw)
To: kvmarm
Cc: Marc Zyngier, James Morse, Suzuki K Poulose, Zenghui Yu, kvm,
Alexander Potapenko, Dmitry Vyukov, Oliver Upton
Assert that accesses to a non-existent vgic-v2 CPU interface
consistently fail across the various KVM device attr ioctls. This also
serves as a regression test for a bug wherein KVM hits a NULL
dereference when the CPUID specified in the ioctl is invalid.
Note that there is no need to print the observed errno, as TEST_ASSERT()
will take care of it.
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
.../testing/selftests/kvm/aarch64/vgic_init.c | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index eef816b80993..ca917c71ff60 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -84,6 +84,18 @@ static struct vm_gic vm_gic_create_with_vcpus(uint32_t gic_dev_type,
return v;
}
+static struct vm_gic vm_gic_create_barebones(uint32_t gic_dev_type)
+{
+ struct vm_gic v;
+
+ v.gic_dev_type = gic_dev_type;
+ v.vm = vm_create_barebones();
+ v.gic_fd = kvm_create_device(v.vm, gic_dev_type);
+
+ return v;
+}
+
+
static void vm_gic_destroy(struct vm_gic *v)
{
close(v->gic_fd);
@@ -357,6 +369,40 @@ static void test_vcpus_then_vgic(uint32_t gic_dev_type)
vm_gic_destroy(&v);
}
+#define KVM_VGIC_V2_ATTR(offset, cpu) \
+ (FIELD_PREP(KVM_DEV_ARM_VGIC_OFFSET_MASK, offset) | \
+ FIELD_PREP(KVM_DEV_ARM_VGIC_CPUID_MASK, cpu))
+
+#define GIC_CPU_CTRL 0x00
+
+static void test_v2_uaccess_cpuif_no_vcpus(void)
+{
+ struct vm_gic v;
+ u64 val = 0;
+ int ret;
+
+ v = vm_gic_create_barebones(KVM_DEV_TYPE_ARM_VGIC_V2);
+ subtest_dist_rdist(&v);
+
+ ret = __kvm_has_device_attr(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
+ KVM_VGIC_V2_ATTR(GIC_CPU_CTRL, 0));
+ TEST_ASSERT(ret && errno == EINVAL,
+ "accessed non-existent CPU interface, want errno: %i",
+ EINVAL);
+ ret = __kvm_device_attr_get(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
+ KVM_VGIC_V2_ATTR(GIC_CPU_CTRL, 0), &val);
+ TEST_ASSERT(ret && errno == EINVAL,
+ "accessed non-existent CPU interface, want errno: %i",
+ EINVAL);
+ ret = __kvm_device_attr_set(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
+ KVM_VGIC_V2_ATTR(GIC_CPU_CTRL, 0), &val);
+ TEST_ASSERT(ret && errno == EINVAL,
+ "accessed non-existent CPU interface, want errno: %i",
+ EINVAL);
+
+ vm_gic_destroy(&v);
+}
+
static void test_v3_new_redist_regions(void)
{
struct kvm_vcpu *vcpus[NR_VCPUS];
@@ -675,6 +721,9 @@ void run_tests(uint32_t gic_dev_type)
test_vcpus_then_vgic(gic_dev_type);
test_vgic_then_vcpus(gic_dev_type);
+ if (VGIC_DEV_IS_V2(gic_dev_type))
+ test_v2_uaccess_cpuif_no_vcpus();
+
if (VGIC_DEV_IS_V3(gic_dev_type)) {
test_v3_new_redist_regions();
test_v3_typer_accesses();
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors
2024-04-24 17:39 [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Oliver Upton
2024-04-24 17:39 ` [PATCH 1/2] KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr() Oliver Upton
2024-04-24 17:39 ` [PATCH 2/2] KVM: selftests: Add test for uaccesses to non-existent vgic-v2 CPUIF Oliver Upton
@ 2024-04-24 18:06 ` Marc Zyngier
2024-04-24 19:15 ` Oliver Upton
3 siblings, 0 replies; 5+ messages in thread
From: Marc Zyngier @ 2024-04-24 18:06 UTC (permalink / raw)
To: Oliver Upton
Cc: kvmarm, James Morse, Suzuki K Poulose, Zenghui Yu, kvm,
Alexander Potapenko, Dmitry Vyukov
On Wed, 24 Apr 2024 18:39:57 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Alex reports that it is possible to trigger a NULL dereference via the
> vgic-v2 device attribute accessors, stemming from a lack of sanitization
> of user input...
>
> Here's a fix + regression test for the bug. Obviously, I intend to take
> these as a fix ASAP.
>
> Oliver Upton (2):
> KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr()
> KVM: selftests: Add test for uaccesses to non-existent vgic-v2 CPUIF
>
> arch/arm64/kvm/vgic/vgic-kvm-device.c | 8 +--
> .../testing/selftests/kvm/aarch64/vgic_init.c | 49 +++++++++++++++++++
> 2 files changed, 53 insertions(+), 4 deletions(-)
>
>
> base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
Thanks Alex for the heads up!
Reviewed-by: Marc Zyngier <maz@kernel.org>
Please queue this at your earliest convenience.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors
2024-04-24 17:39 [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Oliver Upton
` (2 preceding siblings ...)
2024-04-24 18:06 ` [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Marc Zyngier
@ 2024-04-24 19:15 ` Oliver Upton
3 siblings, 0 replies; 5+ messages in thread
From: Oliver Upton @ 2024-04-24 19:15 UTC (permalink / raw)
To: Oliver Upton, kvmarm
Cc: Marc Zyngier, kvm, Zenghui Yu, Suzuki K Poulose, Dmitry Vyukov,
James Morse, Alexander Potapenko
On Wed, 24 Apr 2024 17:39:57 +0000, Oliver Upton wrote:
> Alex reports that it is possible to trigger a NULL dereference via the
> vgic-v2 device attribute accessors, stemming from a lack of sanitization
> of user input...
>
> Here's a fix + regression test for the bug. Obviously, I intend to take
> these as a fix ASAP.
>
> [...]
Applied to kvmarm/fixes, thanks!
[1/2] KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr()
https://git.kernel.org/kvmarm/kvmarm/c/6ddb4f372fc6
[2/2] KVM: selftests: Add test for uaccesses to non-existent vgic-v2 CPUIF
https://git.kernel.org/kvmarm/kvmarm/c/160933e330f4
--
Best,
Oliver
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-24 19:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 17:39 [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Oliver Upton
2024-04-24 17:39 ` [PATCH 1/2] KVM: arm64: vgic-v2: Check for non-NULL vCPU in vgic_v2_parse_attr() Oliver Upton
2024-04-24 17:39 ` [PATCH 2/2] KVM: selftests: Add test for uaccesses to non-existent vgic-v2 CPUIF Oliver Upton
2024-04-24 18:06 ` [PATCH 0/2] KVM: arm64: Avoid NULL dereference in vgic-v2 device attr accessors Marc Zyngier
2024-04-24 19:15 ` Oliver Upton
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).