From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 4/4] x86/microcode_amd: Fail attempts to load a 0-length microcode blob. Date: Tue, 24 Sep 2013 15:31:15 +0100 Message-ID: <5241A233.2060808@citrix.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> <5241A121.2000607@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VOTe3-0004Jc-Cr for xen-devel@lists.xenproject.org; Tue, 24 Sep 2013 14:31:19 +0000 In-Reply-To: <5241A121.2000607@oracle.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: Boris Ostrovsky Cc: xen-devel , Keir Fraser , Suravee Suthikulpanit , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 24/09/13 15:26, Boris Ostrovsky wrote: > 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 may not work as expected. The subtalty here is that we only allocate memory for mc_amd->mpb if mc_amd->mpb_size is less than mpbuf->len. mc_amd->mpb is unconditionally NULL before the first allocation, and mc_amd->mpb_size is 0 (from the caller) Therefore, a user passing 0 for mpbuf->len causes the routine to choose not to allocate any memory, then copy 0 bytes into it. ~Andrew