Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing

From: Borislav Petkov
Date: Tue Jan 17 2017 - 18:32:39 EST

On Tue, Jan 17, 2017 at 09:29:05PM +0100, Thomas Gleixner wrote:
> Stupid question. Why do we need to walk through that blob if we already
> know that it does not contain a patch for this cpu, i.e. eq_id == 0 ?
>
> I assume it has to do with the multiple containers glued together in the
> blob, but that should be mentioned in the comment.

It is mentioned a bit further down in the comment:

/*
* If we have found an eq_id, it means we're looking at the container
* which has a patch for this CPU so return 0 to mean, @ucode already
* points to it and it will be parsed later. Otherwise, we return the
* size we scanned so that we can advance to the next container in the
* buffer.
*/

> So here you set patch_size and mc when the eq_id is matching. I assume we
> continue the scan for the same reason as we do the scan for eq_id = 0, right?

Yes. Also, theoretically, some later container might hold an even newer
patch so we will update that pointer when we get there.

> Now this one is dangerous. If the blob is corrupted we might have exited
> the loop above due to
>
> > + if (hdr[0] != UCODE_UCODE_TYPE)
> > + break;
>
> before the eq_id matching happened. In that case we return success, but
> desc->psize and desc->mc are not set. Not what you want, right?

Ah, good catch. I guess I'll have to do here:

/* Something corrupted the container, invalidate it. */
eq_id = 0;
break;

too.

> So this becomes
> > + return ret;
>
> which is not really better than the original code. I think we really should
> only use the variable when there is something which can change between two
> points, but that's my personal preference and up to you :)

Ok, changed. This check goes away later anyway - it gets moved to the
BSP early path.

> Why are you storing the id when you don't have an idea whether the patch is
> actually available and useable? There might be a proper reason, but w/o a
> comment or access to the microcode crystalball it's hard to tell.

That is gone later, as you discovered yourself.

The thing is, it is hard to change things in this driver one at a time
without breaking it so I had to restrain myself.

Updated and untested version below. Testing is scheduled for the coming
days :-)

---
From: Borislav Petkov <bp@xxxxxxx>
Date: Sun, 18 Dec 2016 20:53:09 +0100
Subject: [PATCH] x86/microcode/AMD: Rework container parsing

It was pretty clumsy before and the whole work of parsing the microcode
containers was spread around the functions wrongly.

Clean it up so that there's a main scan_containers() function which
iterates over the microcode blob and picks apart the containers glued
together. For each container, it calls a parse_container() helper which
concentrates on one container only: sanity-checking, parsing, counting
microcode patches in there, etc.

It makes much more sense now and it is actually very readable. Oh, and
we luvz a diffstat removing more crap than adding.

Signed-off-by: Borislav Petkov <bp@xxxxxxx>
---
arch/x86/kernel/cpu/microcode/amd.c | 242 ++++++++++++++++--------------------
1 file changed, 110 insertions(+), 132 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6935ceaab4fa..52dffc590ff0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -64,43 +64,6 @@ static u16 this_equiv_id;
static const char
ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";

