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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

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