Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560845 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 06/13] x86/microcode/AMD: Rework container parsing Borislav Petkov <bp@alien8.de> - 2017-01-17 18:40 +0100
    Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 21:40 +0100
      Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing Borislav Petkov <bp@alien8.de> - 2017-01-18 00:40 +0100
        Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing Borislav Petkov <bp@alien8.de> - 2017-01-18 15:50 +0100
          Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing Borislav Petkov <bp@alien8.de> - 2017-01-18 16:00 +0100

#1560845 — [PATCH 06/13] x86/microcode/AMD: Rework container parsing

From: Borislav Petkov <bp@suse.de>

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@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 241 ++++++++++++++++--------------------
 1 file changed, 108 insertions(+), 133 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 7073588a2a09..1893f41c2d29 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)
 {
 	int i = 0;
@@ -121,80 +84,109 @@ 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);
+	/* Find the equivalence ID of our CPU in this table: */
+	eq_id = find_equiv_id(eq, eax);
 
-		ret.data = data;
+	buf  += hdr[2] + CONTAINER_HDR_SZ;
+	size -= hdr[2] + CONTAINER_HDR_SZ;
 
-		/* Advance past the container header */
-		offset = header[2] + CONTAINER_HDR_SZ;
-		data  += offset;
-		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;
 
-		eq_id = find_equiv_id(eq, eax);
-		if (eq_id) {
-			ret.size = compute_container_size(ret.data, left + offset);
+		hdr = (u32 *)buf;
 
-			/*
-			 * truncate how much we need to iterate over in the
-			 * ucode update loop below
-			 */
-			left = ret.size - offset;
+		if (hdr[0] != UCODE_UCODE_TYPE)
+			break;
 
-			*desc = ret;
-			return eq_id;
+		/* Sanity-check patch size. */
+		patch_size = hdr[1];
+		if (patch_size > PATCH_MAX_SIZE) {
+			/* Something corrupted the container, invalidate it. */
+			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;
+		/* Skip patch section header: */
+		buf  += SECTION_HDR_SIZE;
+		size -= SECTION_HDR_SIZE;
 
-			if (header[0] == UCODE_MAGIC &&
-			    header[1] == UCODE_EQUIV_CPU_TABLE_TYPE)
-				break;
-
-			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)
@@ -220,17 +212,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);
@@ -241,49 +232,33 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
 #endif
 
 	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;
+		return ret;
 
-		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)
@@ -402,6 +377,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;
 		}
