[PATCH v6 0/8] x86/microcode/AMD: Check microcode file sanity before loading it

[PATCH v6 0/8] x86/microcode/AMD: Check microcode file sanity before loading it

[Maciej S. Szmigiero]

Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode container file since it does very little
consistency checking on data loaded from such file.

This series introduces various checks, mostly on length-type fields,
so all corrupted microcode container files are (hopefully) correctly
rejected instead.
This largely matches what the Intel microcode update driver already does.

It also tries to make the behavior consistent between the early and late
loaders.

Please note that this isn't about verifying the actual microcode update,
that is, the blob that gets sent to the CPU as the new microcode.
Such verification is (hopefully) done by the CPU itself.

It is about verifying a driver-specific container file that includes
many microcode updates for different CPUs of a particular CPU family,
along with metadata that helps the driver select the right microcode
update to actually send to the CPU.

There are purposely-corrupted test files available at
https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.

Changes from v1:
* Capitalize a comment,

* Rename 'eqsize' and 'bufsize' to 'eq_size' and 'buf_size',
respectively,

* Attach a comment about checking the equivalence table header to its
first size check,

* Rename 'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.

Changes from v2:
* Split the patch into separate commits,

* Remove explicit CPU equivalence table size limit,

* Make install_equiv_cpu_table() return a size_t instead of a (signed)
int so no overflow can occur there,

* Automatically compute the PATCH_MAX_SIZE macro and use it for checking
a patch size,

* Make the late loader behavior with respect to late parse failures
consistent with what the early loader does.

Changes from v3:
* Capitalize patch subject names,

* Add a comment describing why we subtract SECTION_HDR_SIZE from a file
leftover length before calling verify_patch_size(),

* Don't break a long line containing the above subtraction,

* Move a comment in parse_container() back to where it was in the original
patch version,

* Add special local vars for container header fields in parse_container(),

* Return the remaining blob size from parse_container() if the equivalence
table is truncated,

* Split the equivalence table size checks into two patches: one for the
early loader and one for the late loader,

* Rename an equivalence table length variable in install_equiv_cpu_table()
for consistency with a similar one in parse_container(),

* Rephrase the error messages in install_equiv_cpu_table() to be more
descriptive and merge two tests there so they print the same message,

* Change install_equiv_cpu_table() to return an unsigned int while moving
the job of adding the container header length to this value to its caller,

* Drop automatic computation of the PATCH_MAX_SIZE macro and add a comment
reminding to do it manually instead,

* Add a local variable patch_type for better code readability in
verify_and_add_patch() function.

Changes from v4:
* Update the first patch in series with Borislav's version,

* Use u32-type variables for microcode container header fields,

* Drop the check for zero-length CPU equivalence table,

* Leave size variables in verify_patch_size() as unsigned ints,

* Rewrite the series to use common microcode container data checking
functions in both the early and the late loader so their code won't get
interspersed with a lot of checks and so will be more readable.

Changes from v5:
* Remove "This commit" or "This patch" prefix from commit messages,

* Make sure that every checking function verifies presence of any data
it accesses so these functions don't have an implicit call order
requirement,

* Integrate verify_patch_size() into verify_patch() after the late loader
is converted to no longer call verify_patch_size() directly,

* Make a patch matching check in the early loader more readable,

* Skip just one byte in the early loader when a container cannot be parsed
successfully so the parser will correctly skip unknown data of any size,

* Move assignment of a pointer to CPU equivalence table header variable
past this table check in install_equiv_cpu_table(),

* Split returned status value from returned length of data to skip in
verify_and_add_patch() so we keep the common convention that a function
zero return value means success while a negative value means an error,

* Don't introduce a new global variable for holding the CPU equivalence
table size, add a terminating zero entry to this table explicitly instead
to prevent scanning past its valid data.


 arch/x86/include/asm/microcode_amd.h |   1 +
 arch/x86/kernel/cpu/microcode/amd.c  | 403 +++++++++++++++++++--------
 2 files changed, 285 insertions(+), 119 deletions(-)

[PATCH v6 1/8] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[Maciej S. Szmigiero]

verify_patch_size() verifies whether the remaining size of the microcode
container file is large enough to contain a patch of the indicated size.

However, the section header length is not included in this indicated
size but it is present in the leftover file length so it should be
subtracted from the leftover file length before passing this value to
verify_patch_size().

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Link: http://lkml.kernel.org/r/b4854b17-e3ba-54d6-488d-0e0bfffe4c71@maciej.szmigiero.name
[ Split comment. ]
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 0624957aa068..dc8ea9a9d962 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -461,8 +461,12 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	return 0;
 }
 
-static unsigned int verify_patch_size(u8 family, u32 patch_size,
-				      unsigned int size)
+/*
+ * Check whether the passed remaining file @size is large enough to contain a
+ * patch of the indicated @patch_size (and also whether this size does not
+ * exceed the per-family maximum).
+ */
+static unsigned int verify_patch_size(u8 family, u32 patch_size, unsigned int size)
 {
 	u32 max_size;
 
@@ -613,7 +617,12 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
 		return crnt_size;
 	}
 
-	ret = verify_patch_size(family, patch_size, leftover);
+	/*
+	 * 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 before passing this value to the function below.
+	 */
+	ret = verify_patch_size(family, patch_size, leftover - SECTION_HDR_SIZE);
 	if (!ret) {
 		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
 		return crnt_size;

[PATCH v6 2/8] x86/microcode/AMD: Add microcode container data checking functions

[Maciej S. Szmigiero]

Add verify_container(), verify_equivalence_table(), verify_patch_section()
and verify_patch() functions to the AMD microcode update driver.

These functions check whether a passed buffer contains the relevant
structure, whether it isn't truncated and (for actual microcode patches)
whether the size of a patch is not too large for a particular CPU family.
By adding these checks as separate functions the actual microcode loading
code won't get interspersed with a lot of checks and so will be more
readable.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/x86/kernel/cpu/microcode/amd.c | 148 +++++++++++++++++++++++++++-
 1 file changed, 145 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index dc8ea9a9d962..f9485ff7183c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -73,6 +73,150 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 	return 0;
 }
 
