Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752602AbdLMJ5E (ORCPT ); Wed, 13 Dec 2017 04:57:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54970 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751853AbdLMJ5A (ORCPT ); Wed, 13 Dec 2017 04:57:00 -0500 Subject: Re: [PATCH RESEND] KVM: X86: Fix stack-out-of-bounds read in write_mmio To: Wanpeng Li Cc: "linux-kernel@vger.kernel.org" , kvm , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Wanpeng Li References: <1513069078-63339-1-git-send-email-wanpeng.li@hotmail.com> From: Paolo Bonzini Message-ID: <17a3382f-ce45-6fcb-66fe-f14dd4107436@redhat.com> Date: Wed, 13 Dec 2017 10:56:57 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Wed, 13 Dec 2017 09:57:00 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1433 Lines: 40 On 13/12/2017 04:05, Wanpeng Li wrote: > 2017-12-13 0:07 GMT+08:00 Paolo Bonzini : >> On 12/12/2017 09:57, Wanpeng Li wrote: >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index bc5d853..51e7932 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -4690,7 +4690,10 @@ 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); >>> + u64 data = 0; >>> + >>> + memcpy(&data, val, min(8, bytes)); >>> + trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, data); >>> return vcpu_mmio_write(vcpu, gpa, bytes, val); >>> } >>> >>> >> >> Please do the memcpy in TRACE_EVENT(kvm_mmio)'s TP_fast_assign block. >> That is done only when the trace event is active. > > I still can observe the stack out-of-bounds read warning if keep *(u64 > *)val as the parameter since it has already been dereferenced. So > maybe we should change the parameter of trace_kvm_mmio() to void *val, Yes, exactly. > however, I'm not sure whether it will break the tracepoint ABI which > this article https://lwn.net/Articles/734039/ "Workload analysis with > tracing" section pointed out. No, the tracepoint ABI refers to TP_PROTO, not TP_STRUCT and TP_fast_assign. Good that you thought about it, though! Thanks, Paolo