[PATCH v3 09/11] KVM: arm/arm64: vgic: Prevent userspace injection of a mapped interrupt

[Eric Auger]

On 08/05/2015 12:53 PM, Christoffer Dall wrote:
> On Wed, Aug 05, 2015 at 10:44:09AM +0100, Marc Zyngier wrote:
>> On 05/08/15 08:32, Eric Auger wrote:
>>> Hi Marc,
>>> On 08/04/2015 06:44 PM, Marc Zyngier wrote:
>>>> On 04/08/15 17:21, Eric Auger wrote:
>>>>> Hi Marc,
>>>>> On 07/24/2015 05:55 PM, Marc Zyngier wrote:
>>>>>> Virtual interrupts mapped to a HW interrupt should only be triggered
>>>>>> from inside the kernel. Otherwise, you could end up confusing the
>>>>>> kernel (and the GIC's) state machine.
>>>>>>
>>>>>> Rearrange the injection path so that kvm_vgic_inject_irq is
>>>>>> used for non-mapped interrupts, and kvm_vgic_inject_mapped_irq is
>>>>>> used for mapped interrupts. The latter should only be called from
>>>>>> inside the kernel (timer, VFIO).
>>>>> nit: I would replace VFIO by irqfd.
>>>>> VFIO just triggers the eventfd/irqfd. This is KVM/irqfd that injects the
>>>>> virtual irq upon the irqfd signaling and he irqfd adaptation/ARM
>>>>> currently is implemented in vgic.c
>>>>
>>>> Ah, thanks for reminding me of the right terminology, I tend to think of
>>>> it as one big bag of nasty tricks... ;-)
>>>>
>>>> I'll update the commit message.
>>>>
>>>>>>
>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>>>>>> ---
>>>>>>  include/kvm/arm_vgic.h |  2 +
>>>>>>  virt/kvm/arm/vgic.c    | 99 ++++++++++++++++++++++++++++++++++----------------
>>>>>>  2 files changed, 70 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>>>> index 7306b4b..f6bfd79 100644
>>>>>> --- a/include/kvm/arm_vgic.h
>>>>>> +++ b/include/kvm/arm_vgic.h
>>>>>> @@ -351,6 +351,8 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
>>>>>>  void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>>>>>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>>>>  			bool level);
>>>>>> +int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>>>>>> +			       struct irq_phys_map *map, bool level);
>>>>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>>>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>>>>>  int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
>>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>>> index 3f7b690..e40ef70 100644
>>>>>> --- a/virt/kvm/arm/vgic.c
>>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>>> @@ -1533,7 +1533,8 @@ static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>>>>>>  }
>>>>>>  
>>>>>>  static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>>>>>> -				  unsigned int irq_num, bool level)
>>>>>> +				   struct irq_phys_map *map,
>>>>>> +				   unsigned int irq_num, bool level)
>>>>>>  {
>>>>> In vgic_update_irq_pending, I needed to modify the following line and
>>>>> add the "&& !map".
>>>>>
>>>>>         if (!vgic_validate_injection(vcpu, irq_num, level) && !map) {
>>>>>
>>>>> Without that, the level being not properly modeled for level sensitive
>>>>> forwarded IRQs, the 2d injection fails.
>>>>
>>>> Ah! Is that because we never see the line being reset to zero, and the
>>>> VGIC still sees the line as pending at the distributor level?
>>> yes indeed
>>
>> Then it is a bigger problem we need to solve, and your solution just
>> papers over the issue.
>>
>> The main problem is that irqfd is essentially an edge-triggered
>> signalling. Fire and forget. Given that we're dealing with a level
>> triggered interrupt, we end up with the interrupt still marked as
>> pending (nobody took the signal down).
this does not really relate to irqfd: irqfd also comes with the concept
of resamplefd. in case the IRQ is not forwarded, this is the
irqfd_resampler_ack function that toggles the IRQ down (eventfd.c). Its
execution is triggered in vgic_process_maintenance. With forwarding the
EOI is not trappable anymore so this disappears.


>> The usual way to get out of that mess in is to evaluate the state of the
>> level on EOI. But we can't trap on EOI for a HW interrupt.
>>
>> So it raises the question: should we instead consider the HW pending
>> state instead of the software one for mapped interrupts? It is
>> expensive, but it feels more correct.
>>
> I thought we already covered this at LCA.  For mapped interrupts
> (forwarded) we should never consider the software pending state, because
> that state is managed by the hardware.  Or am I confusing concepts here?

Yes we discussed we should bypass most of the SW states.

in my previous integration I proposed a patch "KVM: arm: vgic: fix state
machine for forwarded IRQ". What's wrong with the below approach?


    Fix multiple injection of level sensitive forwarded IRQs.
    With current code, the second injection fails since the
    state bitmaps are not reset (process_maintenance is not
    called anymore).

    New implementation follows those principles:
    - A forwarded IRQ only can be sampled when it is pending
    - when queueing the IRQ (programming the LR), the pending state
      is removed as for edge sensitive IRQs
    - an injection of a forwarded IRQ is considered always valid since
      coming from the HW and level always is 1.

Eric

> 
> Thanks,
> -Christoffer
>