+/*
+ * Checks whether there is a valid microcode container file at the beginning
+ * of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_container(const u8 *buf, size_t buf_size, bool early)
+{
+	u32 cont_magic;
+
+	if (buf_size <= CONTAINER_HDR_SZ) {
+		if (!early)
+			pr_err("Truncated microcode container header.\n");
+
+		return false;
+	}
+
+	cont_magic = *(const u32 *)buf;
+	if (cont_magic != UCODE_MAGIC) {
+		if (!early)
+			pr_err("Invalid magic value (0x%08x).\n", cont_magic);
+
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * Checks whether there is a valid, non-truncated CPU equivalence table
+ * at the beginning of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_equivalence_table(const u8 *buf, size_t buf_size, bool early)
+{
+	const u32 *hdr = (const u32 *)buf;
+	u32 cont_type, equiv_tbl_len;
+
+	if (!verify_container(buf, buf_size, early))
+		return false;
+
+	cont_type = hdr[1];
+	if (cont_type != UCODE_EQUIV_CPU_TABLE_TYPE) {
+		if (!early)
+			pr_err("Wrong microcode container equivalence table type: %u.\n",
+			       cont_type);
+
+		return false;
+	}
+
+	equiv_tbl_len = hdr[2];
+	if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
+	    buf_size - CONTAINER_HDR_SZ < equiv_tbl_len) {
+		if (!early)
+			pr_err("Truncated equivalence table.\n");
+
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * Checks whether there is a valid, non-truncated microcode patch section
+ * at the beginning of a passed buffer @buf of size @size.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_patch_section(const u8 *buf, size_t buf_size, bool early)
+{
+	const u32 *hdr = (const u32 *)buf;
+	u32 patch_type, patch_size;
+
+	if (buf_size < SECTION_HDR_SIZE) {
+		if (!early)
+			pr_err("Truncated patch section.\n");
+
+		return false;
+	}
+
+	patch_type = hdr[0];
+	patch_size = hdr[1];
+
+	if (patch_type != UCODE_UCODE_TYPE) {
+		if (!early)
+			pr_err("Invalid type field (%u) in container file section header.\n",
+				patch_type);
+
+		return false;
+	}
+
+	if (patch_size < sizeof(struct microcode_header_amd)) {
+		if (!early)
+			pr_err("Patch of size %u too short.\n", patch_size);
+
+		return false;
+	}
+
+	if (buf_size - SECTION_HDR_SIZE < patch_size) {
+		if (!early)
+			pr_err("Patch of size %u truncated.\n", patch_size);
+
+		return false;
+	}
+
+	return true;
+}
+
+static unsigned int verify_patch_size(u8 family, u32 patch_size,
+				      unsigned int size);
+
+/*
+ * Checks whether a microcode patch located at the beginning of a passed
+ * buffer @buf of size @size is not too large for a particular @family
+ * and is not truncated.
+ * If @early is set this function does not print errors which makes it
+ * usable by the early microcode loader.
+ */
+static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
+{
+	const u32 *hdr = (const u32 *)buf;
+	u32 patch_size;
+
+	if (!verify_patch_section(buf, buf_size, early))
+		return false;
+
+	patch_size = hdr[1];
+
+	/*
+	 * 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 before passing this value to the function below.
+	 */
+	if (!verify_patch_size(family, patch_size, buf_size - SECTION_HDR_SIZE)) {
+		if (!early)
+			pr_err("Patch of size %u too large.\n", patch_size);
+
+		return false;
+	}
+
+	return true;
+}
+
 /*
  * This scans the ucode blob for the proper container as we can have multiple
  * containers glued together. Returns the equivalence ID from the equivalence
@@ -494,10 +638,8 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size, unsigned int si
 		break;
 	}
 
-	if (patch_size > min_t(u32, size, max_size)) {
-		pr_err("patch size mismatch\n");
+	if (patch_size > min_t(u32, size, max_size))
 		return 0;
-	}
 
 	return patch_size;
 }

[PATCH v6 3/8] x86/microcode/AMD: Check microcode container data in the early loader

[Maciej S. Szmigiero]

Convert the early loader in the AMD microcode update driver to use the
container data checking functions introduced by the previous commit.

We have to be careful to call these functions with 'early' parameter set,
so they won't try to print errors as the early loader runs too early for
printk()-style functions to work.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/x86/kernel/cpu/microcode/amd.c | 59 ++++++++++++++++-------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index f9485ff7183c..f4c7479a961c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -224,29 +224,36 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
  * 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 ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
+static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
 {
 	struct equiv_cpu_entry *eq;
-	ssize_t orig_size = size;
+	size_t orig_size = size;
 	u32 *hdr = (u32 *)ucode;
+	u32 equiv_tbl_len;
 	u16 eq_id;
 	u8 *buf;
 
-	/* Am I looking at an equivalence table header? */
-	if (hdr[0] != UCODE_MAGIC ||
-	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
-	    hdr[2] == 0)
-		return CONTAINER_HDR_SZ;
+	/*
+	 * Skip one byte when a container cannot be parsed successfully
+	 * so the parser will correctly skip unknown data of any size until
+	 * it hopefully arrives at something that it is able to recognize.
+	 */
+	if (!verify_container(ucode, size, true) ||
+	    !verify_equivalence_table(ucode, size, true))
+		return 1;
 
 	buf = ucode;
 
+	equiv_tbl_len = hdr[2];
 	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
 
 	/* Find the equivalence ID of our CPU in this table: */
 	eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
 
