Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

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

On 4/25/2014 2:00 AM, Jan Beulich wrote:
>>>> On 24.04.14 at 21:54, <aravind.gopalakrishnan@amd.com> wrote:
>> Each family has a stipulated max patch_size. Use this as
>> additional sanity check before we apply it.
> And iirc there was a size limit check earlier, and it got dropped for one
> reason or another - did you check history?

Yes, I believe you are referring to this commit:

commit 5663cc8258cef27509a437ebd95061b8b01b9c01
Author:     Christoph Egger <Christoph.Egger@amd.com>
AuthorDate: Thu Dec 15 11:00:09 2011 +0100
Commit:     Christoph Egger <Christoph.Egger@amd.com>
CommitDate: Thu Dec 15 11:00:09 2011 +0100

     x86/ucode: fix for AMD Fam15 CPUs

     Remove hardcoded maximum size a microcode patch can have. This is
     dynamic now.

     The microcode patch for family15h can be larger than 2048 bytes and
     gets silently truncated.

     Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
     Signed-off-by: Jan Beulich <jbeulich@suse.com>
     Committed-by: Jan Beulich <jbeulich@suse.com>

The above patch was to make the microcode patch buffer allocation dynamic.
The hunk below simply verifies that we don't exceed the 'max_size'..
>> @@ -94,7 +94,40 @@ static int collect_cpu_info(int cpu, struct cpu_signature *csig)
>>       return 0;
>>   }
>>   
>> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>> +static bool_t verify_patch_size(uint32_t patch_size)
>> +{
>> +    uint8_t family;
>> +    uint32_t max_size;
>> +
>> +#define F1XH_MPB_MAX_SIZE 2048
>> +#define F14H_MPB_MAX_SIZE 1824
>> +#define F15H_MPB_MAX_SIZE 4096
>> +#define F16H_MPB_MAX_SIZE 3458
>> +
>> +    family = boot_cpu_data.x86;
>> +    switch (family)
>> +    {
>> +    case 0x14:
>> +        max_size = F14H_MPB_MAX_SIZE;
>> +        break;
>> +    case 0x15:
>> +        max_size = F15H_MPB_MAX_SIZE;
>> +        break;
>> +    case 0x16:
>> +        max_size = F16H_MPB_MAX_SIZE;
>> +        break;
>> +    default:
>> +        max_size = F1XH_MPB_MAX_SIZE;
>> +        break;
>> +    }
>> +
>> +    if ( patch_size > max_size )
>> +        return 0;
>> +
>> +    return 1;
> Please simply "return patch_size <= max_size" in cases like this.

Okay.

>> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>>   
>>           last_offset = offset;
>>   
>> +        if ( error == -EEXIST ) {
> Coding style, but as Andrew already indicated this block of code isn't
> correct anyway.

Yes, will fix this. But some help in understanding the microcode_init 
calls would help me here..
(Pleas refer reply to Andrew's comments)
>> @@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>       struct microcode_amd *mc_amd = uci->mc.mc_amd;
>>       const struct microcode_amd *src = mc;
>> +    int error;
>>   
>> -    if ( !microcode_fits(src, cpu) )
>> -        return 0;
>> +    error = microcode_fits(src, cpu);
>> +    if ( error )
>> +        return error;
> Is it really correct for this to get switched from success to error return?

Previously, microcode_fits returned '1' for Success, '0' for error. So, 
the condition returns '0' when return val is '0'
This mechanism is still preserved in the changes made above..

>> @@ -383,10 +430,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>>               xfree(mc_amd);
>>           }
>>   
>> +        error = -ENOMEM;
>>           mc_amd = xmalloc(struct microcode_amd);
>>           uci->mc.mc_amd = mc_amd;
>>           if ( !mc_amd )
>> -            return -ENOMEM;
>> +            return error;
> Bogus (pointless) change?
>
>> @@ -408,7 +456,7 @@ err2:
>>   err1:
>>       xfree(mc_amd);
>>       uci->mc.mc_amd = NULL;
>> -    return -ENOMEM;
>> +    return error;
> Same here.
>

Hmm. Okay, Will just revert this.

Thanks,
-Aravind.