* [PATCH] KVM: x86: fix access code passed to gva_to_gpa
@ 2020-08-17 18:00 Paolo Bonzini
2020-08-17 18:42 ` Sean Christopherson
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2020-08-17 18:00 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Sean Christopherson
The PK bit of the error code is computed dynamically in permission_fault
and therefore need not be passed to gva_to_gpa: only the access bits
(fetch, user, write) need to be passed down.
Not doing so causes a splat in the pku test:
WARNING: CPU: 25 PID: 5465 at arch/x86/kvm/mmu.h:197 paging64_walk_addr_generic+0x594/0x750 [kvm]
Hardware name: Intel Corporation WilsonCity/WilsonCity, BIOS WLYDCRB1.SYS.0014.D62.2001092233 01/09/2020
RIP: 0010:paging64_walk_addr_generic+0x594/0x750 [kvm]
Code: <0f> 0b e9 db fe ff ff 44 8b 43 04 4c 89 6c 24 30 8b 13 41 39 d0 89
RSP: 0018:ff53778fc623fb60 EFLAGS: 00010202
RAX: 0000000000000001 RBX: ff53778fc623fbf0 RCX: 0000000000000007
RDX: 0000000000000001 RSI: 0000000000000002 RDI: ff4501efba818000
RBP: 0000000000000020 R08: 0000000000000005 R09: 00000000004000e7
R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000007
R13: ff4501efba818388 R14: 10000000004000e7 R15: 0000000000000000
FS: 00007f2dcf31a700(0000) GS:ff4501f1c8040000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000001dea475005 CR4: 0000000000763ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
paging64_gva_to_gpa+0x3f/0xb0 [kvm]
kvm_fixup_and_inject_pf_error+0x48/0xa0 [kvm]
handle_exception_nmi+0x4fc/0x5b0 [kvm_intel]
kvm_arch_vcpu_ioctl_run+0x911/0x1c10 [kvm]
kvm_vcpu_ioctl+0x23e/0x5d0 [kvm]
ksys_ioctl+0x92/0xb0
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x3e/0xb0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
---[ end trace d17eb998aee991da ]---
Reported-by: Sean Christopherson <sean.j.christopherson@intel.com>
Fixes: 897861479c064 ("KVM: x86: Add helper functions for illegal GPA checking and page fault injection")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
arch/x86/kvm/x86.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2db369a64f29..a6e42ce607ca 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10743,9 +10743,13 @@ EXPORT_SYMBOL_GPL(kvm_spec_ctrl_test_value);
void kvm_fixup_and_inject_pf_error(struct kvm_vcpu *vcpu, gva_t gva, u16 error_code)
{
struct x86_exception fault;
+ const unsigned access_mask =
+ PFERR_WRITE_MASK | PFERR_FETCH_MASK | PFERR_USER_MASK;
if (!(error_code & PFERR_PRESENT_MASK) ||
- vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, error_code, &fault) != UNMAPPED_GVA) {
+ vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva,
+ error_code & access_mask,
+ &fault) != UNMAPPED_GVA) {
/*
* If vcpu->arch.walk_mmu->gva_to_gpa succeeded, the page
* tables probably do not match the TLB. Just proceed
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: fix access code passed to gva_to_gpa
2020-08-17 18:00 [PATCH] KVM: x86: fix access code passed to gva_to_gpa Paolo Bonzini
@ 2020-08-17 18:42 ` Sean Christopherson
2020-08-17 18:56 ` Paolo Bonzini
2020-08-17 18:56 ` Paolo Bonzini
0 siblings, 2 replies; 4+ messages in thread
From: Sean Christopherson @ 2020-08-17 18:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm
On Mon, Aug 17, 2020 at 02:00:42PM -0400, Paolo Bonzini wrote:
> The PK bit of the error code is computed dynamically in permission_fault
> and therefore need not be passed to gva_to_gpa: only the access bits
> (fetch, user, write) need to be passed down.
>
> Not doing so causes a splat in the pku test:
>
> WARNING: CPU: 25 PID: 5465 at arch/x86/kvm/mmu.h:197 paging64_walk_addr_generic+0x594/0x750 [kvm]
> Hardware name: Intel Corporation WilsonCity/WilsonCity, BIOS WLYDCRB1.SYS.0014.D62.2001092233 01/09/2020
> RIP: 0010:paging64_walk_addr_generic+0x594/0x750 [kvm]
> Code: <0f> 0b e9 db fe ff ff 44 8b 43 04 4c 89 6c 24 30 8b 13 41 39 d0 89
> RSP: 0018:ff53778fc623fb60 EFLAGS: 00010202
> RAX: 0000000000000001 RBX: ff53778fc623fbf0 RCX: 0000000000000007
> RDX: 0000000000000001 RSI: 0000000000000002 RDI: ff4501efba818000
> RBP: 0000000000000020 R08: 0000000000000005 R09: 00000000004000e7
> R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000007
> R13: ff4501efba818388 R14: 10000000004000e7 R15: 0000000000000000
> FS: 00007f2dcf31a700(0000) GS:ff4501f1c8040000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000001dea475005 CR4: 0000000000763ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
> paging64_gva_to_gpa+0x3f/0xb0 [kvm]
> kvm_fixup_and_inject_pf_error+0x48/0xa0 [kvm]
> handle_exception_nmi+0x4fc/0x5b0 [kvm_intel]
> kvm_arch_vcpu_ioctl_run+0x911/0x1c10 [kvm]
> kvm_vcpu_ioctl+0x23e/0x5d0 [kvm]
> ksys_ioctl+0x92/0xb0
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x3e/0xb0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ---[ end trace d17eb998aee991da ]---
>
> Reported-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Fixes: 897861479c064 ("KVM: x86: Add helper functions for illegal GPA checking and page fault injection")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/x86.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2db369a64f29..a6e42ce607ca 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10743,9 +10743,13 @@ EXPORT_SYMBOL_GPL(kvm_spec_ctrl_test_value);
> void kvm_fixup_and_inject_pf_error(struct kvm_vcpu *vcpu, gva_t gva, u16 error_code)
Side topic, 'struct x86_exception' really should be using a u32 for the
error code. Practically speaking, I expect bits 31:16 will be reserved in
perpetuity, but it's jarring to see 'u16 error_code', and it leads to
pointless discrepancies, e.g. 'u32 access' in ->gva_to_gpa().
> {
> struct x86_exception fault;
> + const unsigned access_mask =
> + PFERR_WRITE_MASK | PFERR_FETCH_MASK | PFERR_USER_MASK;
Don't suppose you'd be in the mood to kill the bare 'unsigned'?
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
>
> if (!(error_code & PFERR_PRESENT_MASK) ||
> - vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, error_code, &fault) != UNMAPPED_GVA) {
> + vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva,
> + error_code & access_mask,
> + &fault) != UNMAPPED_GVA) {
Alternatively, what about capturing the result in a new variable (instead of
defining the mask) to make the wrap suck less (or just overflow like the
current code), e.g.:
u32 access = error_code &
(PFERR_WRITE_MASK | PFERR_FETCH_MASK | PFERR_USER_MASK);
if (!(error_code & PFERR_PRESENT_MASK) ||
vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, access, &fault) != UNMAPPED_GVA) {
> /*
> * If vcpu->arch.walk_mmu->gva_to_gpa succeeded, the page
> * tables probably do not match the TLB. Just proceed
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: fix access code passed to gva_to_gpa
2020-08-17 18:42 ` Sean Christopherson
@ 2020-08-17 18:56 ` Paolo Bonzini
2020-08-17 18:56 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2020-08-17 18:56 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-kernel, kvm
On 17/08/20 20:42, Sean Christopherson wrote:
>
> Don't suppose you'd be in the mood to kill the bare 'unsigned'?
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
u32 is even better.
> Alternatively, what about capturing the result in a new variable (instead of
> defining the mask) to make the wrap suck less (or just overflow like the
> current code), e.g.:
>
> u32 access = error_code &
> (PFERR_WRITE_MASK | PFERR_FETCH_MASK | PFERR_USER_MASK);
>
> if (!(error_code & PFERR_PRESENT_MASK) ||
> vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, access, &fault) != UNMAPPED_GVA) {
Sure, that's better.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: fix access code passed to gva_to_gpa
2020-08-17 18:42 ` Sean Christopherson
2020-08-17 18:56 ` Paolo Bonzini
@ 2020-08-17 18:56 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2020-08-17 18:56 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-kernel, kvm
On 17/08/20 20:42, Sean Christopherson wrote:
>
> Don't suppose you'd be in the mood to kill the bare 'unsigned'?
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
u32 is even better.
> Alternatively, what about capturing the result in a new variable (instead of
> defining the mask) to make the wrap suck less (or just overflow like the
> current code), e.g.:
>
> u32 access = error_code &
> (PFERR_WRITE_MASK | PFERR_FETCH_MASK | PFERR_USER_MASK);
>
> if (!(error_code & PFERR_PRESENT_MASK) ||
> vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, access, &fault) != UNMAPPED_GVA) {
Sure, that's better.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-17 18:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-17 18:00 [PATCH] KVM: x86: fix access code passed to gva_to_gpa Paolo Bonzini
2020-08-17 18:42 ` Sean Christopherson
2020-08-17 18:56 ` Paolo Bonzini
2020-08-17 18:56 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).