Re: [PATCH] KVM: arm/arm64: check IRQ number on userland injection
On 13/04/15 11:04, Christoffer Dall wrote:
On Fri, Apr 10, 2015 at 05:52:05PM +0100, Andre Przywara wrote:
Hi Christopher,
On 10/04/15 16:29, Christopher Covington wrote:
Hi Andre,
On 04/10/2015 11:17 AM, Andre Przywara wrote:
When userland injects a SPI via the KVM_IRQ_LINE ioctl we currently
only check it against a fixed limit, which historically is set
to 127. With the new dynamic IRQ allocation the effective limit may
actually be smaller (64).
So when now a malicious or buggy userland injects a SPI in that
range, we spill over on our VGIC bitmaps and bytemaps memory.
I could trigger a host kernel NULL pointer dereference with current
mainline by injecting some bogus IRQ number from a hacked kvmtool:
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -195,7 +195,11 @@ struct kvm_arch_memory_slot {
#define KVM_ARM_IRQ_CPU_IRQ 0
#define KVM_ARM_IRQ_CPU_FIQ 1
-/* Highest supported SPI, from VGIC_NR_IRQS */
+/*
+ * This used to hold the highest supported SPI, but it is now obsolete
+ * and only here to provide source code level compatibility with older
+ * userland. The highest SPI number can be set via
KVM_DEV_ARM_VGIC_GRP_NR_IRQS.
+ */
#define KVM_ARM_IRQ_GIC_MAX 127
If that's the case should it maybe only defined when __KERNEL__ is not
defined?
Mmmh, I am not sure it's really worth the hassle. Actually it seems like
that neither kvmtool nor QEMU use this definition, so it's more or less
orphaned by now. I am confident we can avoid it sneaking in in the
kernel again.
TBH, I wouldn't object against Marc enclosing the definition in an
#ifdef __KERNEL__.
Yeah, I'll fix that up (assuming you mean #ifndef rather than #ifdef).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm/arm64: check IRQ number on userland injection
On Mon, Apr 13, 2015 at 11:21:20AM +0100, Marc Zyngier wrote:
On 13/04/15 11:04, Christoffer Dall wrote:
On Fri, Apr 10, 2015 at 05:52:05PM +0100, Andre Przywara wrote:
Hi Christopher,
On 10/04/15 16:29, Christopher Covington wrote:
Hi Andre,
On 04/10/2015 11:17 AM, Andre Przywara wrote:
When userland injects a SPI via the KVM_IRQ_LINE ioctl we currently
only check it against a fixed limit, which historically is set
to 127. With the new dynamic IRQ allocation the effective limit may
actually be smaller (64).
So when now a malicious or buggy userland injects a SPI in that
range, we spill over on our VGIC bitmaps and bytemaps memory.
I could trigger a host kernel NULL pointer dereference with current
mainline by injecting some bogus IRQ number from a hacked kvmtool:
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -195,7 +195,11 @@ struct kvm_arch_memory_slot {
#define KVM_ARM_IRQ_CPU_IRQ 0
#define KVM_ARM_IRQ_CPU_FIQ 1
-/* Highest supported SPI, from VGIC_NR_IRQS */
+/*
+ * This used to hold the highest supported SPI, but it is now obsolete
+ * and only here to provide source code level compatibility with older
+ * userland. The highest SPI number can be set via
KVM_DEV_ARM_VGIC_GRP_NR_IRQS.
+ */
#define KVM_ARM_IRQ_GIC_MAX 127
If that's the case should it maybe only defined when __KERNEL__ is not
defined?
Mmmh, I am not sure it's really worth the hassle. Actually it seems like
that neither kvmtool nor QEMU use this definition, so it's more or less
orphaned by now. I am confident we can avoid it sneaking in in the
kernel again.
TBH, I wouldn't object against Marc enclosing the definition in an
#ifdef __KERNEL__.
Yeah, I'll fix that up (assuming you mean #ifndef rather than #ifdef).
Yes, Monday morning ;)
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