-	buf  += hdr[2] + CONTAINER_HDR_SZ;
-	size -= hdr[2] + CONTAINER_HDR_SZ;
+	buf  += CONTAINER_HDR_SZ;
+	buf  += equiv_tbl_len;
+	size -= CONTAINER_HDR_SZ;
+	size -= equiv_tbl_len;
 
 	/*
 	 * Scan through the rest of the container to find where it ends. We do
@@ -258,25 +265,27 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 
 		hdr = (u32 *)buf;
 
-		if (hdr[0] != UCODE_UCODE_TYPE)
+		if (!verify_patch_section(buf, size, true))
 			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 + SECTION_HDR_SIZE);
+		if (eq_id != mc->hdr.processor_rev_id)
+			goto next_patch;
 
-		mc = (struct microcode_amd *)buf;
-		if (eq_id == mc->hdr.processor_rev_id) {
-			desc->psize = patch_size;
-			desc->mc = mc;
-		}
+		if (!verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
+				  true))
+			goto next_patch;
+
+		/* We have found a matching patch! */
+		desc->psize = patch_size;
+		desc->mc = mc;
 
+next_patch:
+		buf  += SECTION_HDR_SIZE;
 		buf  += patch_size;
+		size -= SECTION_HDR_SIZE;
 		size -= patch_size;
 	}
 
@@ -303,15 +312,13 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
  */
 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);
+	while (size > 0) {
+		size_t s = parse_container(ucode, size, desc);
 		if (!s)
 			return;
 
 		ucode += s;
-		rem   -= s;
+		size  -= s;
 	}
 }
 

[PATCH v6 4/8] x86/microcode/AMD: Split status from data to skip in verify_and_add_patch()

[Maciej S. Szmigiero]

verify_and_add_patch() returned a single "int" value which encoded both
this function error status and also a length of microcode container data to
skip.

Unfortunately, ranges of these two values collide: the length of data to
skip can be any value between 1 and UINT_MAX, so, for example, error status
of -EINVAL maps to a valid return value of 4294967274 bytes.
That's why these two values need to be split.

Let's keep the common convention that a function zero return value means
success while a negative value means an error while moving the returned
length of microcode container data to skip to a separate output parameter.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/x86/kernel/cpu/microcode/amd.c | 33 +++++++++++++++--------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index f4c7479a961c..f8bd74341ed8 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -730,40 +730,41 @@ static void cleanup(void)
 }
 
 /*
- * We return the current size even if some of the checks failed so that
- * we can skip over the next patch. If we return a negative value, we
- * signal a grave error like a memory allocation has failed and the
- * driver cannot continue functioning normally. In such cases, we tear
- * down everything we've used up so far and exit.
+ * We return zero (success) and the current patch data size in @crnt_size
+ * even if some of the checks failed so that we can skip over the next patch.
+ * If we return a negative value, we signal a grave error like a memory
+ * allocation has failed and the driver cannot continue functioning normally.
+ * In such cases, we tear down everything we've used up so far and exit.
  */
-static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
+static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
+				unsigned int *crnt_size)
 {
 	struct microcode_header_amd *mc_hdr;
 	struct ucode_patch *patch;
-	unsigned int patch_size, crnt_size, ret;
+	unsigned int patch_size, ret;
 	u32 proc_fam;
 	u16 proc_id;
 
 	patch_size  = *(u32 *)(fw + 4);
-	crnt_size   = patch_size + SECTION_HDR_SIZE;
+	*crnt_size  = patch_size + SECTION_HDR_SIZE;
 	mc_hdr	    = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
 	proc_id	    = mc_hdr->processor_rev_id;
 
 	proc_fam = find_cpu_family_by_equiv_cpu(proc_id);
 	if (!proc_fam) {
 		pr_err("No patch family for equiv ID: 0x%04x\n", proc_id);
-		return crnt_size;
+		return 0;
 	}
 
 	/* check if patch is for the current family */
 	proc_fam = ((proc_fam >> 8) & 0xf) + ((proc_fam >> 20) & 0xff);
 	if (proc_fam != family)
-		return crnt_size;
+		return 0;
 
 	if (mc_hdr->nb_dev_id || mc_hdr->sb_dev_id) {
 		pr_err("Patch-ID 0x%08x: chipset-specific code unsupported.\n",
 			mc_hdr->patch_id);
-		return crnt_size;
+		return 0;
 	}
 
 	/*
@@ -774,7 +775,7 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
 	ret = verify_patch_size(family, patch_size, leftover - SECTION_HDR_SIZE);
 	if (!ret) {
 		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
-		return crnt_size;
+		return 0;
 	}
 
 	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
@@ -800,7 +801,7 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
 	/* ... and add to cache. */
 	update_cache(patch);
 
-	return crnt_size;
+	return 0;
 }
 
 static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
@@ -809,7 +810,6 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 	enum ucode_state ret = UCODE_ERROR;
 	unsigned int leftover;
 	u8 *fw = (u8 *)data;
-	int crnt_size = 0;
 	int offset;
 
 	offset = install_equiv_cpu_table(data);
@@ -827,8 +827,9 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 	}
 
 	while (leftover) {
-		crnt_size = verify_and_add_patch(family, fw, leftover);
-		if (crnt_size < 0)
+		unsigned int crnt_size;
+
+		if (verify_and_add_patch(family, fw, leftover, &crnt_size) < 0)
 			return ret;
 
 		fw	 += crnt_size;

[PATCH v6 5/8] x86/microcode/AMD: Check microcode container data in the late loader

[Maciej S. Szmigiero]

Convert the late loader in the AMD microcode update driver to use newly
introduced microcode container data checking functions as it was previously
done for the early loader.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/x86/kernel/cpu/microcode/amd.c | 70 +++++++++++++----------------
 1 file changed, 32 insertions(+), 38 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index f8bd74341ed8..3e10a5920f58 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -693,28 +693,26 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	return UCODE_UPDATED;
 }
 
