From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH] x86, amd_ucode: Support multiple container files appended together Date: Mon, 23 Jun 2014 10:47:30 -0500 Message-ID: <53A84C12.3050908@amd.com> References: <1403190841-22892-1-git-send-email-aravind.gopalakrishnan@amd.com> <53A459F2.1080706@oracle.com> <53A46CE8.9010106@amd.com> <53A4EEF2.2050009@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53A4EEF2.2050009@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: keir@xen.org, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 6/20/2014 9:33 PM, Boris Ostrovsky wrote: > >> >> >>> 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. Hmm. Yes, You are right. This check here seems meaningless.. > And it seems to me that you then may well go beyond the end of the > buffer. > > I don't think so as we will hit if ( mpbuf->len == 0 ) and return -EINVAL; >> >> 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. > No. We should hit- if ( mpbuf->len == 0 ) { printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table length\n"); return -EINVAL; } And with the while loop as it is now will not look for a possible 2nd container. Also, as said below, in case we hit if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) or if ( !mc_amd->equiv_cpu_table ) then we will not enter the loop. I have fixed this now. Will send it in a V2. >> >> 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. > Yeah. I shall work on a draft version and send it as RFC. Thanks, -Aravind.