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
