kvm: vmx: fix limit checking in get_vmx_mem_address()
PATCH v2 1/2 kvm: vmx: fix limit checking in get_vmx_mem_address()
Intel SDM vol. 3, 5.3:
The processor causes a
general-protection exception (or, if the segment is SS, a stack-fault exception) any time an attempt is made to access the following addresses in a segment:
- A byte at an offset greater than the effective limit
- A word at an offset greater than the (effective-limit – 1)
- A doubleword at an offset greater than the (effective-limit – 3)
- A quadword at an offset greater than the (effective-limit – 7)
Therefore, the generic limit checking error condition must be
exn = off > limit + 1 - operand_len
but not
exn = off + operand_len > limit
as for now.
--- arch/x86/kvm/vmx/nested.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/vmx/nested.c +1 -1
@@ -4115,7 +4115,7 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,*/ if (!(s.base ==0 && s.limit ==0xffffffff && ((s.type &8 ) || !(s.type &4 )))) - exn = exn || (off +sizeof (u64) > s.limit); + exn = exn || (off > s.limit +1 -sizeof (u64)); }if (exn) { kvm_queue_exception_e(vcpu,
Signature · 1 line
2.21.0
Quoted 12 lines · Eugene Korenevsky
Intel SDM vol. 3, 5.3:
The processor causes a
general-protection exception (or, if the segment is SS, a stack-fault exception) any time an attempt is made to access the following addresses in a segment:
- A byte at an offset greater than the effective limit
- A word at an offset greater than the (effective-limit – 1)
- A doubleword at an offset greater than the (effective-limit – 3)
- A quadword at an offset greater than the (effective-limit – 7)Therefore, the generic limit checking error condition must be
Quoted 7 lines
exn = off > limit + 1 -
operand_lenbut not
exn = off +
operand_len> limitas for now.
Probably worth adding a note in the changelog about the access size being hardcoded to quadword. It's difficult to correlate the code with the changelog without the context of the following patch to add 'len'.
Quoted 10 lines
Signed-off-by Eugene Korenevsky <[email protected]>--- arch/x86/kvm/vmx/nested.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/kvm/vmx/nested.c +0
-0
arch/x86/kvm/vmx/nested.c int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, · line 4118
Quoted 5 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -4115,7 +4115,7 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,*/ if (!(s.base ==0 && s.limit ==0xffffffff && ((s.type &8 ) || !(s.type &4 )))) - exn = exn || (off +sizeof (u64) > s.limit); + exn = exn || (off > s.limit +1 -sizeof (u64));
Adjusting the limit will wrap a small limit, e.g. s.limit=3 will check @off against 0xfffffffc. And IMO, "off + sizeof(u64) - 1 > s.limit" is more intuitive anyways, e.g. it conveys that we're calculating the address of the last byte being accessed and checking to see if that would cause a limit violation.
On a related note, there's a pre-existing wrap bug for 32-bit KVM since @off is a 32-bit value (gva_t is unsigned long), but that's easily fixed by casting @off to a u64.
Quoted 6 lines
arch/x86/kvm/vmx/nested.c +0
-0}if (exn) { kvm_queue_exception_e(vcpu,Signature · 1 line
2.21.0