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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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.