lkml.org 
[lkml]   [2011]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [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@amd.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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2011-04-14 05:39    [W:0.230 / U:9.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site