| Message ID | jpgtvqhuhj3.fsf@linux.bootlegged.copy (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/06/2018 22:04, Bandan Das wrote: > > Certain instructions such as sgdt/sidt call segmented_write_std that > doesn't propagate access correctly. As such, during userspace induced > exception, the guest can incorrectly assume that the exception > happened in the kernel and panic. The emulated write function > segmented_write does seem to check access correctly. > > Reported-by: Andy Lutomirski <luto@kernel.org> > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > arch/x86/kvm/x86.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 71e7cda6d014..871265f6a35f 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4824,10 +4824,11 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt, > struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); > void *data = val; > int r = X86EMUL_CONTINUE; > + u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0; > > while (bytes) { > gpa_t gpa = vcpu->arch.walk_mmu->gva_to_gpa(vcpu, addr, > - PFERR_WRITE_MASK, > + access | PFERR_WRITE_MASK, > exception); > unsigned offset = addr & (PAGE_SIZE-1); > unsigned towrite = min(bytes, (unsigned)PAGE_SIZE - offset); > No, that's why the function is called "*_system". The problem is that this is not the function that should be used when emulating sidt/sgdt. Instead, kvm_write_guest_virt should be used. Thanks, Paolo
On 06.06.2018 15:30, Paolo Bonzini wrote: > On 05/06/2018 22:04, Bandan Das wrote: >> Certain instructions such as sgdt/sidt call segmented_write_std that >> doesn't propagate access correctly. As such, during userspace induced >> exception, the guest can incorrectly assume that the exception >> happened in the kernel and panic. The emulated write function >> segmented_write does seem to check access correctly. >> >> Reported-by: Andy Lutomirski <luto@kernel.org> >> Signed-off-by: Bandan Das <bsd@redhat.com> >> --- >> arch/x86/kvm/x86.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 71e7cda6d014..871265f6a35f 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4824,10 +4824,11 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt, >> struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); >> void *data = val; >> int r = X86EMUL_CONTINUE; >> + u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0; >> >> while (bytes) { >> gpa_t gpa = vcpu->arch.walk_mmu->gva_to_gpa(vcpu, addr, >> - PFERR_WRITE_MASK, >> + access | PFERR_WRITE_MASK, >> exception); >> unsigned offset = addr & (PAGE_SIZE-1); >> unsigned towrite = min(bytes, (unsigned)PAGE_SIZE - offset); >> > No, that's why the function is called "*_system". The problem is that > this is not the function that should be used when emulating sidt/sgdt. > Instead, kvm_write_guest_virt should be used. > > Thanks, > > Paolo Does this mean guest user space can use sidt to write its kernel memory? Where is kvm_write_guest_virt defined ? --Mika
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 71e7cda6d014..871265f6a35f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4824,10 +4824,11 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt, struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); void *data = val; int r = X86EMUL_CONTINUE; + u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0; while (bytes) { gpa_t gpa = vcpu->arch.walk_mmu->gva_to_gpa(vcpu, addr, - PFERR_WRITE_MASK, + access | PFERR_WRITE_MASK, exception); unsigned offset = addr & (PAGE_SIZE-1); unsigned towrite = min(bytes, (unsigned)PAGE_SIZE - offset);
Certain instructions such as sgdt/sidt call segmented_write_std that doesn't propagate access correctly. As such, during userspace induced exception, the guest can incorrectly assume that the exception happened in the kernel and panic. The emulated write function segmented_write does seem to check access correctly. Reported-by: Andy Lutomirski <luto@kernel.org> Signed-off-by: Bandan Das <bsd@redhat.com> --- arch/x86/kvm/x86.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)