Re: [PATCH] KVM: arm/arm64: check IRQ number on userland injection

[Andre Przywara <andre.przywara@arm.com>]

Hi,

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__.

So just to clarify: Isn't this definition part of the userspace API that
we have to maintain forever? That was my understanding when Christopher
suggested to enclose it in #ifndef __KERNEL__ (if I got this correctly).
So we would at least avoid any future kernel code to (ab)use it again.
But putting it inside #ifdef __KERNEL__ does contradict this, doesn't it?

If it is not an API, I'd rather remove it all:
a) it is not used by the kernel anymore after this patch
b) it became meaningless with the introduction of the dynamic NR_IRQS
   allocation
c) none of the two KVM userspace tools (I know of) is using it

So I guess any userspace tool trying to make use of it deserves to not
compile anymore.
But if we somehow promised that any userspace program will compile
forever against even the newest kernel headers, then we have to keep it
in, haven't we?

Or am I totally wrong here?

Cheers,
Andre.