[PATCH v5 0/6] x86/microcode/AMD: Check microcode file sanity before loading it

[PATCH v5 0/6] 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.

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

[PATCH v5 1/6] 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 v5 2/6] x86/microcode/AMD: Add microcode container data checking functions

[Maciej S. Szmigiero]

This commit adds 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 | 140 +++++++++++++++++++++++++++++++++++-
 1 file changed, 137 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index dc8ea9a9d962..4fafaf0852d7 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -73,6 +73,142 @@ 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;
+
+	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 = 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 +630,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 v5 3/6] x86/microcode/AMD: Check microcode container data in the early loader

[Maciej S. Szmigiero]

This commit converts 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 | 45 ++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 4fafaf0852d7..94fcd702a67a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -216,29 +216,33 @@ 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)
+	if (!verify_container(ucode, size, true))
+		return 0;
+
+	if (!verify_equivalence_table(ucode, size, true))
 		return CONTAINER_HDR_SZ;
 
 	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
@@ -250,25 +254,22 @@ 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;
-		if (eq_id == mc->hdr.processor_rev_id) {
+		mc = (struct microcode_amd *)(buf + SECTION_HDR_SIZE);
+		if (eq_id == mc->hdr.processor_rev_id &&
+		    verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
+				 true)) {
 			desc->psize = patch_size;
 			desc->mc = mc;
 		}
 
+		buf  += SECTION_HDR_SIZE;
 		buf  += patch_size;
+		size -= SECTION_HDR_SIZE;
 		size -= patch_size;
 	}
 
@@ -295,15 +296,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 v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Maciej S. Szmigiero]

This commit converts the late loader in the AMD microcode update driver to
use newly introduced microcode container data checking functions as the
previous commit did for the early loader.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 94fcd702a67a..b429d3f554b9 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -677,28 +677,24 @@ 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 = (const u32 *)buf;
+	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;
 
-	equiv_cpu_table = vmalloc(size);
+	equiv_tbl_len = hdr[2];
+	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)
@@ -715,20 +711,26 @@ 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 can skip over the next patch. If we return zero, 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 unsigned int verify_and_add_patch(u8 family, u8 *fw,
+					 unsigned int leftover)
 {
+	u32 *hdr = (u32 *)fw;
 	struct microcode_header_amd *mc_hdr;
 	struct ucode_patch *patch;
-	unsigned int patch_size, crnt_size, ret;
+	u32 patch_size;
+	unsigned int crnt_size;
 	u32 proc_fam;
 	u16 proc_id;
 
-	patch_size  = *(u32 *)(fw + 4);
+	if (!verify_patch_section(fw, leftover, false))
+		return leftover;
+
+	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;
@@ -750,28 +752,20 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
 		return crnt_size;
 	}
 
