From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 4/4] x86/microcode_amd: Fail attempts to load a 0-length microcode blob. Date: Tue, 24 Sep 2013 10:26:41 -0400 Message-ID: <5241A121.2000607@oracle.com> References: <1380024638-14983-1-git-send-email-andrew.cooper3@citrix.com> <1380024638-14983-5-git-send-email-andrew.cooper3@citrix.com> <5241AD7A02000078000F5E40@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VOTXb-0003Iv-4Z for xen-devel@lists.xenproject.org; Tue, 24 Sep 2013 14:24:39 +0000 In-Reply-To: <5241AD7A02000078000F5E40@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , Keir Fraser , Suravee Suthikulpanit , xen-devel List-Id: xen-devel@lists.xenproject.org On 09/24/2013 09:19 AM, Jan Beulich wrote: >>>> On 24.09.13 at 14:10, Andrew Cooper wrote: >> Coverity ID: 1055319 >> >> Coverity identified that when passed a microcode header with a length field of >> 0, get_ucode_from_buffer_amd() would end up calling memcpy(NULL, data, 0) >> which is undefined behaviour. > I think that's at least questionable: memcpy(..., 0) can hardly be > anything but a no-op, no matter whether either of the two pointers > in fact is a NULL one. I think what this patch is trying to prevent is passing NULL pointer to memcpy, not length being zero (if you follow the logic in get_ucode_from_buffer_amd() you will see that destination buffer may not be allocated when length is zero). AMD APM says REP. The REP prefix repeats its associated string instruction the number of times specified in the counter register (rCX). It terminates the repetition when the value in rCX reaches 0. So presumably rCX is checked before rDI and memcpy would indeed be a nop. Still, I think in this particular case (destination can be NULL) the patch is a good thing since we usually (often) check before passing NULL pointer to routines. Alternatively, we can check 'if (mc_amd->mpb != NULL)'. -boris > > That's not to say that I disagree that strict reading of the C standard > may indeed yield this undefined, but I don't think we are to hunt > down all such undefined-nesses. The tool should really stay away > from hair-splitting, and concentrate on pointing out real issues. > > Jan > >> While Xen's implementation of memcpy will do the correct thing in this case, >> any user trying to load a 0 length microcode blob deserves an -EINVAL. >> >> Signed-off-by: Andrew Cooper >> CC: Keir Fraser >> CC: Jan Beulich >> CC: Suravee Suthikulpanit >> CC: Boris Ostrovsky >> --- >> xen/arch/x86/microcode_amd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c >> index a3ceef8..2c767a9 100644 >> --- a/xen/arch/x86/microcode_amd.c >> +++ b/xen/arch/x86/microcode_amd.c >> @@ -202,7 +202,7 @@ static int get_ucode_from_buffer_amd( >> return -EINVAL; >> } >> >> - if ( (off + mpbuf->len) > bufsize ) >> + if ( mpbuf->len == 0 || ((off + mpbuf->len) > bufsize) ) >> { >> printk(KERN_ERR "microcode: Bad data in microcode data file\n"); >> return -EINVAL; >> -- >> 1.7.10.4 > >