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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/24/2014 06:42 PM, Aravind Gopalakrishnan wrote:
> On 6/24/2014 2:23 AM, Jan Beulich wrote:
>
>>> @@ -272,14 +304,13 @@ static int get_ucode_from_buffer_amd(
>>>     static int install_equiv_cpu_table(
>>>       struct microcode_amd *mc_amd,
>>> -    const uint32_t *buf,
>>> -    size_t *offset)
>>> +    const void *data,
>>> +    size_t *curr_offset)
>> Is there any strong reason to rename "offset" to "curr_offset", ...
>
> No. Have fixed this.
>
>>>   {
>>> +    uint32_t *buf = (uint32_t *) (data + *curr_offset);
>>>       const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>>>   -    /* No more data */
>>> -    if ( mpbuf->len + 12 >= *offset )
>>> -        return -EINVAL;
>> Iirc you and Boris agreed that this check is pointless _here_. But I
>> doubt it can be removed without replacement elsewhere.
>
> For single containers, this check made some sense earlier as we verify 
> to see there is *some*
> data beyond the equivalent_table structure.
> Say, mpbuf->len=0 and we return error val; Due to the fact that we 
> have already advanced *offset,
> cases when we reach EOF or *offset goes over bufsize is handled in 
> container_fast_forward
> function.
>
> For multiple containers, we will always have at least two such 
> container headers and hence,
> mpbuf->len + 12 is always less than total_size
>
> If first container for some reason is corrupted and exposes 
> mpbuf->len=0,  we return EINVAL
> and forward to next container.
> (This is infact one reason to  advance *offset earlier. See below)
>
> Now, if the last container were to have mpbuf->len=0,
> As Boris mentioned on earlier thread, we will
> continue because 'if (0+12 >= tot_size) ' is false.
> Here too, we will return EINVAL.
>
> Again, advancing *offset early allows to workaround these issues.
> And this check can be removed as a result.

Let's say we have a single container and the file got truncated (i.e. 
bufsize in cpu_request_microcode() is smaller than it should be). Aren't 
we now risking doing a memcpy out of too short a buffer?

-boris