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

[eric.auger@linaro.org (Eric Auger)]

Hi Marc,
On 08/06/2015 06:44 PM, Marc Zyngier wrote:
> On 05/08/15 14:47, Christoffer Dall wrote:
>> On Wed, Aug 05, 2015 at 01:47:27PM +0200, Eric Auger wrote:
>>> 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@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.
>>>
>> I don't see anything wrong, and I thought this was what we discussed.
> 
> I think I just lost track of what we've discussed at LCA. Ignore me.
> 
> I'll repost the updated first 10 patches tomorrow. Eric, are you willing
> to take custody of patch 11 as part of one of your series?
Yes no problem I can take this over if you prefer. I can easily test it.
May I put it in the irq forwarding series?

Best Regards

Eric
> 
> Thanks,
> 
> 	M.
>