KVM: arm/arm64: vgic: fix off-by-one bug in vgic_get_irq()
PATCH 1/1 KVM: arm/arm64: vgic: fix off-by-one bug in vgic_get_irq()
When using the nospec API, it should be taken into account that:
"...if the CPU speculates past the bounds check then
* array_index_nospec() will clamp the index within the range of [0,
* size)."
The above is part of the header for macro array_index_nospec() in linux/nospec.h
Now, in this particular case, if intid evaluates to exactly VGIC_MAX_SPI or to exaclty VGIC_MAX_PRIVATE, the array_index_nospec() macro ends up returning VGIC_MAX_SPI - 1 or VGIC_MAX_PRIVATE - 1 respectively, instead of VGIC_MAX_SPI or VGIC_MAX_PRIVATE, which, based on the original logic:
/* SGIs and PPIs */ if (intid <= VGIC_MAX_PRIVATE)
return &vcpu->arch.vgic_cpu.private_irqs[intid];
/* SPIs */
if (intid <= VGIC_MAX_SPI)
return &kvm->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
are valid values for intid.
Fix this by calling array_index_nospec() macro with VGIC_MAX_PRIVATE + 1 and VGIC_MAX_SPI + 1 as arguments for its parameter size.
--- virt/kvm/arm/vgic/vgic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
virt/kvm/arm/vgic/vgic.c +2 -2
@@ -103,13 +103,13 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu, {/* SGIs and PPIs */ if (intid <=VGIC_MAX_PRIVATE ) { - intid =array_index_nospec (intid,VGIC_MAX_PRIVATE ); + intid =array_index_nospec (intid,VGIC_MAX_PRIVATE +1 );return &vcpu->arch.vgic_cpu.private_irqs[intid]; }/* SPIs */ if (intid <=VGIC_MAX_SPI ) { - intid =array_index_nospec (intid,VGIC_MAX_SPI ); + intid =array_index_nospec (intid,VGIC_MAX_SPI +1 );return &kvm->arch.vgic.spis[intid -VGIC_NR_PRIVATE_IRQS ]; }
Signature · 1 line
2.17.1
Hi Gustavo,
Quoted 45 lines · Gustavo A. R. Silva
When using the nospec API, it should be taken into account that:
"...if the CPU speculates past the bounds check then
*array_index_nospec()will clamp the index within the range of [0,
* size)."The above is part of the header for macro
array_index_nospec()inlinux/nospec.hNow, in this particular case, if intid evaluates to exactly
VGIC_MAX_SPIor to exacltyVGIC_MAX_PRIVATE, thearray_index_nospec()macro ends up returningVGIC_MAX_SPI- 1 orVGIC_MAX_PRIVATE- 1 respectively, instead ofVGIC_MAX_SPIorVGIC_MAX_PRIVATE, which, based on the original logic:/* SGIs and PPIs */ if (intid <= VGIC_MAX_PRIVATE)virt/kvm/arm/vgic/vgic.c +0
-0return &vcpu->arch.vgic_cpu.private_irqs[intid];/* SPIs */ if (intid <=
VGIC_MAX_SPI)
return &kvm->arch.vgic.spis[intid -VGIC_NR_PRIVATE_IRQS];are valid values for intid.
Fix this by calling
array_index_nospec()macro withVGIC_MAX_PRIVATE+ 1 andVGIC_MAX_SPI+ 1 as arguments for its parameter size.Fixes 41b87599c743 ("KVM: arm/arm64: vgic: fix possible spectre-v1 in vgic_get_irq()")Signed-off-by Gustavo A. R. Silva <[email protected]>--- virt/kvm/arm/vgic/vgic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)virt/kvm/arm/vgic/vgic.c +1
-1@@ -103,13 +103,13 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu, {/* SGIs and PPIs */ if (intid <=VGIC_MAX_PRIVATE ) { - intid =array_index_nospec (intid,VGIC_MAX_PRIVATE ); + intid =array_index_nospec (intid,VGIC_MAX_PRIVATE +1 );
virt/kvm/arm/vgic/vgic.c struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu, · line 115
Quoted 10 lines
virt/kvm/arm/vgic/vgic.c +1
-1return &vcpu->arch.vgic_cpu.private_irqs[intid]; }/* SPIs */ if (intid <=VGIC_MAX_SPI ) { - intid =array_index_nospec (intid,VGIC_MAX_SPI ); + intid =array_index_nospec (intid,VGIC_MAX_SPI +1 );return &kvm->arch.vgic.spis[intid -VGIC_NR_PRIVATE_IRQS ]; }
That's a good one. Out of curiosity, how did you find it?
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...