Re: [PATCH v6 3/8] x86/microcode/AMD: Check microcode container data in the early loader

[Maciej S. Szmigiero]

On 05.06.2018 10:54, Borislav Petkov wrote:
(..)
>> @@ -258,25 +265,27 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
>>  
>>  		hdr = (u32 *)buf;
>>  
>> -		if (hdr[0] != UCODE_UCODE_TYPE)
>> +		if (!verify_patch_section(buf, size, true))
>>  			break;
>>  
>> -		/* Sanity-check patch size. */
>>  		patch_size = hdr[1];
>> -		if (patch_size > PATCH_MAX_SIZE)
>> -			break;
>>  
>> -		/* Skip patch section header: */
>> -		buf  += SECTION_HDR_SIZE;
>> -		size -= SECTION_HDR_SIZE;
>> +		mc = (struct microcode_amd *)(buf + SECTION_HDR_SIZE);
>> +		if (eq_id != mc->hdr.processor_rev_id)
>> +			goto next_patch;
>>  
>> -		mc = (struct microcode_amd *)buf;
>> -		if (eq_id == mc->hdr.processor_rev_id) {
>> -			desc->psize = patch_size;
>> -			desc->mc = mc;
>> -		}
>> +		if (!verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
>> +				  true))
> 
> Let it stick out.
> 
> Ok, so above you do verify_patch_section() and then you take patch_size
> without fully verifying it - it can be something non-sensically huge and
> thus we might skip over good patches.
> 
> What you should do instead is call verify_patch() directly - which
> already calls verify_patch_section() and if the patch size exceeds the
> per-family maximum, return *that* instead and skip only the per family
> maximum inside the buffer so that any patches following can get a chance
> to get inspected.

verify_patch_section() does only very basic patch section checks -
more or less whether the section, its header and a patch header exist and
can be accessed at all.

Here, a check can be added whether the indicated patch size does not
exceed PATCH_MAX_SIZE to catch nonsensically huge patch sizes and to
skip only a maximum patch length of that many bytes.

At this point we don't know the CPU family the particular patch is for
since the patch header contains only CPU rev_id, not an explicit family
number.

Only the late loader is able to translate back this CPU rev_id to its
family number via the CPU equivalence table, the early loader simply
skips patches that have CPU rev_id different from the current CPU one -
so it can always use the current CPU family number for a patch
verification.

But either way, in order to read the CPU rev_id in the patch
header we have to verify its presence in the microcode container file.
This is what verify_patch_section() does.

Then, once the particular loader determines the family number for a
patch, verify_patch() does additional per-family size check.
In principle, this function could be renamed verify_patch_family_size()
and have call to verify_patch_section() at its beginning dropped.

Maciej