[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] x86, amd_ucode: Support multiple container files appended together



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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.