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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

> > > +    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


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