Message ID | 0471d0150a944519d4d3b7fb6f40cb6dd1d43dd0.1515045457.git.khalid.elmously@canonical.com |
---|---|
State | New |
Headers | show |
Series | CVE-2017-17741 | expand |
On 01/04/18 07:58, Khalid Elmously wrote: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > Reported by syzkaller: > > BUG: KASAN: stack-out-of-bounds in write_mmio+0x11e/0x270 [kvm] > Read of size 8 at addr ffff8803259df7f8 by task syz-executor/32298 > > CPU: 6 PID: 32298 Comm: syz-executor Tainted: G OE 4.15.0-rc2+ #18 > Hardware name: LENOVO ThinkCentre M8500t-N000/SHARKBAY, BIOS FBKTC1AUS 02/16/2016 > Call Trace: > dump_stack+0xab/0xe1 > print_address_description+0x6b/0x290 > kasan_report+0x28a/0x370 > write_mmio+0x11e/0x270 [kvm] > emulator_read_write_onepage+0x311/0x600 [kvm] > emulator_read_write+0xef/0x240 [kvm] > emulator_fix_hypercall+0x105/0x150 [kvm] > em_hypercall+0x2b/0x80 [kvm] > x86_emulate_insn+0x2b1/0x1640 [kvm] > x86_emulate_instruction+0x39a/0xb90 [kvm] > handle_exception+0x1b4/0x4d0 [kvm_intel] > vcpu_enter_guest+0x15a0/0x2640 [kvm] > kvm_arch_vcpu_ioctl_run+0x549/0x7d0 [kvm] > kvm_vcpu_ioctl+0x479/0x880 [kvm] > do_vfs_ioctl+0x142/0x9a0 > SyS_ioctl+0x74/0x80 > entry_SYSCALL_64_fastpath+0x23/0x9a > > The path of patched vmmcall will patch 3 bytes opcode 0F 01 C1(vmcall) > to the guest memory, however, write_mmio tracepoint always prints 8 bytes > through *(u64 *)val since kvm splits the mmio access into 8 bytes. This > leaks 5 bytes from the kernel stack (CVE-2017-17741). This patch fixes > it by just accessing the bytes which we operate on. > > Before patch: > > syz-executor-5567 [007] .... 51370.561696: kvm_mmio: mmio write len 3 gpa 0x10 val 0x1ffff10077c1010f > > After patch: > > syz-executor-13416 [002] .... 51302.299573: kvm_mmio: mmio write len 3 gpa 0x10 val 0xc1010f > > > (backported from e39d200fa5bf5b94a0948db0dae44c1b73b84a56) > > > CVE-2017-17741 > > References: CVE-2017-17741 > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Tested-by: Marc Zyngier <marc.zyngier@arm.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> The backport looks good to me: Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> However, some fixes need to be done when applying this patch: - Remove the CVE number from the subject line, it should have been added between square brackets. - Fix the '(backported from ...)' line adding it to the right place. Thanks, Kleber > > > --- > arch/arm/kvm/mmio.c | 4 ++-- > arch/x86/kvm/x86.c | 8 ++++---- > include/trace/events/kvm.h | 7 +++++-- > 3 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c > index 4cb5a93182e9..c47f108c2b8a 100644 > --- a/arch/arm/kvm/mmio.c > +++ b/arch/arm/kvm/mmio.c > @@ -113,7 +113,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) > } > > trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, > - data); > + &data); > data = vcpu_data_host_to_guest(vcpu, data, len); > *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data; > } > @@ -192,7 +192,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, > trace_kvm_mmio((mmio.is_write) ? KVM_TRACE_MMIO_WRITE : > KVM_TRACE_MMIO_READ_UNSATISFIED, > mmio.len, fault_ipa, > - (mmio.is_write) ? data : 0); > + (mmio.is_write) ? &data : NULL); > > if (mmio.is_write) > mmio_write_buf(mmio.data, mmio.len, data); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 7d4f352d6fe4..1768223c702a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3952,7 +3952,7 @@ static int vcpu_mmio_read(struct kvm_vcpu *vcpu, gpa_t addr, int len, void *v) > !kvm_iodevice_read(&vcpu->arch.apic->dev, addr, n, v)) > && kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, addr, n, v)) > break; > - trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, *(u64 *)v); > + trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, v); > handled += n; > addr += n; > len -= n; > @@ -4175,7 +4175,7 @@ static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes) > { > if (vcpu->mmio_read_completed) { > trace_kvm_mmio(KVM_TRACE_MMIO_READ, bytes, > - vcpu->mmio_fragments[0].gpa, *(u64 *)val); > + vcpu->mmio_fragments[0].gpa, val); > vcpu->mmio_read_completed = 0; > return 1; > } > @@ -4197,14 +4197,14 @@ static int write_emulate(struct kvm_vcpu *vcpu, gpa_t gpa, > > static int write_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, int bytes, void *val) > { > - trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, *(u64 *)val); > + trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, val); > return vcpu_mmio_write(vcpu, gpa, bytes, val); > } > > static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, > void *val, int bytes) > { > - trace_kvm_mmio(KVM_TRACE_MMIO_READ_UNSATISFIED, bytes, gpa, 0); > + trace_kvm_mmio(KVM_TRACE_MMIO_READ_UNSATISFIED, bytes, gpa, NULL); > return X86EMUL_IO_NEEDED; > } > > diff --git a/include/trace/events/kvm.h b/include/trace/events/kvm.h > index 131a0bda7aec..bb71db2d884d 100644 > --- a/include/trace/events/kvm.h > +++ b/include/trace/events/kvm.h > @@ -163,7 +163,7 @@ TRACE_EVENT(kvm_ack_irq, > { KVM_TRACE_MMIO_WRITE, "write" } > > TRACE_EVENT(kvm_mmio, > - TP_PROTO(int type, int len, u64 gpa, u64 val), > + TP_PROTO(int type, int len, u64 gpa, void *val), > TP_ARGS(type, len, gpa, val), > > TP_STRUCT__entry( > @@ -177,7 +177,10 @@ TRACE_EVENT(kvm_mmio, > __entry->type = type; > __entry->len = len; > __entry->gpa = gpa; > - __entry->val = val; > + __entry->val = 0; > + if (val) > + memcpy(&__entry->val, val, > + min_t(u32, sizeof(__entry->val), len)); > ), > > TP_printk("mmio %s len %u gpa 0x%llx val 0x%llx", >
Applied to trusty On 2018-01-04 01:58:11 , Khalid Elmously wrote: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > Reported by syzkaller: > > BUG: KASAN: stack-out-of-bounds in write_mmio+0x11e/0x270 [kvm] > Read of size 8 at addr ffff8803259df7f8 by task syz-executor/32298 > > CPU: 6 PID: 32298 Comm: syz-executor Tainted: G OE 4.15.0-rc2+ #18 > Hardware name: LENOVO ThinkCentre M8500t-N000/SHARKBAY, BIOS FBKTC1AUS 02/16/2016 > Call Trace: > dump_stack+0xab/0xe1 > print_address_description+0x6b/0x290 > kasan_report+0x28a/0x370 > write_mmio+0x11e/0x270 [kvm] > emulator_read_write_onepage+0x311/0x600 [kvm] > emulator_read_write+0xef/0x240 [kvm] > emulator_fix_hypercall+0x105/0x150 [kvm] > em_hypercall+0x2b/0x80 [kvm] > x86_emulate_insn+0x2b1/0x1640 [kvm] > x86_emulate_instruction+0x39a/0xb90 [kvm] > handle_exception+0x1b4/0x4d0 [kvm_intel] > vcpu_enter_guest+0x15a0/0x2640 [kvm] > kvm_arch_vcpu_ioctl_run+0x549/0x7d0 [kvm] > kvm_vcpu_ioctl+0x479/0x880 [kvm] > do_vfs_ioctl+0x142/0x9a0 > SyS_ioctl+0x74/0x80 > entry_SYSCALL_64_fastpath+0x23/0x9a > > The path of patched vmmcall will patch 3 bytes opcode 0F 01 C1(vmcall) > to the guest memory, however, write_mmio tracepoint always prints 8 bytes > through *(u64 *)val since kvm splits the mmio access into 8 bytes. This > leaks 5 bytes from the kernel stack (CVE-2017-17741). This patch fixes > it by just accessing the bytes which we operate on. > > Before patch: > > syz-executor-5567 [007] .... 51370.561696: kvm_mmio: mmio write len 3 gpa 0x10 val 0x1ffff10077c1010f > > After patch: > > syz-executor-13416 [002] .... 51302.299573: kvm_mmio: mmio write len 3 gpa 0x10 val 0xc1010f > > > (backported from e39d200fa5bf5b94a0948db0dae44c1b73b84a56) > > > CVE-2017-17741 > > References: CVE-2017-17741 > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Tested-by: Marc Zyngier <marc.zyngier@arm.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > > > --- > arch/arm/kvm/mmio.c | 4 ++-- > arch/x86/kvm/x86.c | 8 ++++---- > include/trace/events/kvm.h | 7 +++++-- > 3 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c > index 4cb5a93182e9..c47f108c2b8a 100644 > --- a/arch/arm/kvm/mmio.c > +++ b/arch/arm/kvm/mmio.c > @@ -113,7 +113,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) > } > > trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, > - data); > + &data); > data = vcpu_data_host_to_guest(vcpu, data, len); > *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data; > } > @@ -192,7 +192,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, > trace_kvm_mmio((mmio.is_write) ? KVM_TRACE_MMIO_WRITE : > KVM_TRACE_MMIO_READ_UNSATISFIED, > mmio.len, fault_ipa, > - (mmio.is_write) ? data : 0); > + (mmio.is_write) ? &data : NULL); > > if (mmio.is_write) > mmio_write_buf(mmio.data, mmio.len, data); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 7d4f352d6fe4..1768223c702a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3952,7 +3952,7 @@ static int vcpu_mmio_read(struct kvm_vcpu *vcpu, gpa_t addr, int len, void *v) > !kvm_iodevice_read(&vcpu->arch.apic->dev, addr, n, v)) > && kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, addr, n, v)) > break; > - trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, *(u64 *)v); > + trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, v); > handled += n; > addr += n; > len -= n; > @@ -4175,7 +4175,7 @@ static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes) > { > if (vcpu->mmio_read_completed) { > trace_kvm_mmio(KVM_TRACE_MMIO_READ, bytes, > - vcpu->mmio_fragments[0].gpa, *(u64 *)val); > + vcpu->mmio_fragments[0].gpa, val); > vcpu->mmio_read_completed = 0; > return 1; > } > @@ -4197,14 +4197,14 @@ static int write_emulate(struct kvm_vcpu *vcpu, gpa_t gpa, > > static int write_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, int bytes, void *val) > { > - trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, *(u64 *)val); > + trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, val); > return vcpu_mmio_write(vcpu, gpa, bytes, val); > } > > static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, > void *val, int bytes) > { > - trace_kvm_mmio(KVM_TRACE_MMIO_READ_UNSATISFIED, bytes, gpa, 0); > + trace_kvm_mmio(KVM_TRACE_MMIO_READ_UNSATISFIED, bytes, gpa, NULL); > return X86EMUL_IO_NEEDED; > } > > diff --git a/include/trace/events/kvm.h b/include/trace/events/kvm.h > index 131a0bda7aec..bb71db2d884d 100644 > --- a/include/trace/events/kvm.h > +++ b/include/trace/events/kvm.h > @@ -163,7 +163,7 @@ TRACE_EVENT(kvm_ack_irq, > { KVM_TRACE_MMIO_WRITE, "write" } > > TRACE_EVENT(kvm_mmio, > - TP_PROTO(int type, int len, u64 gpa, u64 val), > + TP_PROTO(int type, int len, u64 gpa, void *val), > TP_ARGS(type, len, gpa, val), > > TP_STRUCT__entry( > @@ -177,7 +177,10 @@ TRACE_EVENT(kvm_mmio, > __entry->type = type; > __entry->len = len; > __entry->gpa = gpa; > - __entry->val = val; > + __entry->val = 0; > + if (val) > + memcpy(&__entry->val, val, > + min_t(u32, sizeof(__entry->val), len)); > ), > > TP_printk("mmio %s len %u gpa 0x%llx val 0x%llx", > -- > 2.14.1 >
diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c index 4cb5a93182e9..c47f108c2b8a 100644 --- a/arch/arm/kvm/mmio.c +++ b/arch/arm/kvm/mmio.c @@ -113,7 +113,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) } trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, - data); + &data); data = vcpu_data_host_to_guest(vcpu, data, len); *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data; } @@ -192,7 +192,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, trace_kvm_mmio((mmio.is_write) ? KVM_TRACE_MMIO_WRITE : KVM_TRACE_MMIO_READ_UNSATISFIED, mmio.len, fault_ipa, - (mmio.is_write) ? data : 0); + (mmio.is_write) ? &data : NULL); if (mmio.is_write) mmio_write_buf(mmio.data, mmio.len, data); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 7d4f352d6fe4..1768223c702a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3952,7 +3952,7 @@ static int vcpu_mmio_read(struct kvm_vcpu *vcpu, gpa_t addr, int len, void *v) !kvm_iodevice_read(&vcpu->arch.apic->dev, addr, n, v)) && kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, addr, n, v)) break; - trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, *(u64 *)v); + trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, v); handled += n; addr += n; len -= n; @@ -4175,7 +4175,7 @@ static int read_prepare(struct kvm_vcpu *vcpu, void *val, int bytes) { if (vcpu->mmio_read_completed) { trace_kvm_mmio(KVM_TRACE_MMIO_READ, bytes, - vcpu->mmio_fragments[0].gpa, *(u64 *)val); + vcpu->mmio_fragments[0].gpa, val); vcpu->mmio_read_completed = 0; return 1; } @@ -4197,14 +4197,14 @@ static int write_emulate(struct kvm_vcpu *vcpu, gpa_t gpa, static int write_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, int bytes, void *val) { - trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, *(u64 *)val); + trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, val); return vcpu_mmio_write(vcpu, gpa, bytes, val); } static int read_exit_mmio(struct kvm_vcpu *vcpu, gpa_t gpa, void *val, int bytes) { - trace_kvm_mmio(KVM_TRACE_MMIO_READ_UNSATISFIED, bytes, gpa, 0); + trace_kvm_mmio(KVM_TRACE_MMIO_READ_UNSATISFIED, bytes, gpa, NULL); return X86EMUL_IO_NEEDED; } diff --git a/include/trace/events/kvm.h b/include/trace/events/kvm.h index 131a0bda7aec..bb71db2d884d 100644 --- a/include/trace/events/kvm.h +++ b/include/trace/events/kvm.h @@ -163,7 +163,7 @@ TRACE_EVENT(kvm_ack_irq, { KVM_TRACE_MMIO_WRITE, "write" } TRACE_EVENT(kvm_mmio, - TP_PROTO(int type, int len, u64 gpa, u64 val), + TP_PROTO(int type, int len, u64 gpa, void *val), TP_ARGS(type, len, gpa, val), TP_STRUCT__entry( @@ -177,7 +177,10 @@ TRACE_EVENT(kvm_mmio, __entry->type = type; __entry->len = len; __entry->gpa = gpa; - __entry->val = val; + __entry->val = 0; + if (val) + memcpy(&__entry->val, val, + min_t(u32, sizeof(__entry->val), len)); ), TP_printk("mmio %s len %u gpa 0x%llx val 0x%llx",