[v1,01/22] KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP
diff mbox

Message ID 20170314133450.13259-2-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand March 14, 2017, 1:34 p.m. UTC
Avoid races between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP by taking
the kvm->lock when setting up routes.

If KVM_CREATE_IRQCHIP fails, KVM_SET_GSI_ROUTING could have already set
up routes pointing at pic/ioapic, being silently removed already.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 virt/kvm/kvm_main.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Peter Xu March 15, 2017, 6:24 a.m. UTC | #1
On Tue, Mar 14, 2017 at 02:34:29PM +0100, David Hildenbrand wrote:
> Avoid races between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP by taking
> the kvm->lock when setting up routes.
> 
> If KVM_CREATE_IRQCHIP fails, KVM_SET_GSI_ROUTING could have already set
> up routes pointing at pic/ioapic, being silently removed already.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a17d787..ad0f8b2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3079,8 +3079,11 @@ static long kvm_vm_ioctl(struct file *filp,
>  					   routing.nr * sizeof(*entries)))
>  				goto out_free_irq_routing;
>  		}
> +		/* avoid races with KVM_CREATE_IRQCHIP on x86 */
> +		mutex_lock(&kvm->lock);
>  		r = kvm_set_irq_routing(kvm, entries, routing.nr,
>  					routing.flags);
> +		mutex_unlock(&kvm->lock);
>  out_free_irq_routing:
>  		vfree(entries);
>  		break;
> -- 
> 2.9.3
> 

Out of my curiousity: do we have a use case that these two operations
might collapse (KVM_CREATE_IRQCHIP and KVM_SET_GSI_ROUTING)? Or is
this patch only for the sake of security?

Another thing to mention is that, I guess adding this lock will
benefit KVM_CAP_SPLIT_IRQCHIP as well, maybe nice to mention it too in
the commit message. No worth a repost for this single reason though.

Thanks,

-- peterx
David Hildenbrand March 15, 2017, 9:19 a.m. UTC | #2
Am 15.03.2017 um 07:24 schrieb Peter Xu:
> On Tue, Mar 14, 2017 at 02:34:29PM +0100, David Hildenbrand wrote:
>> Avoid races between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP by taking
>> the kvm->lock when setting up routes.
>>
>> If KVM_CREATE_IRQCHIP fails, KVM_SET_GSI_ROUTING could have already set
>> up routes pointing at pic/ioapic, being silently removed already.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  virt/kvm/kvm_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index a17d787..ad0f8b2 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3079,8 +3079,11 @@ static long kvm_vm_ioctl(struct file *filp,
>>  					   routing.nr * sizeof(*entries)))
>>  				goto out_free_irq_routing;
>>  		}
>> +		/* avoid races with KVM_CREATE_IRQCHIP on x86 */
>> +		mutex_lock(&kvm->lock);
>>  		r = kvm_set_irq_routing(kvm, entries, routing.nr,
>>  					routing.flags);
>> +		mutex_unlock(&kvm->lock);
>>  out_free_irq_routing:
>>  		vfree(entries);
>>  		break;
>> -- 
>> 2.9.3
>>
> 
> Out of my curiousity: do we have a use case that these two operations
> might collapse (KVM_CREATE_IRQCHIP and KVM_SET_GSI_ROUTING)? Or is
> this patch only for the sake of security?

Just for the sake of security. I think, in general such calls are not
supposed to be done in parallel. But of course, user space can.

> 
> Another thing to mention is that, I guess adding this lock will
> benefit KVM_CAP_SPLIT_IRQCHIP as well, maybe nice to mention it too in
> the commit message. No worth a repost for this single reason though.
> 

I added

"Also, as a side effect, this patch makes sure that KVM_SET_GSI_ROUTING
and KVM_CAP_SPLIT_IRQCHIP cannot run in parallel."

Thanks!

> Thanks,
> 
> -- peterx
>
Peter Xu March 15, 2017, 9:28 a.m. UTC | #3
On Wed, Mar 15, 2017 at 10:19:53AM +0100, David Hildenbrand wrote:
> Am 15.03.2017 um 07:24 schrieb Peter Xu:
> > On Tue, Mar 14, 2017 at 02:34:29PM +0100, David Hildenbrand wrote:
> >> Avoid races between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP by taking
> >> the kvm->lock when setting up routes.
> >>
> >> If KVM_CREATE_IRQCHIP fails, KVM_SET_GSI_ROUTING could have already set
> >> up routes pointing at pic/ioapic, being silently removed already.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  virt/kvm/kvm_main.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index a17d787..ad0f8b2 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -3079,8 +3079,11 @@ static long kvm_vm_ioctl(struct file *filp,
> >>  					   routing.nr * sizeof(*entries)))
> >>  				goto out_free_irq_routing;
> >>  		}
> >> +		/* avoid races with KVM_CREATE_IRQCHIP on x86 */
> >> +		mutex_lock(&kvm->lock);
> >>  		r = kvm_set_irq_routing(kvm, entries, routing.nr,
> >>  					routing.flags);
> >> +		mutex_unlock(&kvm->lock);
> >>  out_free_irq_routing:
> >>  		vfree(entries);
> >>  		break;
> >> -- 
> >> 2.9.3
> >>
> > 
> > Out of my curiousity: do we have a use case that these two operations
> > might collapse (KVM_CREATE_IRQCHIP and KVM_SET_GSI_ROUTING)? Or is
> > this patch only for the sake of security?
> 
> Just for the sake of security. I think, in general such calls are not
> supposed to be done in parallel. But of course, user space can.

I see.

> 
> > 
> > Another thing to mention is that, I guess adding this lock will
> > benefit KVM_CAP_SPLIT_IRQCHIP as well, maybe nice to mention it too in
> > the commit message. No worth a repost for this single reason though.
> > 
> 
> I added
> 
> "Also, as a side effect, this patch makes sure that KVM_SET_GSI_ROUTING
> and KVM_CAP_SPLIT_IRQCHIP cannot run in parallel."

Yeah it looks better. Thanks!

-- peterx

Patch
diff mbox

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a17d787..ad0f8b2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3079,8 +3079,11 @@  static long kvm_vm_ioctl(struct file *filp,
 					   routing.nr * sizeof(*entries)))
 				goto out_free_irq_routing;
 		}
+		/* avoid races with KVM_CREATE_IRQCHIP on x86 */
+		mutex_lock(&kvm->lock);
 		r = kvm_set_irq_routing(kvm, entries, routing.nr,
 					routing.flags);
+		mutex_unlock(&kvm->lock);
 out_free_irq_routing:
 		vfree(entries);
 		break;