-static int install_equiv_cpu_table(const u8 *buf)
+static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 {
-	unsigned int *ibuf = (unsigned int *)buf;
-	unsigned int type = ibuf[1];
-	unsigned int size = ibuf[2];
+	const u32 *hdr;
+	u32 equiv_tbl_len;
 
-	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
-		pr_err("empty section/"
-		       "invalid type field in container file section header\n");
-		return -EINVAL;
-	}
+	if (!verify_equivalence_table(buf, buf_size, false))
+		return 0;
+
+	hdr = (const u32 *)buf;
+	equiv_tbl_len = hdr[2];
 
-	equiv_cpu_table = vmalloc(size);
+	equiv_cpu_table = vmalloc(equiv_tbl_len);
 	if (!equiv_cpu_table) {
 		pr_err("failed to allocate equivalent CPU table\n");
-		return -ENOMEM;
+		return 0;
 	}
 
-	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
+	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
 
-	/* add header length */
-	return size + CONTAINER_HDR_SZ;
+	return equiv_tbl_len;
 }
 
 static void free_equiv_cpu_table(void)
@@ -739,13 +737,19 @@ static void cleanup(void)
 static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
 				unsigned int *crnt_size)
 {
+	u32 *hdr = (u32 *)fw;
 	struct microcode_header_amd *mc_hdr;
 	struct ucode_patch *patch;
-	unsigned int patch_size, ret;
+	u32 patch_size;
 	u32 proc_fam;
 	u16 proc_id;
 
-	patch_size  = *(u32 *)(fw + 4);
+	if (!verify_patch_section(fw, leftover, false)) {
+		*crnt_size = leftover;
+		return 0;
+	}
+
+	patch_size  = hdr[1];
 	*crnt_size  = patch_size + SECTION_HDR_SIZE;
 	mc_hdr	    = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
 	proc_id	    = mc_hdr->processor_rev_id;
@@ -767,16 +771,8 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
 		return 0;
 	}
 