-	/*
-	 * 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 crnt_size;
-	}
 
 	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
 	if (!patch) {
 		pr_err("Patch allocation failure.\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	patch->data = kmemdup(fw + SECTION_HDR_SIZE, patch_size, GFP_KERNEL);
 	if (!patch->data) {
 		pr_err("Patch data allocation failure.\n");
 		kfree(patch);
-		return -EINVAL;
+		return 0;
 	}
 
 	INIT_LIST_HEAD(&patch->plist);
@@ -793,26 +787,27 @@ 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;
+	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;
+
 		crnt_size = verify_and_add_patch(family, fw, leftover);
-		if (crnt_size < 0)
+		if (!crnt_size)
 			return ret;
 
 		fw	 += crnt_size;
@@ -895,10 +890,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 v5 5/6] 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 b429d3f554b9..2df366cc71ce 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -605,6 +605,10 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size, unsigned int si
 {
 	u32 max_size;
 
+/*
+ * 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 v5 6/6] x86/microcode/AMD: Check the equivalence table size when scanning it

[Maciej S. Szmigiero]

Currently, the code scanning the CPU equivalence table read from a
microcode container file assumes that it actually contains a terminating
zero entry.
Let's check also the size of this table to make sure that we don't read
past it in case it actually doesn't.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 2df366cc71ce..5ba933ce1d51 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -39,6 +39,7 @@
 #include <asm/msr.h>
 
 static struct equiv_cpu_entry *equiv_cpu_table;
+static unsigned int equiv_cpu_table_entries;
 
 /*
  * This points to the current valid container of microcode patches which we will
@@ -63,12 +64,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;
 }
@@ -237,7 +244,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;
@@ -499,20 +507,21 @@ 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);
+	return find_equiv_id(equiv_cpu_table, equiv_cpu_table_entries,
+			     uci->cpu_sig.sig);
 }
 
 static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
 {
-	int i = 0;
+	unsigned int i;
 
 	BUG_ON(!equiv_cpu_table);
 
-	while (equiv_cpu_table[i].equiv_cpu != 0) {
+	for (i = 0; i < equiv_cpu_table_entries &&
+		     equiv_cpu_table[i].equiv_cpu != 0; i++)
 		if (equiv_cpu == equiv_cpu_table[i].equiv_cpu)
 			return equiv_cpu_table[i].installed_cpu;
-		i++;
-	}
+
 	return 0;
 }
 
@@ -697,6 +706,7 @@ static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 	}
 
 	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
+	equiv_cpu_table_entries = equiv_tbl_len / sizeof(struct equiv_cpu_entry);
 
 	return equiv_tbl_len;
 }
@@ -705,6 +715,7 @@ static void free_equiv_cpu_table(void)
 {
 	vfree(equiv_cpu_table);
 	equiv_cpu_table = NULL;
+	equiv_cpu_table_entries = 0;
 }
 
 static void cleanup(void)

Re: [PATCH v5 2/6] x86/microcode/AMD: Add microcode container data checking functions

[Borislav Petkov]

On Mon, Apr 23, 2018 at 11:34:07PM +0200, Maciej S. Szmigiero wrote:
> This commit adds verify_container(), verify_equivalence_table(),

Avoid beginning the commit message of a patch with "This patch" or "This
commit". It is tautologically useless.

> 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 | 140 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 137 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index dc8ea9a9d962..4fafaf0852d7 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -73,6 +73,142 @@ 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;
> +
> +	cont_type = hdr[1];

You need to check the size of buf so that there's enough buf passed in
before you index into it like that.

> +	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];

And that.

> +	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);

No forward declarations pls.

> +
> +/*
> + * 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 = hdr[1];

Just like in the first comment above.

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

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Mon, Apr 23, 2018 at 11:34:08PM +0200, Maciej S. Szmigiero wrote:
> This commit converts the early loader in the AMD microcode update driver to

Avoid beginning the commit message of a patch with "This patch" or "This
commit". It is tautologically useless. Simply get to the point directly:

"Convert the container parsing function ... "

> 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 | 45 ++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 4fafaf0852d7..94fcd702a67a 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -216,29 +216,33 @@ 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)
> +	if (!verify_container(ucode, size, true))
> +		return 0;

 		return CONTAINER_HDR_SZ;

We want to make some forward progress after all.

> +	if (!verify_equivalence_table(ucode, size, true))
>  		return CONTAINER_HDR_SZ;
>  
>  	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
> @@ -250,25 +254,22 @@ 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))

So there's verify_patch_section(), verify_patch() and
verify_patch_size() now. One of the three looks redundant to me. Sounds
to me verify_patch_size() could be expanded into verify_patch() and
former dropped.

>  			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) {
> +		mc = (struct microcode_amd *)(buf + SECTION_HDR_SIZE);
> +		if (eq_id == mc->hdr.processor_rev_id &&
> +		    verify_patch(x86_family(desc->cpuid_1_eax), buf, size,
> +				 true)) {

This needs to be made readable.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Borislav Petkov]

On Mon, Apr 23, 2018 at 11:34:09PM +0200, Maciej S. Szmigiero wrote:
> This commit converts the late loader in the AMD microcode update driver to
> use newly introduced microcode container data checking functions as the
> previous commit did for the early loader.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 87 +++++++++++++++++--------------------
>  1 file changed, 40 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 94fcd702a67a..b429d3f554b9 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -677,28 +677,24 @@ 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 = (const u32 *)buf;

Ok, since we're verifying now, let's do that assignment...

> +	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;

... after the check has passed.

> -	equiv_cpu_table = vmalloc(size);
> +	equiv_tbl_len = hdr[2];

<---- newline here.

> +	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)
> @@ -715,20 +711,26 @@ 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 can skip over the next patch. If we return zero, 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 unsigned int verify_and_add_patch(u8 family, u8 *fw,
> +					 unsigned int leftover)
>  {
> +	u32 *hdr = (u32 *)fw;
>  	struct microcode_header_amd *mc_hdr;
>  	struct ucode_patch *patch;
> -	unsigned int patch_size, crnt_size, ret;
> +	u32 patch_size;
> +	unsigned int crnt_size;
>  	u32 proc_fam;
>  	u16 proc_id;
>  
> -	patch_size  = *(u32 *)(fw + 4);
> +	if (!verify_patch_section(fw, leftover, false))
> +		return leftover;
> +
> +	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;
> @@ -750,28 +752,20 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
>  		return crnt_size;
>  	}
>  
> -	/*
> -	 * 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 crnt_size;
> -	}
>  
>  	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
>  	if (!patch) {
>  		pr_err("Patch allocation failure.\n");
> -		return -EINVAL;
> +		return 0;

So by convention returning 0 is success and negative value means error.
I don't see the reason for changing that in the whole code.

>  	}
>  
>  	patch->data = kmemdup(fw + SECTION_HDR_SIZE, patch_size, GFP_KERNEL);
>  	if (!patch->data) {
>  		pr_err("Patch data allocation failure.\n");
>  		kfree(patch);
> -		return -EINVAL;
> +		return 0;
>  	}
>  
>  	INIT_LIST_HEAD(&patch->plist);
> @@ -793,26 +787,27 @@ 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;
> +	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;
> +
>  		crnt_size = verify_and_add_patch(family, fw, leftover);
> -		if (crnt_size < 0)
> +		if (!crnt_size)

Ditto.

>  			return ret;
>  
>  		fw	 += crnt_size;
> @@ -895,10 +890,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);
>  

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 6/6] x86/microcode/AMD: Check the equivalence table size when scanning it

[Borislav Petkov]

On Mon, Apr 23, 2018 at 11:34:11PM +0200, Maciej S. Szmigiero wrote:
> Currently, the code scanning the CPU equivalence table read from a
> microcode container file assumes that it actually contains a terminating
> zero entry.
> Let's check also the size of this table to make sure that we don't read
> past it in case it actually doesn't.

...

> @@ -697,6 +706,7 @@ static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
>  	}
>  
>  	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
> +	equiv_cpu_table_entries = equiv_tbl_len / sizeof(struct equiv_cpu_entry);
>  
>  	return equiv_tbl_len;

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.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 2/6] x86/microcode/AMD: Add microcode container data checking functions

[Maciej S. Szmigiero]

On 30.04.2018 11:04, Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 11:34:07PM +0200, Maciej S. Szmigiero wrote:
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> +/*
>> + * 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;
>> +
>> +	cont_type = hdr[1];
> 
> You need to check the size of buf so that there's enough buf passed in
> before you index into it like that.

These checking functions are supposed to be called in order:
first verify_container() verifies the basic container, then
verify_equivalence_table() verifies the equivalence table while not
repeating the checks that were already done by the former function.

>> +	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];
> 
> And that.

Same situation here.

>> +
>> +/*
>> + * 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 = hdr[1];
> 
> Just like in the first comment above.
> 

And a similar situation here - verify_patch() does not verify things
that were already checked by verify_container() or
verify_patch_section().

Thanks,
Maciej

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

[Maciej S. Szmigiero]

On 30.04.2018 11:05, Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 11:34:08PM +0200, Maciej S. Szmigiero wrote:
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -216,29 +216,33 @@ 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)
>> +	if (!verify_container(ucode, size, true))
>> +		return 0;
> 
>  		return CONTAINER_HDR_SZ;
> 
> We want to make some forward progress after all.

Even if the container file main magic number is wrong? In this case
parsing is terminated by returning a zero from the above function.

If we want to make the parser more resistant to garbage or
forward-compatible to containers with a different main magic number in
their headers we probably should return a length of a single byte here
instead of 12 (CONTAINER_HDR_SZ) so the parser would correctly skip
unknown data of size which isn't an integer multiple of this number.

Thanks,
Maciej

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Maciej S. Szmigiero]

On 30.04.2018 11:05, Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 11:34:09PM +0200, Maciej S. Szmigiero wrote:
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -750,28 +752,20 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
>>  		return crnt_size;
>>  	}
>>  
>> -	/*
>> -	 * 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 crnt_size;
>> -	}
>>  
>>  	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
>>  	if (!patch) {
>>  		pr_err("Patch allocation failure.\n");
>> -		return -EINVAL;
>> +		return 0;
> 
> So by convention returning 0 is success and negative value means error.
> I don't see the reason for changing that in the whole code.

1) -EINVAL maps to a valid return value of 4294967274 bytes.
We have a different behavior for invalid data in the container file
(including too large lengths) than for grave errors like a failed memory
allocation.

2) This function single caller (__load_microcode_amd()) normalized any
error that verify_and_add_patch() returned to UCODE_ERROR anyway,

3) The existing code uses a convention that zero return value means
'terminate processing' for the parse_container() function in the early
loader which normally returns a 'bytes consumed' value, as this function
does.

Thanks,
Maciej

Re: [PATCH v5 2/6] x86/microcode/AMD: Add microcode container data checking functions

[Borislav Petkov]

On Tue, May 01, 2018 at 12:27:17AM +0200, Maciej S. Szmigiero wrote:
> These checking functions are supposed to be called in order:

We don't do magical rules like that - you either verify fully and
correctly or you don't bother at all. And since you're adamant that this
verification needs to happen, then please do it completely.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Borislav Petkov]

On Tue, May 01, 2018 at 12:27:51AM +0200, Maciej S. Szmigiero wrote:
> 1) -EINVAL maps to a valid return value of 4294967274 bytes.
> We have a different behavior for invalid data in the container file
> (including too large lengths) than for grave errors like a failed memory
> allocation.

WTF?

> 2) This function single caller (__load_microcode_amd()) normalized any
> error that verify_and_add_patch() returned to UCODE_ERROR anyway,

So?

> 3) The existing code uses a convention that zero return value means
> 'terminate processing' for the parse_container() function in the early
> loader which normally returns a 'bytes consumed' value, as this function
> does.

parse_container() could very well change its convention to return
negative on error and positive value if the loop is supposed to skip
bytes.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Tue, May 01, 2018 at 12:27:34AM +0200, Maciej S. Szmigiero wrote:
> If we want to make the parser more resistant to garbage or
> forward-compatible to containers with a different main magic number in
> their headers we probably should return a length of a single byte here
> instead of 12 (CONTAINER_HDR_SZ) so the parser would correctly skip
> unknown data of size which isn't an integer multiple of this number.

Ok.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 2/6] x86/microcode/AMD: Add microcode container data checking functions

[Maciej S. Szmigiero]

On 01.05.2018 10:18, Borislav Petkov wrote:
> On Tue, May 01, 2018 at 12:27:17AM +0200, Maciej S. Szmigiero wrote:
>> These checking functions are supposed to be called in order:
> 
> We don't do magical rules like that - you either verify fully and
> correctly or you don't bother at all. And since you're adamant that this
> verification needs to happen, then please do it completely.
> 

These are internal functions to this driver, declared as "static", so
there is no problem if they have additional requirements with respect
to their call order.

But it is, of course, possible to do these checks also in the later
checking functions as you wish.

Maciej

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Maciej S. Szmigiero]

On 01.05.2018 10:43, Borislav Petkov wrote:
> On Tue, May 01, 2018 at 12:27:51AM +0200, Maciej S. Szmigiero wrote:
>> 1) -EINVAL maps to a valid return value of 4294967274 bytes.
>> We have a different behavior for invalid data in the container file
>> (including too large lengths) than for grave errors like a failed memory
>> allocation.
> 
> WTF?

Could you please elaborate this comment?

-EINVAL cast to unsigned int is 4294967274 and this value is also
a valid count of bytes to skip that this function can return.

The "grave errors" behavior comes from the existing code, a comment
in code above verify_and_add_patch() says:
"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."

>> 2) This function single caller (__load_microcode_amd()) normalized any
>> error that verify_and_add_patch() returned to UCODE_ERROR anyway,
> 
> So?

This means that there is no loss of information here.

The function these three points are about (verify_and_add_patch()) is
declared as "static", so it cannot be called from any other kernel code.

>> 3) The existing code uses a convention that zero return value means
>> 'terminate processing' for the parse_container() function in the early
>> loader which normally returns a 'bytes consumed' value, as this function
>> does.
> 
> parse_container() could very well change its convention to return
> negative on error and positive value if the loop is supposed to skip
> bytes.
> 

Yes, but then the problem from the point 1) above will be introduced
also to parse_container().

Maciej

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Borislav Petkov]

On Tue, May 01, 2018 at 06:19:56PM +0200, Maciej S. Szmigiero wrote:
> -EINVAL cast to unsigned int is 4294967274 and this value is also
> a valid count of bytes to skip that this function can return.

And where exactly in the *old* code do we do that?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Maciej S. Szmigiero]

On 01.05.2018 22:03, Borislav Petkov wrote:
> On Tue, May 01, 2018 at 06:19:56PM +0200, Maciej S. Szmigiero wrote:
>> -EINVAL cast to unsigned int is 4294967274 and this value is also
>> a valid count of bytes to skip that this function can return.
> 
> And where exactly in the *old* code do we do that?
> 

The old code returned this value as a signed int, but then any
"patch_size" value (which is u32) above INT_MAX read from a section header
wrapped around to a negative pseudo-error code (which likely didn't match
any actual error number).

Maciej

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Borislav Petkov]

On Wed, May 02, 2018 at 02:47:39AM +0200, Maciej S. Szmigiero wrote:
> On 01.05.2018 22:03, Borislav Petkov wrote:
> > On Tue, May 01, 2018 at 06:19:56PM +0200, Maciej S. Szmigiero wrote:
> >> -EINVAL cast to unsigned int is 4294967274 and this value is also
> >> a valid count of bytes to skip that this function can return.
> > 
> > And where exactly in the *old* code do we do that?
> 
> The old code returned this value as a signed int, but then any
> "patch_size" value (which is u32) above INT_MAX read from a section header
> wrapped around to a negative pseudo-error code (which likely didn't match
> any actual error number).

Lemme repeat my question: *where* *exactly* in the old code do we do that?

Feel free to paste snippets to show what you mean.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Maciej S. Szmigiero]

On 03.05.2018 12:01, Borislav Petkov wrote:
> On Wed, May 02, 2018 at 02:47:39AM +0200, Maciej S. Szmigiero wrote:
>> On 01.05.2018 22:03, Borislav Petkov wrote:
>>> On Tue, May 01, 2018 at 06:19:56PM +0200, Maciej S. Szmigiero wrote:
>>>> -EINVAL cast to unsigned int is 4294967274 and this value is also
>>>> a valid count of bytes to skip that this function can return.
>>>
>>> And where exactly in the *old* code do we do that?
>>
>> The old code returned this value as a signed int, but then any
>> "patch_size" value (which is u32) above INT_MAX read from a section header
>> wrapped around to a negative pseudo-error code (which likely didn't match
>> any actual error number).
> 
> Lemme repeat my question: *where* *exactly* in the old code do we do that?
> 
> Feel free to paste snippets to show what you mean.
> 

>From verify_and_add_patch():
> static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
> {
> 	struct microcode_header_amd *mc_hdr;
> 	struct ucode_patch *patch;
> 	unsigned int patch_size, crnt_size, ret;
> 	u32 proc_fam;
> 	u16 proc_id;
> 
> 	patch_size  = *(u32 *)(fw + 4);

Here we read a u32 (= unsigned int) value from a section header
and store it into an unsigned int variable.

> 	crnt_size   = patch_size + SECTION_HDR_SIZE;

Here we add 8 (SECTION_HDR_SIZE) to this value and once again store it
into an unsigned int variable.

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

Here we return this variable, implicitly converting it into a
(signed) int.
Any value above INT_MAX will wrap around to a negative pseudo-error
code (which might not match any actual error number).

Maciej

Re: [PATCH v5 4/6] x86/microcode/AMD: Check microcode container data in the late loader

[Borislav Petkov]

On Fri, May 04, 2018 at 01:26:50AM +0200, Maciej S. Szmigiero wrote:
> Here we return this variable, implicitly converting it into a
> (signed) int.
> Any value above INT_MAX will wrap around to a negative pseudo-error
> code (which might not match any actual error number).

If you want to widen the return type and do proper checking then have
the function return int which denotes success (0) or negative on
error and then return crnt_size through an u32 * arg.

All this in a separate patch with properly explained situation in the
commit message.

Thx.

-- 
Regards/Gruss,
    Boris.

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