@@ -440,7 +416,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)) {
@@ -456,8 +431,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

[toc] | [next] | [standalone]

#1560992

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> +	/* Find the equivalence ID of our CPU in this table: */
> +	eq_id = find_equiv_id(eq, eax);

So here we figure out whether the blob claims to have a patch for that cpu.

> +	/*
> +	 * Scan through the rest of the container to find where it ends. We do
> +	 * some basic sanity-checking too.
> +	 */

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.

> +	while (size > 0) {
> +		struct microcode_amd *mc;
> +		u32 patch_size;
>  
> -		eq_id = find_equiv_id(eq, eax);
> -		if (eq_id) {
> -			ret.size = compute_container_size(ret.data, left + offset);
> +		hdr = (u32 *)buf;
>  
> -			/*
> -			 * truncate how much we need to iterate over in the
> -			 * ucode update loop below
> -			 */
> -			left = ret.size - offset;
> +		if (hdr[0] != UCODE_UCODE_TYPE)
> +			break;
>  
> -			*desc = ret;
> -			return eq_id;
> +		/* Sanity-check patch size. */
> +		patch_size = hdr[1];
> +		if (patch_size > PATCH_MAX_SIZE) {
> +			/* Something corrupted the container, invalidate it. */
> +			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;
> +		/* Skip patch section header: */
> +		buf  += SECTION_HDR_SIZE;
> +		size -= SECTION_HDR_SIZE;
>  
> -			if (header[0] == UCODE_MAGIC &&
> -			    header[1] == UCODE_EQUIV_CPU_TABLE_TYPE)
> -				break;
> -
> -			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;

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?

>  		}
>  
> -		/* 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) {

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?

> +		desc->eq_id = eq_id;
> +		desc->data  = ucode;
> +		desc->size  = orig_size - size;
> +
> +		return 0;
  
> @@ -241,49 +232,33 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
>  #endif
>  
>  	if (check_current_patch_level(&rev, true))
> -		return false;

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 :)

> +	if (!desc.eq_id)
> +		return ret;
>  
> -		mc = (struct microcode_amd *)(data + SECTION_HDR_SIZE);
> +	this_equiv_id = desc.eq_id;

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.

>  static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
> @@ -402,6 +377,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;

What's the point of fiddling with the local variable at all if we return
right away?

>  			return;
>  		}
> @@ -440,7 +416,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)) {
> @@ -456,8 +431,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;

Ditto. That might be fixed in a seperate patch because thats existing code.

>  				return -EINVAL;

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1561118

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@suse.de>
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@suse.de>
---
 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.

[toc] | [prev] | [next] | [standalone]

#1561864

On Wed, Jan 18, 2017 at 12:31:24AM +0100, Borislav Petkov wrote:
> Ah, good catch. I guess I'll have to do here:
> 
>                         /* Something corrupted the container, invalidate it. */
>                         eq_id = 0;
>                         break;
> 
> too.

Ok, I should've done the below from the beginning. I should be looking
at desc->mc which says whether we have found a patch or not. And then
prepare retvals accordingly.

The equivalence id (eq_id) is then used to signal that fact.

And in a future patch, I'll check desc->mc after that function returns
and not eq_id. (I had to do eq_id because of the global this_equiv_id
but that's gone now too).

        /*
         * 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;

                hdr = (u32 *)buf;

                if (hdr[0] != UCODE_UCODE_TYPE)
                        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;
                if (eq_id == mc->hdr.processor_rev_id) {
                        desc->psize = patch_size;
                        desc->mc = mc;
                }

                buf  += patch_size;
                size -= patch_size;
        }

        /*
         * If we have found a patch (desc->mc), it means we're looking at the
         * container which has a patch for this CPU so return 0 to mean, @ucode
         * already points to the proper container. Otherwise, we return the size
         * we scanned so that we can advance to the next container in the
         * buffer.
         */
        if (desc->mc) {
                desc->eq_id = eq_id;
                desc->data  = ucode;
                desc->size  = orig_size - size;

                return 0;
        }

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1561874

On Wed, Jan 18, 2017 at 03:44:46PM +0100, Borislav Petkov wrote:
> And in a future patch, I'll check desc->mc after that function returns
> and not eq_id. (I had to do eq_id because of the global this_equiv_id
> but that's gone now too).

... which is pretty nice, I can get rid of eq_id now as it is private to
the container scanning code now and the callers shouldn't care. Cool.

Now on to test whether that actually makes sense :-)

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 18 Jan 2017 15:55:26 +0100
Subject: [PATCH] x86/microcode/AMD: Remove struct cont_desc.eq_id

The equivalence ID was needed outside of the container scanning logic
but now, after this has been cleaned up, not anymore. Now, cont_desc.mc
is used to denote whether the container we're looking at has the proper
microcode patch for this CPU or not.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 5e1b57747c2f..7889ae492af0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -49,7 +49,6 @@ struct cont_desc {
 	struct microcode_amd *mc;
 	u32		     cpuid_1_eax;
 	u32		     psize;
-	u16		     eq_id;
 	u8		     *data;
 	size_t		     size;
 };
@@ -92,10 +91,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 	/* 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;
+	    hdr[2] == 0)
 		return CONTAINER_HDR_SZ;
-	}
 
 	buf = ucode;
 
@@ -147,9 +144,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 	 * buffer.
 	 */
 	if (desc->mc) {
-		desc->eq_id = eq_id;
-		desc->data  = ucode;
-		desc->size  = orig_size - size;
+		desc->data = ucode;
+		desc->size = orig_size - size;
 
 		return 0;
 	}
@@ -220,8 +216,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p
 	desc.cpuid_1_eax = cpuid_1_eax;
 
 	scan_containers(ucode, size, &desc);
-	if (!desc.eq_id)
-		return ret;
 
 	mc = desc.mc;
 	if (!mc)
@@ -341,7 +335,7 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 	desc.cpuid_1_eax = cpuid_1_eax;
 
 	scan_containers(cp.data, cp.size, &desc);
-	if (!desc.eq_id)
+	if (!desc.mc)
 		return -EINVAL;
 
 	ret = load_microcode_amd(smp_processor_id(), x86_family(cpuid_1_eax),
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web