linux-kernel - Re: [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 26 Jun 2018 00:18:44 +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 2/2] x86/microcode/AMD: Check patch size for all known CPU
 families

On 25.06.2018 20:37, Borislav Petkov wrote:
(..)
>> +		break;
>>  	case 0x14:
>>  		max_size = F14H_MPB_MAX_SIZE;
>>  		break;
>> @@ -251,22 +239,41 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>>  		max_size = F17H_MPB_MAX_SIZE;
>>  		break;
>>  	default:
>> -		max_size = F1XH_MPB_MAX_SIZE;
>> +		/*
>> +		 * Don't know the max size for future families...
>> +		 * Set a patch length limit of slightly less than U32_MAX to
>> +		 * prevent overflowing 32-bit variables holding the whole
>> +		 * patch section size.
>> +		 */
>> +		max_size = U32_MAX - SECTION_HDR_SIZE;
> 
> No, just do a WARN_ON_ONCE here.

So you want to have just WARN_ON_ONCE and no max_size (or fam_size) check
in such case of an unknown family number?

We still need to cap patch_size at that default case above value (or less)
so adding the section header size later won't overflow.

> 
>>  		break;
>>  	}
>>  
>> -	/*
>> -	 * The section header length is not included in this indicated size
>> -	 * but is present in the leftover file length so we need to subtract
>> -	 * it from the leftover file length.
>> -	 */
>> -	if (patch_size > min_t(u32, buf_size - SECTION_HDR_SIZE, max_size)) {
>> +	if (patch_size > max_size) {
>> +		if (!early)
>> +			pr_err("Patch of size %u exceeds family %u maximum.\n",
>> +			       patch_size, (unsigned int)patch_fam);
>> +
>> +		*crnt_size = min_t(unsigned int,
>> +				   SECTION_HDR_SIZE + max_size,
>> +				   buf_size);
>> +		return false;
>> +	}
>> +
>> +	if (buf_size - SECTION_HDR_SIZE < patch_size) {
>>  		if (!early)
>> -			pr_err("Patch of size %u too large.\n", patch_size);
>> +			pr_err("Patch of size %u truncated.\n", patch_size);
>>  
>> +		*crnt_size = buf_size;
>>  		return false;
>>  	}
>>  
>> +	*crnt_size = SECTION_HDR_SIZE + patch_size;
>> +
>> +	/* Is the patch for the proper CPU family? */
>> +	if (family != patch_fam)
>> +		return false;
> 
> Why are you moving the family check down?
> 
> What it should do instead is the moment it knows the family, check
> whether they're equal. If not, skip over with minimum of the size of the
> corresponding patch_fam and buf_size.
> 
> And then you do all the remaining checks.
> 

Previously the family check was done before computing patch_fam so the
biggest component of the move was jumping over past that computation
(which we have to do anyway to know how many bytes to skip).

Moving the family check further down past these two last checks visible
in the patch hunk above allows us to:
1) Produce an error message that the indicated patch size exceeds family
maximum - for CPU families other that the currently running one.

This is useful in case the patch actually should be of that indicated
size - for some reason - since in this case by skipping over a smaller
length we will end in the middle of a patch and so we'll produce a
bunch of error messages about invalid patch section type field, etc.,
until the driver finally resynchronizes on the next section boundary.

2) In case the last patch (for another family) was truncated a proper
diagnostic message would be printed instead of the patch being skipped
silently.

Maciej