Messages in this thread | | | Subject | Re: [PATCH v6 3/8] x86/microcode/AMD: Check microcode container data in the early loader | From | "Maciej S. Szmigiero" <> | Date | Thu, 14 Jun 2018 22:56:07 +0200 |
| |
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
| |