From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] x86, amd_ucode: Support multiple container files appended together Date: Fri, 20 Jun 2014 22:33:22 -0400 Message-ID: <53A4EEF2.2050009@oracle.com> References: <1403190841-22892-1-git-send-email-aravind.gopalakrishnan@amd.com> <53A459F2.1080706@oracle.com> <53A46CE8.9010106@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53A46CE8.9010106@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, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> + while ( (error = install_equiv_cpu_table(mc_amd, buf, &tot_size, >>> + &offset)) == 0 ) >> >> install_equiv_cpu_table() checks whether container size is larger >> than the buffer size. > Not really. > It is basically checking if we have data at all to parse (like the > comment says:/* No more data */) > >> If we have multiple containers merged I think tot_size is the size of >> the merged file, not of an individual container. > > That's right. > >> And this makes the first check in install_equiv_cpu_table() not >> especially meaningful. >> > > Theoretically- > If it so happens that we don't have any patch files and just the > container header, > then mpbuf->len would be zero and > if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size ) > will fail in this case- (which gives some meaning to the check being > there) > 1. If containers are concatenated, but there is no matching patch on > the first one > 2. So, we fast-fwd to next container file but this one has mpbuf->len=0 So if you have two merged containers, the first one being 1K and the second one empty, then while processing the second container you will instead of returning -EINVAL continue because 'if (0+12 >= 1K) ' will evaluate to false. And it seems to me that you then may well go beyond the end of the buffer. > > Which leads me to another scenario though- > 1. If we do have multiple containers and if right patch is not on the > first one, but on some subsequent container > 2. If the first container has mpbuf->len=0 > Then we'd just error out right there. Hmm.. I'll have to re-work the > logic then. Isn't it the other way around? We won't error out, we will continue. Which is wrong. > > Come to think, if the first container is corrupted for some reason and > returns error (or there's an -ENOMEM), then we are going to return > pre-maturely > Instead, we should probably (try to) forward to next container and > look for patches there as well. > > Shall fix this logic in a v2.. > BTW, it may be worth documenting container format in English. As it stands now, one has to look at the code to figure out the layout. -boris