[PATCH] KVM: x86: Take PIC lock on KVM_GET_IRQCHIP path

[PATCH] KVM: x86: Take PIC lock on KVM_GET_IRQCHIP path

[Carlos López]

When userspace issues the KVM_SET_IRQCHIP ioctl to set the state of
the PIC, kvm_vm_ioctl_set_irqchip() grabs @kvm->arch.vpic->lock before
updating the state. However, the KVM_GET_IRQCHIP ioctl to retrieve the
same PIC state does not grab such lock, potentially causing torn reads
for userspace.

Fix this by grabbing the lock on the read path.

This issue goes all the way back. The bug was introduced with the
addition of PIC ioctl code itself in 6ceb9d791eee ("KVM: Add get/
set irqchip ioctls for in-kernel PIC live migration support"). Later,
894a9c5543ab ("KVM: x86: missing locking in PIT/IRQCHIP/SET_BSP_CPU
ioctl paths") added the locking for kvm_vm_ioctl_set_irqchip(), but
missed kvm_vm_ioctl_get_irqchip().

Fixes: 6ceb9d791eee ("KVM: Add get/set irqchip ioctls for in-kernel PIC live migration support")
Fixes: 894a9c5543ab ("KVM: x86: missing locking in PIT/IRQCHIP/SET_BSP_CPU ioctl paths")
Cc: stable@vger.kernel.org
Reported-by: Claude Code:claude-opus-4.6
Signed-off-by: Carlos López <clopez@suse.de>
---
 arch/x86/kvm/irq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 9519fec09ee6..251df563427b 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -584,14 +584,18 @@ int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 
 	r = 0;
 	switch (chip->chip_id) {
-	case KVM_IRQCHIP_PIC_MASTER:
+	case KVM_IRQCHIP_PIC_MASTER: {
+		guard(spinlock)(&pic->lock);
 		memcpy(&chip->chip.pic, &pic->pics[0],
 			sizeof(struct kvm_pic_state));
 		break;
-	case KVM_IRQCHIP_PIC_SLAVE:
+	}
+	case KVM_IRQCHIP_PIC_SLAVE: {
+		guard(spinlock)(&pic->lock);
 		memcpy(&chip->chip.pic, &pic->pics[1],
 			sizeof(struct kvm_pic_state));
 		break;
+	}
 	case KVM_IRQCHIP_IOAPIC:
 		kvm_get_ioapic(kvm, &chip->chip.ioapic);
 		break;

base-commit: d1568b1332b6b3b36b222c2868fc102727c12a34
-- 
2.51.0

Re: [PATCH] KVM: x86: Take PIC lock on KVM_GET_IRQCHIP path

[Sean Christopherson]

On Fri, May 29, 2026, Carlos López wrote:
> When userspace issues the KVM_SET_IRQCHIP ioctl to set the state of
> the PIC, kvm_vm_ioctl_set_irqchip() grabs @kvm->arch.vpic->lock before
> updating the state. However, the KVM_GET_IRQCHIP ioctl to retrieve the
> same PIC state does not grab such lock, potentially causing torn reads
> for userspace.

Meh, if userspace hasn't fully paused the VM, save/restore is going to fail
anyways.  Heck, torn reads is probably _better_ than the alternative, because
at least that might cause visible failure during the restore.  If there are
concurrent modifications in-flight, then KVM_GET_IRQCHIP is going to return
stale data (assuming userspace doesn't redo KVM_GET_IRQCHIP), i.e. save/restore
will effectively corrupt the guest.

> Fix this by grabbing the lock on the read path.
> 
> This issue goes all the way back. The bug was introduced with the
> addition of PIC ioctl code itself in 6ceb9d791eee ("KVM: Add get/
> set irqchip ioctls for in-kernel PIC live migration support"). Later,
> 894a9c5543ab ("KVM: x86: missing locking in PIT/IRQCHIP/SET_BSP_CPU
> ioctl paths") added the locking for kvm_vm_ioctl_set_irqchip(), but
> missed kvm_vm_ioctl_get_irqchip().
> 
> Fixes: 6ceb9d791eee ("KVM: Add get/set irqchip ioctls for in-kernel PIC live migration support")
> Fixes: 894a9c5543ab ("KVM: x86: missing locking in PIT/IRQCHIP/SET_BSP_CPU ioctl paths")
> Cc: stable@vger.kernel.org

This isn't stable material.  There's basically zero chance this actively
problematic for any VMM.

Honestly, it's tempting to I'm tempted to do the opposite, and yank out the
locking for the KVM_SET_IRQCHIP path, because userspace really can't be relying
on kernel locking for correctness across save/restore.  I don't _actually_ think
we should do that, but it certainly is tempting.

Ah, actually, maybe SET has locking because it's also used to reset PIC state,
i.e. isn't limited to just save/restore?  Doesn't really matter.

> Reported-by: Claude Code:claude-opus-4.6
> Signed-off-by: Carlos López <clopez@suse.de>
> ---
>  arch/x86/kvm/irq.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 9519fec09ee6..251df563427b 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -584,14 +584,18 @@ int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
>  
>  	r = 0;
>  	switch (chip->chip_id) {
> -	case KVM_IRQCHIP_PIC_MASTER:
> +	case KVM_IRQCHIP_PIC_MASTER: {
> +		guard(spinlock)(&pic->lock);

I'd much rather use "manual" spin_(un)lock() instead of guard().  Or scoped_guard()
to avoid the curly braces, but even then, I find this:

		scoped_guard(spinlock, &pic->lock)
			memcpy(&chip->chip.pic, &pic->pics[0],
			       sizeof(struct kvm_pic_state));

to be much harder to read than:

		spin_lock(&pic->lock);
		memcpy(&chip->chip.pic, &pic->pics[0],
			sizeof(struct kvm_pic_state));
		spin_unlock(&pic->lock);

And no one can reasonably argue that guard() or scoped_guard() makes the this
particular code more robust.

>  		memcpy(&chip->chip.pic, &pic->pics[0],
>  			sizeof(struct kvm_pic_state));
>  		break;
> -	case KVM_IRQCHIP_PIC_SLAVE:
> +	}
> +	case KVM_IRQCHIP_PIC_SLAVE: {
> +		guard(spinlock)(&pic->lock);
>  		memcpy(&chip->chip.pic, &pic->pics[1],
>  			sizeof(struct kvm_pic_state));
>  		break;
> +	}
>  	case KVM_IRQCHIP_IOAPIC:
>  		kvm_get_ioapic(kvm, &chip->chip.ioapic);
>  		break;
> 
> base-commit: d1568b1332b6b3b36b222c2868fc102727c12a34
> -- 
> 2.51.0
> 

Re: [PATCH] KVM: x86: Take PIC lock on KVM_GET_IRQCHIP path

[Carlos López]

On 5/29/26 3:24 PM, Sean Christopherson wrote:
> On Fri, May 29, 2026, Carlos López wrote:
>> When userspace issues the KVM_SET_IRQCHIP ioctl to set the state of
>> the PIC, kvm_vm_ioctl_set_irqchip() grabs @kvm->arch.vpic->lock before
>> updating the state. However, the KVM_GET_IRQCHIP ioctl to retrieve the
>> same PIC state does not grab such lock, potentially causing torn reads
>> for userspace.
> 
> Meh, if userspace hasn't fully paused the VM, save/restore is going to fail
> anyways.  Heck, torn reads is probably _better_ than the alternative, because
> at least that might cause visible failure during the restore.  If there are
> concurrent modifications in-flight, then KVM_GET_IRQCHIP is going to return
> stale data (assuming userspace doesn't redo KVM_GET_IRQCHIP), i.e. save/restore
> will effectively corrupt the guest.

Right, do you want a v2 to at least prevent userspace from reading a
torn state? It seems wrong to have this asymmetry with KVM_SET_IRQCHIP
and other save/restore ioctls (e.g. KVM_{G,S}ET_PIT).

>> Fix this by grabbing the lock on the read path.
>>
>> This issue goes all the way back. The bug was introduced with the
>> addition of PIC ioctl code itself in 6ceb9d791eee ("KVM: Add get/
>> set irqchip ioctls for in-kernel PIC live migration support"). Later,
>> 894a9c5543ab ("KVM: x86: missing locking in PIT/IRQCHIP/SET_BSP_CPU
>> ioctl paths") added the locking for kvm_vm_ioctl_set_irqchip(), but
>> missed kvm_vm_ioctl_get_irqchip().
>>
>> Fixes: 6ceb9d791eee ("KVM: Add get/set irqchip ioctls for in-kernel PIC live migration support")
>> Fixes: 894a9c5543ab ("KVM: x86: missing locking in PIT/IRQCHIP/SET_BSP_CPU ioctl paths")
>> Cc: stable@vger.kernel.org
> 
> This isn't stable material.  There's basically zero chance this actively
> problematic for any VMM.
> 
> Honestly, it's tempting to I'm tempted to do the opposite, and yank out the
> locking for the KVM_SET_IRQCHIP path, because userspace really can't be relying
> on kernel locking for correctness across save/restore.  I don't _actually_ think
> we should do that, but it certainly is tempting.
> 
> Ah, actually, maybe SET has locking because it's also used to reset PIC state,
> i.e. isn't limited to just save/restore?  Doesn't really matter.
> 
>> Reported-by: Claude Code:claude-opus-4.6
>> Signed-off-by: Carlos López <clopez@suse.de>
>> ---
>>  arch/x86/kvm/irq.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
>> index 9519fec09ee6..251df563427b 100644
>> --- a/arch/x86/kvm/irq.c
>> +++ b/arch/x86/kvm/irq.c
>> @@ -584,14 +584,18 @@ int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
>>  
>>  	r = 0;
>>  	switch (chip->chip_id) {
>> -	case KVM_IRQCHIP_PIC_MASTER:
>> +	case KVM_IRQCHIP_PIC_MASTER: {
>> +		guard(spinlock)(&pic->lock);
> 
> I'd much rather use "manual" spin_(un)lock() instead of guard().  Or scoped_guard()
> to avoid the curly braces, but even then, I find this:
> 
> 		scoped_guard(spinlock, &pic->lock)
> 			memcpy(&chip->chip.pic, &pic->pics[0],
> 			       sizeof(struct kvm_pic_state));
> 
> to be much harder to read than:
> 
> 		spin_lock(&pic->lock);
> 		memcpy(&chip->chip.pic, &pic->pics[0],
> 			sizeof(struct kvm_pic_state));
> 		spin_unlock(&pic->lock);
> 
> And no one can reasonably argue that guard() or scoped_guard() makes the this
> particular code more robust.

Oh well, the guard seemed more readable to me but I don't have a strong
opinion either way, I can add this to v2 (if you want it).

Re: [PATCH] KVM: x86: Take PIC lock on KVM_GET_IRQCHIP path

[Sean Christopherson]

On Fri, May 29, 2026, Carlos López wrote:
> On 5/29/26 3:24 PM, Sean Christopherson wrote:
> > On Fri, May 29, 2026, Carlos López wrote:
> >> When userspace issues the KVM_SET_IRQCHIP ioctl to set the state of
> >> the PIC, kvm_vm_ioctl_set_irqchip() grabs @kvm->arch.vpic->lock before
> >> updating the state. However, the KVM_GET_IRQCHIP ioctl to retrieve the
> >> same PIC state does not grab such lock, potentially causing torn reads
> >> for userspace.
> > 
> > Meh, if userspace hasn't fully paused the VM, save/restore is going to fail
> > anyways.  Heck, torn reads is probably _better_ than the alternative, because
> > at least that might cause visible failure during the restore.  If there are
> > concurrent modifications in-flight, then KVM_GET_IRQCHIP is going to return
> > stale data (assuming userspace doesn't redo KVM_GET_IRQCHIP), i.e. save/restore
> > will effectively corrupt the guest.
> 
> Right, do you want a v2 to at least prevent userspace from reading a
> torn state? It seems wrong to have this asymmetry with KVM_SET_IRQCHIP
> and other save/restore ioctls (e.g. KVM_{G,S}ET_PIT).

Yeah, please send a v2.  I 100% agree there should be symmetry, which is why
it's tempting to drop the locks for SET :-)