-	/*
-	 * 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 before passing this value to the function below.
-	 */
-	ret = verify_patch_size(family, patch_size, leftover - SECTION_HDR_SIZE);
-	if (!ret) {
-		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
+	if (!verify_patch(family, fw, leftover, false))
 		return 0;
-	}
 
 	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
 	if (!patch) {
@@ -810,21 +806,21 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 	enum ucode_state ret = UCODE_ERROR;
 	unsigned int leftover;
 	u8 *fw = (u8 *)data;
-	int offset;
+	unsigned int offset;
 
-	offset = install_equiv_cpu_table(data);
-	if (offset < 0) {
+	offset = install_equiv_cpu_table(data, size);
+	if (!offset) {
 		pr_err("failed to create equivalent cpu table\n");
 		return ret;
 	}
-	fw += offset;
-	leftover = size - offset;
 
-	if (*(u32 *)fw != UCODE_UCODE_TYPE) {
-		pr_err("invalid type field in container file section header\n");
-		free_equiv_cpu_table();
-		return ret;
-	}
+	/*
+	 * Skip also the container header, since install_equiv_cpu_table()
+	 * returns just the raw equivalence table size without the header.
+	 */
+	fw += CONTAINER_HDR_SZ;
+	fw += offset;
+	leftover = size - CONTAINER_HDR_SZ - offset;
 
 	while (leftover) {
 		unsigned int crnt_size;
@@ -912,10 +908,8 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	}
 
 	ret = UCODE_ERROR;
-	if (*(u32 *)fw->data != UCODE_MAGIC) {
-		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
+	if (!verify_container(fw->data, fw->size, false))
 		goto fw_release;
-	}
 
 	ret = load_microcode_amd(bsp, c->x86, fw->data, fw->size);
 

[PATCH v6 6/8] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()

[Maciej S. Szmigiero]

Now that the previous commit converted the late loader to do a full patch
checking via verify_patch() instead of calling verify_patch_size() directly
we can integrate the later function into the former because it was its only
user.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/x86/kernel/cpu/microcode/amd.c | 72 +++++++++++------------------
 1 file changed, 27 insertions(+), 45 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 3e10a5920f58..764dac82c03f 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -182,9 +182,6 @@ static bool verify_patch_section(const u8 *buf, size_t buf_size, bool early)
 	return true;
 }
 
-static unsigned int verify_patch_size(u8 family, u32 patch_size,
-				      unsigned int size);
-
 /*
  * Checks whether a microcode patch located at the beginning of a passed
  * buffer @buf of size @size is not too large for a particular @family
@@ -195,19 +192,43 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
 static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
 {
 	const u32 *hdr = (const u32 *)buf;
-	u32 patch_size;
+	u32 patch_size, max_size;
 
 	if (!verify_patch_section(buf, buf_size, early))
 		return false;
 
 	patch_size = hdr[1];
 
+#define F1XH_MPB_MAX_SIZE 2048
+#define F14H_MPB_MAX_SIZE 1824
+#define F15H_MPB_MAX_SIZE 4096
+#define F16H_MPB_MAX_SIZE 3458
+#define F17H_MPB_MAX_SIZE 3200
+
+	switch (family) {
+	case 0x14:
+		max_size = F14H_MPB_MAX_SIZE;
+		break;
+	case 0x15:
+		max_size = F15H_MPB_MAX_SIZE;
+		break;
+	case 0x16:
+		max_size = F16H_MPB_MAX_SIZE;
+		break;
+	case 0x17:
+		max_size = F17H_MPB_MAX_SIZE;
+		break;
+	default:
+		max_size = F1XH_MPB_MAX_SIZE;
+		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 before passing this value to the function below.
+	 * it from the leftover file length.
 	 */
-	if (!verify_patch_size(family, patch_size, buf_size - SECTION_HDR_SIZE)) {
+	if (patch_size > min_t(u32, max_size, buf_size - SECTION_HDR_SIZE)) {
 		if (!early)
 			pr_err("Patch of size %u too large.\n", patch_size);
 
@@ -612,45 +633,6 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	return 0;
 }
 
-/*
- * Check whether the passed remaining file @size is large enough to contain a
- * patch of the indicated @patch_size (and also whether this size does not
- * exceed the per-family maximum).
- */
-static unsigned int verify_patch_size(u8 family, u32 patch_size, unsigned int size)
-{
-	u32 max_size;
-
-#define F1XH_MPB_MAX_SIZE 2048
-#define F14H_MPB_MAX_SIZE 1824
-#define F15H_MPB_MAX_SIZE 4096
-#define F16H_MPB_MAX_SIZE 3458
-#define F17H_MPB_MAX_SIZE 3200
-
-	switch (family) {
-	case 0x14:
-		max_size = F14H_MPB_MAX_SIZE;
-		break;
-	case 0x15:
-		max_size = F15H_MPB_MAX_SIZE;
-		break;
-	case 0x16:
-		max_size = F16H_MPB_MAX_SIZE;
-		break;
-	case 0x17:
-		max_size = F17H_MPB_MAX_SIZE;
-		break;
-	default:
-		max_size = F1XH_MPB_MAX_SIZE;
-		break;
-	}
-
-	if (patch_size > min_t(u32, size, max_size))
-		return 0;
-
-	return patch_size;
-}
-
 static enum ucode_state apply_microcode_amd(int cpu)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);

[PATCH v6 7/8] x86/microcode/AMD: Add a reminder about PATCH_MAX_SIZE macro

[Maciej S. Szmigiero]

The PATCH_MAX_SIZE macro should contain the maximum of all family patch
sizes.
Since these sizes are defined in an other place than this macro, let's add
a reminder to them so people will remember to verify PATCH_MAX_SIZE
correctness when modifying a family patch size or adding a new family.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/x86/include/asm/microcode_amd.h | 1 +
 arch/x86/kernel/cpu/microcode/amd.c  | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/arch/x86/include/asm/microcode_amd.h b/arch/x86/include/asm/microcode_amd.h
index 209492849566..8ea477fbc65f 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -41,6 +41,7 @@ struct microcode_amd {
 	unsigned int			mpb[0];
 };
 
+/* Maximum patch size of all supported families */
 #define PATCH_MAX_SIZE PAGE_SIZE
 
 #ifdef CONFIG_MICROCODE_AMD
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 764dac82c03f..cc38182c76d2 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -199,6 +199,10 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
 
 	patch_size = hdr[1];
 
+/*
+ * If you modify these values or add a new one also check whether
+ * PATCH_MAX_SIZE in include/asm/microcode_amd.h needs updating, too.
+ */
 #define F1XH_MPB_MAX_SIZE 2048
 #define F14H_MPB_MAX_SIZE 1824
 #define F15H_MPB_MAX_SIZE 4096

[PATCH v6 8/8] x86/microcode/AMD: Don't scan past the CPU equivalence table data

[Maciej S. Szmigiero]

Currently, the code scanning a CPU equivalence table read from a microcode
container file assumes that it actually contains a terminating zero entry,
but if does not then the code will continue the scan past its valid data.

For the late loader this can be improved by always appending a terminating
zero entry to such table when loading it.
This way we don't need an extra global variable for holding the table size
and we don't have to reject such incomplete tables (for backward
compatibility with the existing code which didn't do so).

For the early loader, since we can't allocate memory and have to work
in-place, let's pass an explicit size of this table to its scanning
functions so they will know when to stop.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/x86/kernel/cpu/microcode/amd.c | 46 ++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index cc38182c76d2..f443aede50d5 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -63,12 +63,18 @@ static u8 amd_ucode_patch[PATCH_MAX_SIZE];
 static const char
 ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
 
-static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
+static u16 find_equiv_id(const struct equiv_cpu_entry *equiv_table,
+			 unsigned int equiv_table_entries, u32 sig)
 {
-	for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
-		if (sig == equiv_table->installed_cpu)
-			return equiv_table->equiv_cpu;
-	}
+	unsigned int i;
+
+	if (!equiv_table)
+		return 0;
+
+	for (i = 0; i < equiv_table_entries && equiv_table[i].installed_cpu;
+	     i++)
+		if (sig == equiv_table[i].installed_cpu)
+			return equiv_table[i].equiv_cpu;
 
 	return 0;
 }
@@ -273,7 +279,8 @@ static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
 	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
 
 	/* Find the equivalence ID of our CPU in this table: */
-	eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
+	eq_id = find_equiv_id(eq, equiv_tbl_len / sizeof(*eq),
+			      desc->cpuid_1_eax);
 
 	buf  += CONTAINER_HDR_SZ;
 	buf  += equiv_tbl_len;
