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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7c47bfe2-3293-2973-d9bb-daa218d5751b@maciej.szmigiero.name>
Date:   Thu, 14 Jun 2018 22:56:07 +0200
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 3/8] x86/microcode/AMD: Check microcode container data
 in the early loader

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