-static size_t compute_container_size(u8 *data, u32 total_size)
-{
- size_t size = 0;
- u32 *header = (u32 *)data;
-
- if (header[0] != UCODE_MAGIC ||
- header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
- header[2] == 0) /* size */
- return size;
-
- size = header[2] + CONTAINER_HDR_SZ;
- total_size -= size;
- data += size;
-
- while (total_size) {
- u16 patch_size;
-
- header = (u32 *)data;
-
- if (header[0] != UCODE_UCODE_TYPE)
- break;
-
- /*
- * Sanity-check patch size.
- */
- patch_size = header[1];
- if (patch_size > PATCH_MAX_SIZE)
- break;
-
- size += patch_size + SECTION_HDR_SIZE;
- data += patch_size + SECTION_HDR_SIZE;
- total_size -= patch_size + SECTION_HDR_SIZE;
- }
-
- return size;
-}
-
static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
{
for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
@@ -115,80 +78,112 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
* This scans the ucode blob for the proper container as we can have multiple
* containers glued together. Returns the equivalence ID from the equivalence
* table or 0 if none found.
+ * Returns the amount of bytes consumed while scanning. @desc contains all the
+ * data we're going to use in later stages of the application.
*/
-static u16
-find_proper_container(u8 *ucode, size_t size, struct cont_desc *desc)
+static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
{
- struct cont_desc ret = { 0 };
- u32 eax, ebx, ecx, edx;
struct equiv_cpu_entry *eq;
- int offset, left;
- u16 eq_id = 0;
- u32 *header;
- u8 *data;
+ ssize_t orig_size = size;
+ u32 *hdr = (u32 *)ucode;
+ u32 eax, ebx, ecx, edx;
+ u16 eq_id;
+ u8 *buf;

- data = ucode;
- left = size;
- header = (u32 *)data;
+ /* Am I looking at an equivalence table header? */
+ if (hdr[0] != UCODE_MAGIC ||
+ hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
+ hdr[2] == 0) {
+ desc->eq_id = 0;
+ return CONTAINER_HDR_SZ;
+ }

+ buf = ucode;

- /* find equiv cpu table */
- if (header[0] != UCODE_MAGIC ||
- header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
- header[2] == 0) /* size */
- return eq_id;
+ eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);

- eax = 0x00000001;
+ eax = 1;
ecx = 0;
native_cpuid(&eax, &ebx, &ecx, &edx);

- while (left > 0) {
- eq = (struct equiv_cpu_entry *)(data + CONTAINER_HDR_SZ);
-
- ret.data = data;
+ /* Find the equivalence ID of our CPU in this table: */
+ eq_id = find_equiv_id(eq, eax);

- /* Advance past the container header */
- offset = header[2] + CONTAINER_HDR_SZ;
- data += offset;
- left -= offset;
+ buf += hdr[2] + CONTAINER_HDR_SZ;
+ size -= hdr[2] + CONTAINER_HDR_SZ;

- eq_id = find_equiv_id(eq, eax);
- if (eq_id) {
- ret.size = compute_container_size(ret.data, left + offset);
+ /*
+ * Scan through the rest of the container to find where it ends. We do
+ * some basic sanity-checking too.
+ */
+ while (size > 0) {
+ struct microcode_amd *mc;
+ u32 patch_size;

- /*
- * truncate how much we need to iterate over in the
- * ucode update loop below
- */
- left = ret.size - offset;
+ hdr = (u32 *)buf;

- *desc = ret;
- return eq_id;
+ if (hdr[0] != UCODE_UCODE_TYPE) {
+ /* Invalidate container. */
+ eq_id = 0;
+ break;
}

- /*
- * support multiple container files appended together. if this
- * one does not have a matching equivalent cpu entry, we fast
- * forward to the next container file.
- */
- while (left > 0) {
- header = (u32 *)data;
+ /* Sanity-check patch size. */
+ patch_size = hdr[1];
+ if (patch_size > PATCH_MAX_SIZE) {
+ /* Something corrupted the container, invalidate it. */
+ eq_id = 0;
+ break;
+ }

- if (header[0] == UCODE_MAGIC &&
- header[1] == UCODE_EQUIV_CPU_TABLE_TYPE)
- break;
+ /* Skip patch section header: */
+ buf += SECTION_HDR_SIZE;
+ size -= SECTION_HDR_SIZE;

- offset = header[1] + SECTION_HDR_SIZE;
- data += offset;
- left -= offset;
+ mc = (struct microcode_amd *)buf;
+ if (eq_id == mc->hdr.processor_rev_id) {
+ desc->psize = patch_size;
+ desc->mc = mc;
}

- /* mark where the next microcode container file starts */
- offset = data - (u8 *)ucode;
- ucode = data;
+ buf += patch_size;
+ size -= patch_size;
+ }
+
+ /*
+ * If we have found an eq_id, it means we're looking at the container
+ * which has a patch for this CPU so return 0 to mean, @ucode already
+ * points to it and it will be parsed later. Otherwise, we return the
+ * size we scanned so that we can advance to the next container in the
+ * buffer.
+ */
+ if (eq_id) {
+ desc->eq_id = eq_id;
+ desc->data = ucode;
+ desc->size = orig_size - size;
+
+ return 0;
}

- return eq_id;
+ return orig_size - size;
+}
+
+/*
+ * Scan the ucode blob for the proper container as we can have multiple
+ * containers glued together.
+ */
+static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
+{
+ ssize_t rem = size;
+
+ while (rem >= 0) {
+ ssize_t s = parse_container(ucode, rem, desc);
+ if (!s)
+ return;
+
+ ucode += s;
+ rem -= s;
+ }
}

static int __apply_microcode_amd(struct microcode_amd *mc)
@@ -214,17 +209,16 @@ static int __apply_microcode_amd(struct microcode_amd *mc)
* load_microcode_amd() to save equivalent cpu table and microcode patches in
* kernel heap memory.
*
- * Returns true if container found (sets @ret_cont), false otherwise.
+ * Returns true if container found (sets @desc), false otherwise.
*/
static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
- struct cont_desc *desc)
+ struct cont_desc *ret_desc)
{
+ struct cont_desc desc = { 0 };
u8 (*patch)[PATCH_MAX_SIZE];
- u32 rev, *header, *new_rev;
- struct cont_desc ret;
- int offset, left;
- u16 eq_id = 0;
- u8 *data;
+ struct microcode_amd *mc;
+ u32 rev, *new_rev;
+ bool ret = false;

#ifdef CONFIG_X86_32
new_rev = (u32 *)__pa_nodebug(&ucode_new_rev);
@@ -237,47 +231,31 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
if (check_current_patch_level(&rev, true))
return false;

- eq_id = find_proper_container(ucode, size, &ret);
- if (!eq_id)
- return false;
-
- this_equiv_id = eq_id;
- header = (u32 *)ret.data;
-
- /* We're pointing to an equiv table, skip over it. */
- data = ret.data + header[2] + CONTAINER_HDR_SZ;
- left = ret.size - (header[2] + CONTAINER_HDR_SZ);
-
- while (left > 0) {
- struct microcode_amd *mc;
-
- header = (u32 *)data;
- if (header[0] != UCODE_UCODE_TYPE || /* type */
- header[1] == 0) /* size */
- break;
+ scan_containers(ucode, size, &desc);
+ if (!desc.eq_id)
+ return ret;

- mc = (struct microcode_amd *)(data + SECTION_HDR_SIZE);
+ this_equiv_id = desc.eq_id;

- if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id) {
+ mc = desc.mc;
+ if (!mc)
+ return ret;

- if (!__apply_microcode_amd(mc)) {
- rev = mc->hdr.patch_id;
- *new_rev = rev;
+ if (rev >= mc->hdr.patch_id)
+ return ret;

- if (save_patch)
- memcpy(patch, mc, min_t(u32, header[1], PATCH_MAX_SIZE));
- }
- }
+ if (!__apply_microcode_amd(mc)) {
+ *new_rev = mc->hdr.patch_id;
+ ret = true;

- offset = header[1] + SECTION_HDR_SIZE;
- data += offset;
- left -= offset;
+ if (save_patch)
+ memcpy(patch, mc, min_t(u32, desc.psize, PATCH_MAX_SIZE));
}

- if (desc)
- *desc = ret;
+ if (ret_desc)
+ *ret_desc = desc;

- return true;
+ return ret;
}

static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
@@ -396,6 +374,7 @@ void load_ucode_amd_ap(unsigned int family)
}

if (!apply_microcode_early_amd(cp.data, cp.size, false, &cont)) {
+ cont.data = NULL;
cont.size = -1;
return;
}
@@ -434,7 +413,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
{
enum ucode_state ret;
int retval = 0;
- u16 eq_id;

if (!cont.data) {
if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {
@@ -450,8 +428,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
return -EINVAL;
}

- eq_id = find_proper_container(cp.data, cp.size, &cont);
- if (!eq_id) {
+ scan_containers(cp.data, cp.size, &cont);
+ if (!cont.eq_id) {
cont.size = -1;
return -EINVAL;
}
--
2.11.0


--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.