linux-kernel - Re: [Stable-review] [56/74] x86, microcode, AMD: Extend ucode size verification

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]

Message-ID: <1302752223.5282.674.camel@localhost>
Date:	Thu, 14 Apr 2011 04:37:03 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Boris Ostrovsky <boris.ostrovsky@....com>
Cc:	linux-kernel@...r.kernel.org, stable@...nel.org,
	Borislav Petkov <borislav.petkov@....com>,
	akpm@...ux-foundation.org, torvalds@...ux-foundation.org,
	stable-review@...nel.org, alan@...rguk.ukuu.org.uk,
	Greg KH <gregkh@...e.de>
Subject: Re: [Stable-review] [56/74] x86, microcode, AMD: Extend ucode size
 verification

On Wed, 2011-04-13 at 08:51 -0700, Greg KH wrote:
> 2.6.32-longterm review patch.  If anyone has any objections, please let us know.
> 
> ------------------
> 
> 
> From: Borislav Petkov <borislav.petkov@....com>
> 
> Upstream commit: 44d60c0f5c58c2168f31df9a481761451840eb54
> 
> The different families have a different max size for the ucode patch,
> adjust size checking to the family we're running on. Also, do not
> vzalloc the max size of the ucode but only the actual size that is
> passed on from the firmware loader.
[...]
> @@ -125,6 +124,37 @@ static int get_matching_microcode(int cp
>  	return 1;
>  }
>  
> +static unsigned int verify_ucode_size(int cpu, const u8 *buf, unsigned int size)
> +{
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	unsigned int max_size, actual_size;
> +
> +#define F1XH_MPB_MAX_SIZE 2048
> +#define F14H_MPB_MAX_SIZE 1824
> +#define F15H_MPB_MAX_SIZE 4096
> +
> +	switch (c->x86) {
> +	case 0x14:
> +		max_size = F14H_MPB_MAX_SIZE;
> +		break;
> +	case 0x15:
> +		max_size = F15H_MPB_MAX_SIZE;
> +		break;
> +	default:
> +		max_size = F1XH_MPB_MAX_SIZE;
> +		break;
> +	}
> +
> +	actual_size = buf[4] + (buf[5] << 8);
> +
> +	if (actual_size > size || actual_size > max_size) {

Surely:

	if (actual_size + UCODE_CONTAINER_SECTION_HDR > size || ...

> +		pr_err("section size mismatch\n");
> +		return 0;
> +	}
> +
> +	return actual_size;
> +}
> +
>  static int apply_microcode_amd(int cpu)
>  {
>  	u32 rev, dummy;
> @@ -164,11 +194,11 @@ static int get_ucode_data(void *to, cons
>  }
>  
>  static void *
> -get_next_ucode(const u8 *buf, unsigned int size, unsigned int *mc_size)
> +get_next_ucode(int cpu, const u8 *buf, unsigned int size, unsigned int *mc_size)
>  {
> -	unsigned int total_size;
> +	unsigned int actual_size = 0;
>  	u8 section_hdr[UCODE_CONTAINER_SECTION_HDR];
> -	void *mc;
> +	void *mc = NULL;

Dummy initialisations mean the compiler won't warn if you fail to
properly initialise them later.

>  	if (get_ucode_data(section_hdr, buf, UCODE_CONTAINER_SECTION_HDR))
>  		return NULL;
> @@ -179,23 +209,18 @@ get_next_ucode(const u8 *buf, unsigned i
>  		return NULL;
>  	}
>  
> -	total_size = (unsigned long) (section_hdr[4] + (section_hdr[5] << 8));
> +	actual_size = verify_ucode_size(cpu, buf, size);
> +	if (!actual_size)
> +		return NULL;
>  
> -	if (total_size > size || total_size > UCODE_MAX_SIZE) {
> -		printk(KERN_ERR "microcode: error: size mismatch\n");
> +	mc = vmalloc(actual_size);
> +	if (!mc)
>  		return NULL;
> -	}
>  
> -	mc = vmalloc(UCODE_MAX_SIZE);
> -	if (mc) {
> -		memset(mc, 0, UCODE_MAX_SIZE);
> -		if (get_ucode_data(mc, buf + UCODE_CONTAINER_SECTION_HDR,
> -				   total_size)) {
> -			vfree(mc);
> -			mc = NULL;
> -		} else
> -			*mc_size = total_size + UCODE_CONTAINER_SECTION_HDR;
> -	}
> +	memset(mc, 0, actual_size);
> +	get_ucode_data(mc, buf + UCODE_CONTAINER_SECTION_HDR, actual_size);
[...]

So I wondered why the result of get_ucode_data() is no longer being
checked.  And the answer is: because it's a trivial wrapper for
memcpy(), but with a 'return 0'.  So the memset() is redundant.

Good thing nothing important depends on this validation, oh wait...

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)