From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH V2] x86, amd_ucode: Support multiple container files appended together Date: Wed, 25 Jun 2014 00:04:18 -0400 Message-ID: <53AA4A42.2090304@oracle.com> References: <1403555119-12793-1-git-send-email-aravind.gopalakrishnan@amd.com> <53A94389020000780001CADA@mail.emea.novell.com> <53A9FED0.7090203@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53A9FED0.7090203@amd.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: Aravind Gopalakrishnan Cc: keir@xen.org, Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 06/24/2014 06:42 PM, Aravind Gopalakrishnan wrote: > On 6/24/2014 2:23 AM, Jan Beulich wrote: > >>> @@ -272,14 +304,13 @@ static int get_ucode_from_buffer_amd( >>> static int install_equiv_cpu_table( >>> struct microcode_amd *mc_amd, >>> - const uint32_t *buf, >>> - size_t *offset) >>> + const void *data, >>> + size_t *curr_offset) >> Is there any strong reason to rename "offset" to "curr_offset", ... > > No. Have fixed this. > >>> { >>> + uint32_t *buf = (uint32_t *) (data + *curr_offset); >>> const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1]; >>> - /* No more data */ >>> - if ( mpbuf->len + 12 >= *offset ) >>> - return -EINVAL; >> Iirc you and Boris agreed that this check is pointless _here_. But I >> doubt it can be removed without replacement elsewhere. > > For single containers, this check made some sense earlier as we verify > to see there is *some* > data beyond the equivalent_table structure. > Say, mpbuf->len=0 and we return error val; Due to the fact that we > have already advanced *offset, > cases when we reach EOF or *offset goes over bufsize is handled in > container_fast_forward > function. > > For multiple containers, we will always have at least two such > container headers and hence, > mpbuf->len + 12 is always less than total_size > > If first container for some reason is corrupted and exposes > mpbuf->len=0, we return EINVAL > and forward to next container. > (This is infact one reason to advance *offset earlier. See below) > > Now, if the last container were to have mpbuf->len=0, > As Boris mentioned on earlier thread, we will > continue because 'if (0+12 >= tot_size) ' is false. > Here too, we will return EINVAL. > > Again, advancing *offset early allows to workaround these issues. > And this check can be removed as a result. Let's say we have a single container and the file got truncated (i.e. bufsize in cpu_request_microcode() is smaller than it should be). Aren't we now risking doing a memcpy out of too short a buffer? -boris