@@ -540,12 +547,14 @@ void reload_ucode_amd(void)
 static u16 __find_equiv_id(unsigned int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	return find_equiv_id(equiv_cpu_table, uci->cpu_sig.sig);
+
+	/* install_equiv_cpu_table() guarantees us a terminating entry */
+	return find_equiv_id(equiv_cpu_table, UINT_MAX, uci->cpu_sig.sig);
 }
 
 static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
 {
-	int i = 0;
+	unsigned int i = 0;
 
 	BUG_ON(!equiv_cpu_table);
 
@@ -683,20 +692,37 @@ static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 {
 	const u32 *hdr;
 	u32 equiv_tbl_len;
+	unsigned int equiv_tbl_entries;
+	const unsigned int equiv_tbl_entries_max =
+		UINT_MAX / sizeof(struct equiv_cpu_entry);
 
 	if (!verify_equivalence_table(buf, buf_size, false))
 		return 0;
 
 	hdr = (const u32 *)buf;
 	equiv_tbl_len = hdr[2];
+	equiv_tbl_entries = equiv_tbl_len / sizeof(struct equiv_cpu_entry);
 
-	equiv_cpu_table = vmalloc(equiv_tbl_len);
+	/*
+	 * Make space for a terminating entry. If there is no space left we
+	 * reuse the last entry instead.
+	 */
+	if (equiv_tbl_entries < equiv_tbl_entries_max)
+		equiv_tbl_entries++;
+
+	equiv_cpu_table = vmalloc(equiv_tbl_entries *
+				  sizeof(struct equiv_cpu_entry));
 	if (!equiv_cpu_table) {
 		pr_err("failed to allocate equivalent CPU table\n");
 		return 0;
 	}
 
-	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
+	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ,
+	       (equiv_tbl_entries - 1) * sizeof(struct equiv_cpu_entry));
+
+	/* Set up the terminating entry. */
+	memset(&equiv_cpu_table[equiv_tbl_entries - 1], 0,
+	       sizeof(struct equiv_cpu_entry));
 
 	return equiv_tbl_len;
 }

Re: [PATCH v6 2/8] x86/microcode/AMD: Add microcode container data checking functions

[Borislav Petkov]

On Sun, May 20, 2018 at 12:07:16AM +0200, Maciej S. Szmigiero wrote:
> Add verify_container(), verify_equivalence_table(), verify_patch_section()
> and verify_patch() functions to the AMD microcode update driver.
> 
> These functions check whether a passed buffer contains the relevant
> structure, whether it isn't truncated and (for actual microcode patches)
> whether the size of a patch is not too large for a particular CPU family.
> By adding these checks as separate functions the actual microcode loading
> code won't get interspersed with a lot of checks and so will be more
> readable.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 148 +++++++++++++++++++++++++++-
>  1 file changed, 145 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index dc8ea9a9d962..f9485ff7183c 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -73,6 +73,150 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
>  	return 0;
>  }
>  
> +/*
> + * Checks whether there is a valid microcode container file at the beginning

"Check whether... " imperative tone. Ditto for the rest.

> + * of a passed buffer @buf of size @size.

@buf_size

Also, fix the other comments too.

> + * If @early is set this function does not print errors which makes it
> + * usable by the early microcode loader.
> + */
> +static bool verify_container(const u8 *buf, size_t buf_size, bool early)

...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v6 3/8] x86/microcode/AMD: Check microcode container data in the early loader

[Borislav Petkov]

On Sun, May 20, 2018 at 12:07:17AM +0200, Maciej S. Szmigiero wrote:
> Convert the early loader in the AMD microcode update driver to use the
> container data checking functions introduced by the previous commit.
> 
> We have to be careful to call these functions with 'early' parameter set,
> so they won't try to print errors as the early loader runs too early for
> printk()-style functions to work.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 59 ++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index f9485ff7183c..f4c7479a961c 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -224,29 +224,36 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size, bool early)
>   * 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 ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
> +static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
>  {
>  	struct equiv_cpu_entry *eq;
> -	ssize_t orig_size = size;
> +	size_t orig_size = size;
>  	u32 *hdr = (u32 *)ucode;
> +	u32 equiv_tbl_len;
>  	u16 eq_id;
>  	u8 *buf;
>  
> -	/* Am I looking at an equivalence table header? */
> -	if (hdr[0] != UCODE_MAGIC ||
> -	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
> -	    hdr[2] == 0)
> -		return CONTAINER_HDR_SZ;
> +	/*
> +	 * Skip one byte when a container cannot be parsed successfully
> +	 * so the parser will correctly skip unknown data of any size until
> +	 * it hopefully arrives at something that it is able to recognize.
> +	 */
> +	if (!verify_container(ucode, size, true) ||
> +	    !verify_equivalence_table(ucode, size, true))

That function already calls verify_container().

> +		return 1;
>  
>  	buf = ucode;
>  
> +	equiv_tbl_len = hdr[2];
>  	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
>  
>  	/* Find the equivalence ID of our CPU in this table: */
>  	eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
>  
> -	buf  += hdr[2] + CONTAINER_HDR_SZ;
> -	size -= hdr[2] + CONTAINER_HDR_SZ;
> +	buf  += CONTAINER_HDR_SZ;
> +	buf  += equiv_tbl_len;
> +	size -= CONTAINER_HDR_SZ;
> +	size -= equiv_tbl_len;
>  
>  	/*
>  	 * Scan through the rest of the container to find where it ends. We do
> @@ -258,25 +265,27 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
>  
>  		hdr = (u32 *)buf;
>  
> -		if (hdr[0] != UCODE_UCODE_TYPE)
> +		if (!verify_patch_section(buf, size, true))
>  			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 + SECTION_HDR_SIZE);
> +		if (eq_id != mc->hdr.processor_rev_id)
> +			goto next_patch;
>  
> -		mc = (struct microcode_amd *)buf;
> -		if (eq_id == mc->hdr.processor_rev_id) {
> -			desc->psize = patch_size;
> -			desc->mc = mc;
> -		}
> +		if (!verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
> +				  true))

Let it stick out.

Ok, so above you do verify_patch_section() and then you take patch_size
without fully verifying it - it can be something non-sensically huge and
thus we might skip over good patches.

What you should do instead is call verify_patch() directly - which
already calls verify_patch_section() and if the patch size exceeds the
per-family maximum, return *that* instead and skip only the per family
maximum inside the buffer so that any patches following can get a chance
to get inspected.

For that you'll have to reshuffle the change of integrating
verify_patch_size() to happen before that change here.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v6 5/8] x86/microcode/AMD: Check microcode container data in the late loader

[Borislav Petkov]

On Sun, May 20, 2018 at 12:07:19AM +0200, Maciej S. Szmigiero wrote:
> Convert the late loader in the AMD microcode update driver to use newly
> introduced microcode container data checking functions as it was previously
> done for the early loader.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 70 +++++++++++++----------------
>  1 file changed, 32 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index f8bd74341ed8..3e10a5920f58 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -693,28 +693,26 @@ static enum ucode_state apply_microcode_amd(int cpu)
>  	return UCODE_UPDATED;
>  }
>  
> -static int install_equiv_cpu_table(const u8 *buf)
> +static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
>  {
> -	unsigned int *ibuf = (unsigned int *)buf;
> -	unsigned int type = ibuf[1];
> -	unsigned int size = ibuf[2];
> +	const u32 *hdr;
> +	u32 equiv_tbl_len;
>  
> -	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
> -		pr_err("empty section/"
> -		       "invalid type field in container file section header\n");
> -		return -EINVAL;
> -	}
> +	if (!verify_equivalence_table(buf, buf_size, false))
> +		return 0;
> +
> +	hdr = (const u32 *)buf;
> +	equiv_tbl_len = hdr[2];
>  
> -	equiv_cpu_table = vmalloc(size);
> +	equiv_cpu_table = vmalloc(equiv_tbl_len);
>  	if (!equiv_cpu_table) {
>  		pr_err("failed to allocate equivalent CPU table\n");
> -		return -ENOMEM;
> +		return 0;
>  	}
>  
> -	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
> +	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
>  
> -	/* add header length */
> -	return size + CONTAINER_HDR_SZ;
> +	return equiv_tbl_len;
>  }
>  
>  static void free_equiv_cpu_table(void)
> @@ -739,13 +737,19 @@ static void cleanup(void)
>  static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
>  				unsigned int *crnt_size)
>  {
> +	u32 *hdr = (u32 *)fw;
>  	struct microcode_header_amd *mc_hdr;
>  	struct ucode_patch *patch;
> -	unsigned int patch_size, ret;
> +	u32 patch_size;
>  	u32 proc_fam;
>  	u16 proc_id;
>  
> -	patch_size  = *(u32 *)(fw + 4);
> +	if (!verify_patch_section(fw, leftover, false)) {
> +		*crnt_size = leftover;

I'm not sure about this: we verify the patch section and in the error
case we skip over the whole leftover buffer?

Maybe skipping over SECTION_HDR_SIZE or better yet skip 1 byte here
too, like in parse_container() to give us the highest chance of finding
something sensible later...

> +		return 0;
> +	}
> +
> +	patch_size  = hdr[1];

Same comment as before: verify_patch_size()

But I think you can simply do verify_patch() at the beginning of the
function and be done with the verification in that function.

>  	*crnt_size  = patch_size + SECTION_HDR_SIZE;
>  	mc_hdr	    = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
>  	proc_id	    = mc_hdr->processor_rev_id;
> @@ -767,16 +771,8 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
>  		return 0;
>  	}
>  
> -	/*
> -	 * 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 before passing this value to the function below.
> -	 */
> -	ret = verify_patch_size(family, patch_size, leftover - SECTION_HDR_SIZE);
> -	if (!ret) {
> -		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
> +	if (!verify_patch(family, fw, leftover, false))
>  		return 0;
> -	}
>  
>  	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
>  	if (!patch) {
> @@ -810,21 +806,21 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
>  	enum ucode_state ret = UCODE_ERROR;
>  	unsigned int leftover;
>  	u8 *fw = (u8 *)data;
> -	int offset;
> +	unsigned int offset;
>  
> -	offset = install_equiv_cpu_table(data);
> -	if (offset < 0) {
> +	offset = install_equiv_cpu_table(data, size);
> +	if (!offset) {
>  		pr_err("failed to create equivalent cpu table\n");

No need for that error message anymore I guess -
install_equiv_cpu_table() and verify_equivalence_table() are pretty
vocal in the error case already.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v6 8/8] x86/microcode/AMD: Don't scan past the CPU equivalence table data

[Borislav Petkov]

On Sun, May 20, 2018 at 12:07:22AM +0200, Maciej S. Szmigiero wrote:
> Currently, the code scanning a CPU equivalence table read from a microcode
> container file assumes that it actually contains a terminating zero entry,
> but if does not then the code will continue the scan past its valid data.
> 
> For the late loader this can be improved by always appending a terminating
> zero entry to such table when loading it.
> This way we don't need an extra global variable for holding the table size
> and we don't have to reject such incomplete tables (for backward
> compatibility with the existing code which didn't do so).
> 
> For the early loader, since we can't allocate memory and have to work
> in-place, let's pass an explicit size of this table to its scanning
> functions so they will know when to stop.

I don't like the difference between early and late here. Just pass
explicit size to the late loader too.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v6 8/8] x86/microcode/AMD: Don't scan past the CPU equivalence table data

[Maciej S. Szmigiero]

On 05.06.2018 10:55, Borislav Petkov wrote:
> On Sun, May 20, 2018 at 12:07:22AM +0200, Maciej S. Szmigiero wrote:
>> Currently, the code scanning a CPU equivalence table read from a microcode
>> container file assumes that it actually contains a terminating zero entry,
>> but if does not then the code will continue the scan past its valid data.
>>
>> For the late loader this can be improved by always appending a terminating
>> zero entry to such table when loading it.
>> This way we don't need an extra global variable for holding the table size
>> and we don't have to reject such incomplete tables (for backward
>> compatibility with the existing code which didn't do so).
>>
>> For the early loader, since we can't allocate memory and have to work
>> in-place, let's pass an explicit size of this table to its scanning
>> functions so they will know when to stop.
> 
> I don't like the difference between early and late here. Just pass
> explicit size to the late loader too.

That was the solution before this patch series version (6) - there was
a variable holding the CPU equivalence table size for the late loader,
but you didn't like it:> Instead of adding yet another global var which needs handling too,
> and touching so many places, just do all checks and preparations in
> install_equiv_cpu_table() so that the rest of the code can get what it
> expects: terminating zero entry and proper size.

And we would need to hold this explicit size somewhere since the
table scanning function in the late loader is on a different call
path than microcode file parsing.

Maciej

Re: [PATCH v6 3/8] x86/microcode/AMD: Check microcode container data in the early loader

[Maciej S. Szmigiero]

On 05.06.2018 10:54, Borislav Petkov wrote:
(..)
>> @@ -258,25 +265,27 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
>>  
>>  		hdr = (u32 *)buf;
>>  
>> -		if (hdr[0] != UCODE_UCODE_TYPE)
>> +		if (!verify_patch_section(buf, size, true))
>>  			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 + SECTION_HDR_SIZE);
>> +		if (eq_id != mc->hdr.processor_rev_id)
>> +			goto next_patch;
>>  
>> -		mc = (struct microcode_amd *)buf;
>> -		if (eq_id == mc->hdr.processor_rev_id) {
>> -			desc->psize = patch_size;
>> -			desc->mc = mc;
>> -		}
>> +		if (!verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
>> +				  true))
> 
> Let it stick out.
> 
> Ok, so above you do verify_patch_section() and then you take patch_size
> without fully verifying it - it can be something non-sensically huge and
> thus we might skip over good patches.
> 
> What you should do instead is call verify_patch() directly - which
> already calls verify_patch_section() and if the patch size exceeds the
> per-family maximum, return *that* instead and skip only the per family
> maximum inside the buffer so that any patches following can get a chance
> to get inspected.

verify_patch_section() does only very basic patch section checks -
more or less whether the section, its header and a patch header exist and
can be accessed at all.

Here, a check can be added whether the indicated patch size does not
exceed PATCH_MAX_SIZE to catch nonsensically huge patch sizes and to
skip only a maximum patch length of that many bytes.

At this point we don't know the CPU family the particular patch is for
since the patch header contains only CPU rev_id, not an explicit family
number.

Only the late loader is able to translate back this CPU rev_id to its
family number via the CPU equivalence table, the early loader simply
skips patches that have CPU rev_id different from the current CPU one -
so it can always use the current CPU family number for a patch
verification.

But either way, in order to read the CPU rev_id in the patch
header we have to verify its presence in the microcode container file.
This is what verify_patch_section() does.

Then, once the particular loader determines the family number for a
patch, verify_patch() does additional per-family size check.
In principle, this function could be renamed verify_patch_family_size()
and have call to verify_patch_section() at its beginning dropped.

Maciej

Re: [PATCH v6 3/8] x86/microcode/AMD: Check microcode container data in the early loader

[Borislav Petkov]

On Thu, Jun 14, 2018 at 10:56:07PM +0200, Maciej S. Szmigiero wrote:
> At this point we don't know the CPU family the particular patch is for
> since the patch header contains only CPU rev_id, not an explicit family
> number.

	patch_fam = 0xf + (mc->processor_rev_id >> 12);

which means, you can do

        if (patch_fam != family)
                return 0;

like verify_and_add_patch() does before calling verify_patch() with the
correct family of the current CPU.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v6 8/8] x86/microcode/AMD: Don't scan past the CPU equivalence table data

[Borislav Petkov]

On Tue, Jun 12, 2018 at 11:08:31PM +0200, Maciej S. Szmigiero wrote:
> That was the solution before this patch series version (6) - there was
> a variable holding the CPU equivalence table size for the late loader,
> but you didn't like it:> Instead of adding yet another global var which needs handling too,
> > and touching so many places, just do all checks and preparations in
> > install_equiv_cpu_table() so that the rest of the code can get what it
> > expects: terminating zero entry and proper size.

So what's wrong with computing the size in the late loader just like you
do in parse_container()?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v6 8/8] x86/microcode/AMD: Don't scan past the CPU equivalence table data

[Maciej S. Szmigiero]

On 18.06.2018 18:44, Borislav Petkov wrote:
> On Tue, Jun 12, 2018 at 11:08:31PM +0200, Maciej S. Szmigiero wrote:
>> That was the solution before this patch series version (6) - there was
>> a variable holding the CPU equivalence table size for the late loader,
>> but you didn't like it:> Instead of adding yet another global var which needs handling too,
>>> and touching so many places, just do all checks and preparations in
>>> install_equiv_cpu_table() so that the rest of the code can get what it
>>> expects: terminating zero entry and proper size.
> 
> So what's wrong with computing the size in the late loader just like you
> do in parse_container()?
> 

The equivalence table size can be computed in the late loader - there is
no problem there.

However, this computed size needs to be passed somehow to functions
scanning the equivalence table.

One of such functions, __find_equiv_id() can be called from
->collect_cpu_info() and ->apply_microcode() microcode_ops callbacks
which do not allow passing any specific state to the microcode update
driver.

This means that the size would need to be stored in a global variable,
just like a pointer to the equivalence table itself is.

Maciej

Re: [PATCH v6 8/8] x86/microcode/AMD: Don't scan past the CPU equivalence table data

[Borislav Petkov]

On Mon, Jun 18, 2018 at 08:11:53PM +0200, Maciej S. Szmigiero wrote:
> The equivalence table size can be computed in the late loader - there is
> no problem there.
> 
> However, this computed size needs to be passed somehow to functions
> scanning the equivalence table.

Ok, then let's make a compromise with a proper global descriptor which
gets passed around:

static struct equiv_cpu_table {
        struct equiv_cpu_entry *table;
        unsigned int size;
} *equiv_table;

But make that conversion in a prepatch pls.

Thx.

-- 
Regards/Gruss,
    Boris.

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