All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it
@ 2018-06-19 18:47 Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 1/9] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length Maciej S. Szmigiero
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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.

Changes from v6:
* Rewrite comments describing checking functions in the imperative mood
and make sure they don't refer to "@buf_size" parameter as "@size",

* Remove a call to verify_container() from parse_container() since
invoking verify_equivalence_table() is enough as it includes such a call,

* Remove a now-unnecessary additional error message in
__load_microcode_amd() if install_equiv_cpu_table() fails,

* Introduce a check whether the indicated size of a patch makes sense
for its declared CPU family - for all CPU families known to the driver
so we don't skip over good patches by mistake if the indicated size
turns out to be something nonsensically huge,

* Make sure we skip just one byte of microcode data in the late loader
if a patch section does not have a plausible header so we try harder to
not skip good patches in presence of some garbage between patch sections,

* Convert pointer to the CPU equivalence table global static variable
into a struct descriptor,

* Add CPU equivalence table size field to the above descriptor and use
it to prevent scanning past this table in both the early and the late
loader.

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v7 1/9] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-11-19 10:12   ` [tip:x86/microcode] " tip-bot for Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 2/9] x86/microcode/AMD: Add microcode container data checking functions Maciej S. Szmigiero
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 2/9] x86/microcode/AMD: Add microcode container data checking functions
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 1/9] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-11-19 10:13   ` [tip:x86/microcode] x86/microcode/AMD: Add microcode container verification tip-bot for Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch() Maciej S. Szmigiero
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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 | 180 +++++++++++++++++++++++++++-
 1 file changed, 177 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index dc8ea9a9d962..120778771909 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -73,6 +73,182 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 	return 0;
 }
 
+/*
+ * Check whether there is a valid microcode container file at the beginning
+ * of a passed buffer @buf of size @buf_size.
+ * If @early is set do not print errors so this function is 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;
+}
+
+/*
+ * Check whether there is a valid, non-truncated CPU equivalence table
+ * at the beginning of a passed buffer @buf of size @buf_size.
+ * If @early is set do not print errors so this function is 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;
+}
+
+/*
+ * Check whether there is a valid, non-truncated microcode patch section
+ * at the beginning of a passed buffer @buf of size @buf_size.
+ * If @early is set do not print errors so this function is usable by the
+ * early microcode loader.
+ */
+static bool verify_patch_section(const u8 *buf, size_t buf_size, bool early)
+{
+	const u32 *hdr;
+	u32 patch_type, patch_size;
+
+	if (buf_size < SECTION_HDR_SIZE) {
+		if (!early)
+			pr_err("Truncated patch section.\n");
+
+		return false;
+	}
+
+	hdr = (const u32 *)buf;
+	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;
+	}
+
+	return true;
+}
+
+static unsigned int verify_patch_size(u8 family, u32 patch_size,
+				      unsigned int size);
+
+/*
+ * Check whether there is a valid, non-truncated microcode patch of the
+ * right size for a particular family @family at the beginning of a passed
+ * buffer @buf, buffer that is of size @buf_size.
+ * Will return the length of current patch data in @crnt_size (even on
+ * failure), so the caller knows how many bytes it should skip.
+ * If @early is set do not print errors so this function is usable by the
+ * early microcode loader.
+ */
+static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
+			 unsigned int *crnt_size, bool early)
+{
+	const u32 *hdr;
+	u32 patch_size;
+	const struct microcode_header_amd *mc_hdr;
+	u8 patch_fam;
+
+	if (!verify_patch_section(buf, buf_size, early)) {
+		*crnt_size = min_t(unsigned int, 1, buf_size);
+		return false;
+	}
+
+	hdr = (const u32 *)buf;
+	patch_size = hdr[1];
+
+	if (buf_size - SECTION_HDR_SIZE < patch_size) {
+		if (!early)
+			pr_err("Patch of size %u truncated.\n", patch_size);
+
+		*crnt_size = buf_size;
+		return false;
+	}
+
+	/*
+	 * Set a patch length limit of slightly less than U32_MAX to
+	 * prevent overflowing 32-bit variables holding the whole
+	 * patch section size.
+	 */
+	if (patch_size > U32_MAX - SECTION_HDR_SIZE) {
+		if (!early)
+			pr_err("Patch of size %u too large.\n", patch_size);
+
+		*crnt_size = SECTION_HDR_SIZE + PATCH_MAX_SIZE;
+		return false;
+	}
+
+	*crnt_size = SECTION_HDR_SIZE + patch_size;
+
+	mc_hdr = (const struct microcode_header_amd *)(buf + SECTION_HDR_SIZE);
+	patch_fam = 0xf + (mc_hdr->processor_rev_id >> 12);
+
+	/* Is the patch for the proper CPU family? */
+	if (family != patch_fam)
+		return false;
+
+	/*
+	 * 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 +670,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;
 }

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 1/9] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 2/9] x86/microcode/AMD: Add microcode container data checking functions Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-06-21  8:36   ` Borislav Petkov
  2018-06-19 18:47 ` [PATCH v7 4/9] x86/microcode/AMD: Check microcode container data in the early loader Maciej S. Szmigiero
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

Integrating verify_patch_size() into verify_patch() allows us to check
whether the indicated patch size makes sense for its indicated CPU family -
for all CPU families known to the driver.

If we spot a patch that is longer than expected for its family we'll
carefully skip over only the expected length to make sure we don't miss
good patches in case the indicated patch size was something nonsensically
huge.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 120778771909..67147e784c0e 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -176,9 +176,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);
-
 /*
  * Check whether there is a valid, non-truncated microcode patch of the
  * right size for a particular family @family at the beginning of a passed
@@ -192,7 +189,7 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
 			 unsigned int *crnt_size, bool early)
 {
 	const u32 *hdr;
-	u32 patch_size;
+	u32 patch_size, max_size;
 	const struct microcode_header_amd *mc_hdr;
 	u8 patch_fam;
 
@@ -204,48 +201,79 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
 	hdr = (const u32 *)buf;
 	patch_size = hdr[1];
 
-	if (buf_size - SECTION_HDR_SIZE < patch_size) {
+	if (buf_size - SECTION_HDR_SIZE < sizeof(*mc_hdr)) {
 		if (!early)
-			pr_err("Patch of size %u truncated.\n", patch_size);
+			pr_err("Truncated patch header.\n");
 
 		*crnt_size = buf_size;
 		return false;
 	}
 
+	mc_hdr = (const struct microcode_header_amd *)(buf + SECTION_HDR_SIZE);
+	patch_fam = 0xf + (mc_hdr->processor_rev_id >> 12);
+
 	/*
-	 * Set a patch length limit of slightly less than U32_MAX to
-	 * prevent overflowing 32-bit variables holding the whole
-	 * patch section size.
+	 * Check whether patch_size isn't something nonsensically huge so
+	 * we don't skip over good patches by mistake.
 	 */
-	if (patch_size > U32_MAX - SECTION_HDR_SIZE) {
-		if (!early)
-			pr_err("Patch of size %u too large.\n", patch_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
 
-		*crnt_size = SECTION_HDR_SIZE + PATCH_MAX_SIZE;
-		return false;
+	switch (patch_fam) {
+	case 0x10 ... 0x13:
+		max_size = F1XH_MPB_MAX_SIZE;
+		break;
+	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:
+		/*
+		 * Don't know the max size for future families...
+		 * Set a patch length limit of slightly less than U32_MAX to
+		 * prevent overflowing 32-bit variables holding the whole
+		 * patch section size.
+		 */
+		max_size = U32_MAX - SECTION_HDR_SIZE;
+		break;
 	}
 
-	*crnt_size = SECTION_HDR_SIZE + patch_size;
-
-	mc_hdr = (const struct microcode_header_amd *)(buf + SECTION_HDR_SIZE);
-	patch_fam = 0xf + (mc_hdr->processor_rev_id >> 12);
+	if (patch_size > max_size) {
+		if (!early)
+			pr_err("Patch of size %u exceeds family maximum.\n",
+			       patch_size);
 
-	/* Is the patch for the proper CPU family? */
-	if (family != patch_fam)
+		*crnt_size = min_t(unsigned int,
+				   SECTION_HDR_SIZE + max_size,
+				   buf_size);
 		return false;
+	}
 
-	/*
-	 * 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 (buf_size - SECTION_HDR_SIZE < patch_size) {
 		if (!early)
-			pr_err("Patch of size %u too large.\n", patch_size);
+			pr_err("Patch of size %u truncated.\n", patch_size);
 
+		*crnt_size = buf_size;
 		return false;
 	}
 
+	*crnt_size = SECTION_HDR_SIZE + patch_size;
+
+	/* Is the patch for the proper CPU family? */
+	if (family != patch_fam)
+		return false;
+
 	return true;
 }
 
@@ -637,45 +665,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);
@@ -765,7 +754,7 @@ 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;
+	unsigned int patch_size, crnt_size;
 	u32 proc_fam;
 	u16 proc_id;
 
@@ -791,13 +780,7 @@ 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) {
+	if (!verify_patch(family, fw, leftover, &crnt_size, false)) {
 		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
 		return crnt_size;
 	}

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 4/9] x86/microcode/AMD: Check microcode container data in the early loader
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
                   ` (2 preceding siblings ...)
  2018-06-19 18:47 ` [PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch() Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 5/9] x86/microcode/AMD: Split status from data to skip in verify_and_add_patch() Maciej S. Szmigiero
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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

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 | 70 ++++++++++++++++-------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 67147e784c0e..c05531540b57 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -284,29 +284,35 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
  * 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_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
@@ -314,30 +320,36 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 	 */
 	while (size > 0) {
 		struct microcode_amd *mc;
+		unsigned int crnt_size;
 		u32 patch_size;
 
-		hdr = (u32 *)buf;
-
-		if (hdr[0] != UCODE_UCODE_TYPE)
+		/*
+		 * If this patch section hasn't got a plausible header we
+		 * probably have arrived at the beginning of a next container.
+		 * Bail out from patch processing here so this new container
+		 * can be scanned.
+		 */
+		if (!verify_patch_section(buf, size, true))
 			break;
 
-		/* Sanity-check patch size. */
+		if (!verify_patch(x86_family(desc->cpuid_1_eax),
+				  buf, size, &crnt_size, true))
+			goto next_patch;
+
+		hdr = (u32 *)buf;
 		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;
-		}
+		/* We have found a matching patch! */
+		desc->psize = patch_size;
+		desc->mc = mc;
 
-		buf  += patch_size;
-		size -= patch_size;
+next_patch:
+		buf  += crnt_size;
+		size -= crnt_size;
 	}
 
 	/*
@@ -363,15 +375,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;
 	}
 }
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 5/9] x86/microcode/AMD: Split status from data to skip in verify_and_add_patch()
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
                   ` (3 preceding siblings ...)
  2018-06-19 18:47 ` [PATCH v7 4/9] x86/microcode/AMD: Check microcode container data in the early loader Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 6/9] x86/microcode/AMD: Check microcode container data in the late loader Maciej S. Szmigiero
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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 | 35 +++++++++++++++--------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index c05531540b57..9e29374ce4d0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -754,45 +754,46 @@ 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;
+	unsigned int patch_size;
 	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;
 	}
 
-	if (!verify_patch(family, fw, leftover, &crnt_size, false)) {
+	if (!verify_patch(family, fw, leftover, crnt_size, false)) {
 		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
-		return crnt_size;
+		return 0;
 	}
 
 	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
@@ -818,7 +819,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,
@@ -827,7 +828,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);
@@ -845,8 +845,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;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 6/9] x86/microcode/AMD: Check microcode container data in the late loader
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
                   ` (4 preceding siblings ...)
  2018-06-19 18:47 ` [PATCH v7 5/9] x86/microcode/AMD: Split status from data to skip in verify_and_add_patch() Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 7/9] x86/microcode/AMD: Add a reminder about PATCH_MAX_SIZE macro Maciej S. Szmigiero
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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 | 67 +++++++++++++----------------
 1 file changed, 30 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 9e29374ce4d0..2dbdd7951475 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -717,28 +717,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)
@@ -763,14 +761,18 @@ static void cleanup(void)
 static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
 				unsigned int *crnt_size)
 {
+	const u32 *hdr;
 	struct microcode_header_amd *mc_hdr;
 	struct ucode_patch *patch;
-	unsigned int patch_size;
+	u32 patch_size;
 	u32 proc_fam;
 	u16 proc_id;
 
-	patch_size  = *(u32 *)(fw + 4);
-	*crnt_size  = patch_size + SECTION_HDR_SIZE;
+	if (!verify_patch(family, fw, leftover, crnt_size, false))
+		return 0;
+
+	hdr = (const u32 *)fw;
+	patch_size  = hdr[1];
 	mc_hdr	    = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
 	proc_id	    = mc_hdr->processor_rev_id;
 
@@ -791,11 +793,6 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
 		return 0;
 	}
 
-	if (!verify_patch(family, fw, leftover, crnt_size, false)) {
-		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
-		return 0;
-	}
-
 	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
 	if (!patch) {
 		pr_err("Patch allocation failure.\n");
@@ -828,21 +825,19 @@ 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) {
-		pr_err("failed to create equivalent cpu table\n");
+	offset = install_equiv_cpu_table(data, size);
+	if (!offset)
 		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;
@@ -930,10 +925,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);
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 7/9] x86/microcode/AMD: Add a reminder about PATCH_MAX_SIZE macro
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
                   ` (5 preceding siblings ...)
  2018-06-19 18:47 ` [PATCH v7 6/9] x86/microcode/AMD: Check microcode container data in the late loader Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 8/9] x86/microcode/AMD: Convert CPU equivalence table variable into a struct Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 9/9] x86/microcode/AMD: Check the equivalence table size when scanning it Maciej S. Szmigiero
  8 siblings, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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  | 3 +++
 2 files changed, 4 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 2dbdd7951475..9807e27f8fde 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -215,6 +215,9 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
 	/*
 	 * Check whether patch_size isn't something nonsensically huge so
 	 * we don't skip over good patches by mistake.
+	 *
+	 * 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 8/9] x86/microcode/AMD: Convert CPU equivalence table variable into a struct
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
                   ` (6 preceding siblings ...)
  2018-06-19 18:47 ` [PATCH v7 7/9] x86/microcode/AMD: Add a reminder about PATCH_MAX_SIZE macro Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  2018-06-19 18:47 ` [PATCH v7 9/9] x86/microcode/AMD: Check the equivalence table size when scanning it Maciej S. Szmigiero
  8 siblings, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

Convert pointer to CPU equivalence table global static variable into a
struct descriptor in preparation for tracking also the size of this table.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 9807e27f8fde..10aabd9d3121 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -38,7 +38,9 @@
 #include <asm/cpu.h>
 #include <asm/msr.h>
 
-static struct equiv_cpu_entry *equiv_cpu_table;
+static struct equiv_cpu_table {
+	struct equiv_cpu_entry *table;
+} equiv_table;
 
 /*
  * This points to the current valid container of microcode patches which we will
@@ -581,18 +583,18 @@ 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_table.table, uci->cpu_sig.sig);
 }
 
 static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
 {
 	int i = 0;
 
-	BUG_ON(!equiv_cpu_table);
+	BUG_ON(!equiv_table.table);
 
-	while (equiv_cpu_table[i].equiv_cpu != 0) {
-		if (equiv_cpu == equiv_cpu_table[i].equiv_cpu)
-			return equiv_cpu_table[i].installed_cpu;
+	while (equiv_table.table[i].equiv_cpu != 0) {
+		if (equiv_cpu == equiv_table.table[i].equiv_cpu)
+			return equiv_table.table[i].installed_cpu;
 		i++;
 	}
 	return 0;
@@ -731,21 +733,21 @@ static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 	hdr = (const u32 *)buf;
 	equiv_tbl_len = hdr[2];
 
-	equiv_cpu_table = vmalloc(equiv_tbl_len);
-	if (!equiv_cpu_table) {
+	equiv_table.table = vmalloc(equiv_tbl_len);
+	if (!equiv_table.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_table.table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
 
 	return equiv_tbl_len;
 }
 
 static void free_equiv_cpu_table(void)
 {
-	vfree(equiv_cpu_table);
-	equiv_cpu_table = NULL;
+	vfree(equiv_table.table);
+	equiv_table.table = NULL;
 }
 
 static void cleanup(void)

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v7 9/9] x86/microcode/AMD: Check the equivalence table size when scanning it
  2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
                   ` (7 preceding siblings ...)
  2018-06-19 18:47 ` [PATCH v7 8/9] x86/microcode/AMD: Convert CPU equivalence table variable into a struct Maciej S. Szmigiero
@ 2018-06-19 18:47 ` Maciej S. Szmigiero
  8 siblings, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-19 18:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

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 10aabd9d3121..7548dfcf12b0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -40,6 +40,7 @@
 
 static struct equiv_cpu_table {
 	struct equiv_cpu_entry *table;
+	unsigned int entries;
 } equiv_table;
 
 /*
@@ -65,12 +66,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;
 }
@@ -312,7 +319,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;
@@ -583,20 +591,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_table.table, uci->cpu_sig.sig);
+	return find_equiv_id(equiv_table.table, equiv_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_table.table);
 
-	while (equiv_table.table[i].equiv_cpu != 0) {
+	for (i = 0; i < equiv_table.entries &&
+		     equiv_table.table[i].equiv_cpu != 0; i++)
 		if (equiv_cpu == equiv_table.table[i].equiv_cpu)
 			return equiv_table.table[i].installed_cpu;
-		i++;
-	}
+
 	return 0;
 }
 
@@ -740,6 +749,7 @@ static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 	}
 
 	memcpy(equiv_table.table, buf + CONTAINER_HDR_SZ, equiv_tbl_len);
+	equiv_table.entries = equiv_tbl_len / sizeof(struct equiv_cpu_entry);
 
 	return equiv_tbl_len;
 }
@@ -748,6 +758,7 @@ static void free_equiv_cpu_table(void)
 {
 	vfree(equiv_table.table);
 	equiv_table.table = NULL;
+	equiv_table.entries = 0;
 }
 
 static void cleanup(void)

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()
  2018-06-19 18:47 ` [PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch() Maciej S. Szmigiero
@ 2018-06-21  8:36   ` Borislav Petkov
  2018-06-22 22:32     ` [PATCH 1/2] " Maciej S. Szmigiero
  2018-06-22 22:33     ` [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families Maciej S. Szmigiero
  0 siblings, 2 replies; 17+ messages in thread
From: Borislav Petkov @ 2018-06-21  8:36 UTC (permalink / raw)
  To: Maciej S. Szmigiero
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

On Tue, Jun 19, 2018 at 08:47:33PM +0200, Maciej S. Szmigiero wrote:
> Integrating verify_patch_size() into verify_patch() allows us to check
> whether the indicated patch size makes sense for its indicated CPU family -
> for all CPU families known to the driver.
> 
> If we spot a patch that is longer than expected for its family we'll
> carefully skip over only the expected length to make sure we don't miss
> good patches in case the indicated patch size was something nonsensically
> huge.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 131 ++++++++++++----------------
>  1 file changed, 57 insertions(+), 74 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 120778771909..67147e784c0e 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -176,9 +176,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);
> -
>  /*
>   * Check whether there is a valid, non-truncated microcode patch of the
>   * right size for a particular family @family at the beginning of a passed
> @@ -192,7 +189,7 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>  			 unsigned int *crnt_size, bool early)
>  {
>  	const u32 *hdr;
> -	u32 patch_size;
> +	u32 patch_size, max_size;
>  	const struct microcode_header_amd *mc_hdr;
>  	u8 patch_fam;
>  
> @@ -204,48 +201,79 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>  	hdr = (const u32 *)buf;
>  	patch_size = hdr[1];
>  
> -	if (buf_size - SECTION_HDR_SIZE < patch_size) {
> +	if (buf_size - SECTION_HDR_SIZE < sizeof(*mc_hdr)) {
>  		if (!early)
> -			pr_err("Patch of size %u truncated.\n", patch_size);
> +			pr_err("Truncated patch header.\n");
>  
>  		*crnt_size = buf_size;
>  		return false;
>  	}
>  
> +	mc_hdr = (const struct microcode_header_amd *)(buf + SECTION_HDR_SIZE);
> +	patch_fam = 0xf + (mc_hdr->processor_rev_id >> 12);
> +
>  	/*
> -	 * Set a patch length limit of slightly less than U32_MAX to
> -	 * prevent overflowing 32-bit variables holding the whole
> -	 * patch section size.
> +	 * Check whether patch_size isn't something nonsensically huge so
> +	 * we don't skip over good patches by mistake.
>  	 */
> -	if (patch_size > U32_MAX - SECTION_HDR_SIZE) {
> -		if (!early)
> -			pr_err("Patch of size %u too large.\n", patch_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
>  
> -		*crnt_size = SECTION_HDR_SIZE + PATCH_MAX_SIZE;
> -		return false;
> +	switch (patch_fam) {
> +	case 0x10 ... 0x13:

This is not how you integrate a function into another one.

In the first patch, you move the code verbatim to the place where you
want it to be, without adding any other changes whatsoever. The diff
needs to show code movement only.

In follow-on patches you do the other changes you've done and you
explain why.

But no need to resend the whole series - just split this patch as
suggested and send the new ones as a reply to this subthread.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/2] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()
  2018-06-21  8:36   ` Borislav Petkov
@ 2018-06-22 22:32     ` Maciej S. Szmigiero
  2018-06-22 22:33     ` [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families Maciej S. Szmigiero
  1 sibling, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-22 22:32 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

Integrating verify_patch_size() into verify_patch() will allows us to
introduce in the next commit a check whether the indicated patch size makes
sense for its indicated CPU family - for all CPU families known to the
driver.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
This is part 1/2 of a replacement for
"[PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()".

 arch/x86/kernel/cpu/microcode/amd.c | 82 ++++++++++-------------------
 1 file changed, 29 insertions(+), 53 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 120778771909..53820b92aabb 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -176,9 +176,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);
-
 /*
  * Check whether there is a valid, non-truncated microcode patch of the
  * right size for a particular family @family at the beginning of a passed
@@ -192,7 +189,7 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
 			 unsigned int *crnt_size, bool early)
 {
 	const u32 *hdr;
-	u32 patch_size;
+	u32 patch_size, max_size;
 	const struct microcode_header_amd *mc_hdr;
 	u8 patch_fam;
 
@@ -234,12 +231,36 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
 	if (family != patch_fam)
 		return false;
 
+#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, buf_size - SECTION_HDR_SIZE, max_size)) {
 		if (!early)
 			pr_err("Patch of size %u too large.\n", patch_size);
 
@@ -637,45 +658,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);
@@ -765,7 +747,7 @@ 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;
+	unsigned int patch_size, crnt_size;
 	u32 proc_fam;
 	u16 proc_id;
 
@@ -791,13 +773,7 @@ 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) {
+	if (!verify_patch(family, fw, leftover, &crnt_size, false)) {
 		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
 		return crnt_size;
 	}
-- 
2.17.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families
  2018-06-21  8:36   ` Borislav Petkov
  2018-06-22 22:32     ` [PATCH 1/2] " Maciej S. Szmigiero
@ 2018-06-22 22:33     ` Maciej S. Szmigiero
  2018-06-25 18:37       ` Borislav Petkov
  1 sibling, 1 reply; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-22 22:33 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

Previously, the AMD microcode update driver has only checked the indicated
microcode patch size for patches for the currently running CPU family.
Patches for other families had their size trusted without further
verification.

Introduce such check for all CPU families known to the driver so if we spot
a patch that is longer than expected for its family we'll carefully skip
over only the expected length to make sure we don't miss good patches in
case the indicated patch size was something nonsensically huge.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
This is part 2/2 of a replacement for
"[PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()".

 arch/x86/kernel/cpu/microcode/amd.c | 67 ++++++++++++++++-------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 53820b92aabb..04a298da4c21 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -201,43 +201,31 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
 	hdr = (const u32 *)buf;
 	patch_size = hdr[1];
 
-	if (buf_size - SECTION_HDR_SIZE < patch_size) {
+	if (buf_size - SECTION_HDR_SIZE < sizeof(*mc_hdr)) {
 		if (!early)
-			pr_err("Patch of size %u truncated.\n", patch_size);
+			pr_err("Truncated patch header.\n");
 
 		*crnt_size = buf_size;
 		return false;
 	}
 
-	/*
-	 * Set a patch length limit of slightly less than U32_MAX to
-	 * prevent overflowing 32-bit variables holding the whole
-	 * patch section size.
-	 */
-	if (patch_size > U32_MAX - SECTION_HDR_SIZE) {
-		if (!early)
-			pr_err("Patch of size %u too large.\n", patch_size);
-
-		*crnt_size = SECTION_HDR_SIZE + PATCH_MAX_SIZE;
-		return false;
-	}
-
-	*crnt_size = SECTION_HDR_SIZE + patch_size;
-
 	mc_hdr = (const struct microcode_header_amd *)(buf + SECTION_HDR_SIZE);
 	patch_fam = 0xf + (mc_hdr->processor_rev_id >> 12);
 
-	/* Is the patch for the proper CPU family? */
-	if (family != patch_fam)
-		return false;
-
+	/*
+	 * Check whether patch_size isn't something nonsensically huge so
+	 * we don't skip over good patches by mistake.
+	 */
 #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) {
+	switch (patch_fam) {
+	case 0x10 ... 0x13:
+		max_size = F1XH_MPB_MAX_SIZE;
+		break;
 	case 0x14:
 		max_size = F14H_MPB_MAX_SIZE;
 		break;
@@ -251,22 +239,41 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
 		max_size = F17H_MPB_MAX_SIZE;
 		break;
 	default:
-		max_size = F1XH_MPB_MAX_SIZE;
+		/*
+		 * Don't know the max size for future families...
+		 * Set a patch length limit of slightly less than U32_MAX to
+		 * prevent overflowing 32-bit variables holding the whole
+		 * patch section size.
+		 */
+		max_size = U32_MAX - SECTION_HDR_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 from the leftover file length.
-	 */
-	if (patch_size > min_t(u32, buf_size - SECTION_HDR_SIZE, max_size)) {
+	if (patch_size > max_size) {
+		if (!early)
+			pr_err("Patch of size %u exceeds family %u maximum.\n",
+			       patch_size, (unsigned int)patch_fam);
+
+		*crnt_size = min_t(unsigned int,
+				   SECTION_HDR_SIZE + max_size,
+				   buf_size);
+		return false;
+	}
+
+	if (buf_size - SECTION_HDR_SIZE < patch_size) {
 		if (!early)
-			pr_err("Patch of size %u too large.\n", patch_size);
+			pr_err("Patch of size %u truncated.\n", patch_size);
 
+		*crnt_size = buf_size;
 		return false;
 	}
 
+	*crnt_size = SECTION_HDR_SIZE + patch_size;
+
+	/* Is the patch for the proper CPU family? */
+	if (family != patch_fam)
+		return false;
+
 	return true;
 }
 
-- 
2.17.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families
  2018-06-22 22:33     ` [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families Maciej S. Szmigiero
@ 2018-06-25 18:37       ` Borislav Petkov
  2018-06-25 22:18         ` Maciej S. Szmigiero
  0 siblings, 1 reply; 17+ messages in thread
From: Borislav Petkov @ 2018-06-25 18:37 UTC (permalink / raw)
  To: Maciej S. Szmigiero
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

On Sat, Jun 23, 2018 at 12:33:24AM +0200, Maciej S. Szmigiero wrote:
> Previously, the AMD microcode update driver has only checked the indicated
> microcode patch size for patches for the currently running CPU family.
> Patches for other families had their size trusted without further
> verification.
> 
> Introduce such check for all CPU families known to the driver so if we spot
> a patch that is longer than expected for its family we'll carefully skip
> over only the expected length to make sure we don't miss good patches in
> case the indicated patch size was something nonsensically huge.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
> This is part 2/2 of a replacement for
> "[PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()".
> 
>  arch/x86/kernel/cpu/microcode/amd.c | 67 ++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 53820b92aabb..04a298da4c21 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -201,43 +201,31 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>  	hdr = (const u32 *)buf;
>  	patch_size = hdr[1];
>  
> -	if (buf_size - SECTION_HDR_SIZE < patch_size) {
> +	if (buf_size - SECTION_HDR_SIZE < sizeof(*mc_hdr)) {

This function adds and removes SECTION_HDR_SIZE a bunch of times so that
my head spins trying to remind myself which is which. Please define
properly named local variables.

>  		if (!early)
> -			pr_err("Patch of size %u truncated.\n", patch_size);
> +			pr_err("Truncated patch header.\n");
>  
>  		*crnt_size = buf_size;
>  		return false;
>  	}
>  
> -	/*
> -	 * Set a patch length limit of slightly less than U32_MAX to
> -	 * prevent overflowing 32-bit variables holding the whole
> -	 * patch section size.
> -	 */
> -	if (patch_size > U32_MAX - SECTION_HDR_SIZE) {
> -		if (!early)
> -			pr_err("Patch of size %u too large.\n", patch_size);
> -
> -		*crnt_size = SECTION_HDR_SIZE + PATCH_MAX_SIZE;
> -		return false;
> -	}
> -
> -	*crnt_size = SECTION_HDR_SIZE + patch_size;
> -
>  	mc_hdr = (const struct microcode_header_amd *)(buf + SECTION_HDR_SIZE);
>  	patch_fam = 0xf + (mc_hdr->processor_rev_id >> 12);
>  
> -	/* Is the patch for the proper CPU family? */
> -	if (family != patch_fam)
> -		return false;
> -
> +	/*
> +	 * Check whether patch_size isn't something nonsensically huge so
> +	 * we don't skip over good patches by mistake.
> +	 */
>  #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) {
> +	switch (patch_fam) {
> +	case 0x10 ... 0x13:
> +		max_size = F1XH_MPB_MAX_SIZE;

I guess this variable should be called fam_size now.

> +		break;
>  	case 0x14:
>  		max_size = F14H_MPB_MAX_SIZE;
>  		break;
> @@ -251,22 +239,41 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>  		max_size = F17H_MPB_MAX_SIZE;
>  		break;
>  	default:
> -		max_size = F1XH_MPB_MAX_SIZE;
> +		/*
> +		 * Don't know the max size for future families...
> +		 * Set a patch length limit of slightly less than U32_MAX to
> +		 * prevent overflowing 32-bit variables holding the whole
> +		 * patch section size.
> +		 */
> +		max_size = U32_MAX - SECTION_HDR_SIZE;

No, just do a WARN_ON_ONCE here.

>  		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 from the leftover file length.
> -	 */
> -	if (patch_size > min_t(u32, buf_size - SECTION_HDR_SIZE, max_size)) {
> +	if (patch_size > max_size) {
> +		if (!early)
> +			pr_err("Patch of size %u exceeds family %u maximum.\n",
> +			       patch_size, (unsigned int)patch_fam);
> +
> +		*crnt_size = min_t(unsigned int,
> +				   SECTION_HDR_SIZE + max_size,
> +				   buf_size);
> +		return false;
> +	}
> +
> +	if (buf_size - SECTION_HDR_SIZE < patch_size) {
>  		if (!early)
> -			pr_err("Patch of size %u too large.\n", patch_size);
> +			pr_err("Patch of size %u truncated.\n", patch_size);
>  
> +		*crnt_size = buf_size;
>  		return false;
>  	}
>  
> +	*crnt_size = SECTION_HDR_SIZE + patch_size;
> +
> +	/* Is the patch for the proper CPU family? */
> +	if (family != patch_fam)
> +		return false;

Why are you moving the family check down?

What it should do instead is the moment it knows the family, check
whether they're equal. If not, skip over with minimum of the size of the
corresponding patch_fam and buf_size.

And then you do all the remaining checks.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families
  2018-06-25 18:37       ` Borislav Petkov
@ 2018-06-25 22:18         ` Maciej S. Szmigiero
  0 siblings, 0 replies; 17+ messages in thread
From: Maciej S. Szmigiero @ 2018-06-25 22:18 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel

On 25.06.2018 20:37, Borislav Petkov wrote:
(..)
>> +		break;
>>  	case 0x14:
>>  		max_size = F14H_MPB_MAX_SIZE;
>>  		break;
>> @@ -251,22 +239,41 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>>  		max_size = F17H_MPB_MAX_SIZE;
>>  		break;
>>  	default:
>> -		max_size = F1XH_MPB_MAX_SIZE;
>> +		/*
>> +		 * Don't know the max size for future families...
>> +		 * Set a patch length limit of slightly less than U32_MAX to
>> +		 * prevent overflowing 32-bit variables holding the whole
>> +		 * patch section size.
>> +		 */
>> +		max_size = U32_MAX - SECTION_HDR_SIZE;
> 
> No, just do a WARN_ON_ONCE here.

So you want to have just WARN_ON_ONCE and no max_size (or fam_size) check
in such case of an unknown family number?

We still need to cap patch_size at that default case above value (or less)
so adding the section header size later won't overflow.

> 
>>  		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 from the leftover file length.
>> -	 */
>> -	if (patch_size > min_t(u32, buf_size - SECTION_HDR_SIZE, max_size)) {
>> +	if (patch_size > max_size) {
>> +		if (!early)
>> +			pr_err("Patch of size %u exceeds family %u maximum.\n",
>> +			       patch_size, (unsigned int)patch_fam);
>> +
>> +		*crnt_size = min_t(unsigned int,
>> +				   SECTION_HDR_SIZE + max_size,
>> +				   buf_size);
>> +		return false;
>> +	}
>> +
>> +	if (buf_size - SECTION_HDR_SIZE < patch_size) {
>>  		if (!early)
>> -			pr_err("Patch of size %u too large.\n", patch_size);
>> +			pr_err("Patch of size %u truncated.\n", patch_size);
>>  
>> +		*crnt_size = buf_size;
>>  		return false;
>>  	}
>>  
>> +	*crnt_size = SECTION_HDR_SIZE + patch_size;
>> +
>> +	/* Is the patch for the proper CPU family? */
>> +	if (family != patch_fam)
>> +		return false;
> 
> Why are you moving the family check down?
> 
> What it should do instead is the moment it knows the family, check
> whether they're equal. If not, skip over with minimum of the size of the
> corresponding patch_fam and buf_size.
> 
> And then you do all the remaining checks.
> 

Previously the family check was done before computing patch_fam so the
biggest component of the move was jumping over past that computation
(which we have to do anyway to know how many bytes to skip).

Moving the family check further down past these two last checks visible
in the patch hunk above allows us to:
1) Produce an error message that the indicated patch size exceeds family
maximum - for CPU families other that the currently running one.

This is useful in case the patch actually should be of that indicated
size - for some reason - since in this case by skipping over a smaller
length we will end in the middle of a patch and so we'll produce a
bunch of error messages about invalid patch section type field, etc.,
until the driver finally resynchronizes on the next section boundary.

2) In case the last patch (for another family) was truncated a proper
diagnostic message would be printed instead of the patch being skipped
silently.

Maciej

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [tip:x86/microcode] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length
  2018-06-19 18:47 ` [PATCH v7 1/9] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length Maciej S. Szmigiero
@ 2018-11-19 10:12   ` tip-bot for Maciej S. Szmigiero
  0 siblings, 0 replies; 17+ messages in thread
From: tip-bot for Maciej S. Szmigiero @ 2018-11-19 10:12 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: bp, x86, mingo, linux-kernel, mail, hpa, tglx

Commit-ID:  479229d1607b6051abc4f9c2ccbb65338f030c65
Gitweb:     https://git.kernel.org/tip/479229d1607b6051abc4f9c2ccbb65338f030c65
Author:     Maciej S. Szmigiero <mail@maciej.szmigiero.name>
AuthorDate: Tue, 19 Jun 2018 20:47:31 +0200
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Mon, 19 Nov 2018 10:44:59 +0100

x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

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().

 [ bp: Split comment. ]

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: x86-ml <x86@kernel.org>
Link: http://lkml.kernel.org/r/6df43f4f6a28186a13a66e8d7e61143c5e1a2324.1529424596.git.mail@maciej.szmigiero.name
---
 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 07b5fc00b188..b44c9029860c 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;
 
@@ -621,7 +625,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;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [tip:x86/microcode] x86/microcode/AMD: Add microcode container verification
  2018-06-19 18:47 ` [PATCH v7 2/9] x86/microcode/AMD: Add microcode container data checking functions Maciej S. Szmigiero
@ 2018-11-19 10:13   ` tip-bot for Maciej S. Szmigiero
  0 siblings, 0 replies; 17+ messages in thread
From: tip-bot for Maciej S. Szmigiero @ 2018-11-19 10:13 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: mail, linux-kernel, hpa, x86, tglx, bp, mingo

Commit-ID:  f4ff25916c1186912e1b53c25861c8abc9f15687
Gitweb:     https://git.kernel.org/tip/f4ff25916c1186912e1b53c25861c8abc9f15687
Author:     Maciej S. Szmigiero <mail@maciej.szmigiero.name>
AuthorDate: Tue, 19 Jun 2018 20:47:32 +0200
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Mon, 19 Nov 2018 10:45:03 +0100

x86/microcode/AMD: Add microcode container verification

Add container and patch verification 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.

 [ bp: Make all pr_err() calls into pr_debug() and drop the
   verify_patch() bits. ]

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: x86-ml <x86@kernel.org>
Link: http://lkml.kernel.org/r/3014e96c82cd90761b4601bd2cfe59c4119e46a7.1529424596.git.mail@maciej.szmigiero.name
---
 arch/x86/kernel/cpu/microcode/amd.c | 101 ++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index b44c9029860c..36aeb01b3ae8 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -73,6 +73,107 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 	return 0;
 }
 
+/*
+ * Check whether there is a valid microcode container file at the beginning
+ * of @buf of size @buf_size. Set @early to use this function in the early path.
+ */
+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_debug("Truncated microcode container header.\n");
+
+		return false;
+	}
+
+	cont_magic = *(const u32 *)buf;
+	if (cont_magic != UCODE_MAGIC) {
+		if (!early)
+			pr_debug("Invalid magic value (0x%08x).\n", cont_magic);
+
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * Check whether there is a valid, non-truncated CPU equivalence table at the
+ * beginning of @buf of size @buf_size. Set @early to use this function in the
+ * early path.
+ */
+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_debug("Wrong microcode container equivalence table type: %u.\n",
+			       cont_type);
+
+		return false;
+	}
+
+	buf_size -= CONTAINER_HDR_SZ;
+
+	equiv_tbl_len = hdr[2];
+	if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
+	    buf_size < equiv_tbl_len) {
+		if (!early)
+			pr_debug("Truncated equivalence table.\n");
+
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * Check whether there is a valid, non-truncated microcode patch section at the
+ * beginning of @buf of size @buf_size. Set @early to use this function in the
+ * early path.
+ */
+static bool verify_patch_section(const u8 *buf, size_t buf_size, bool early)
+{
+	const u32 *hdr;
+	u32 patch_type, patch_size;
+
+	if (buf_size < SECTION_HDR_SIZE) {
+		if (!early)
+			pr_debug("Truncated patch section.\n");
+
+		return false;
+	}
+
+	hdr = (const u32 *)buf;
+	patch_type = hdr[0];
+	patch_size = hdr[1];
+
+	if (patch_type != UCODE_UCODE_TYPE) {
+		if (!early)
+			pr_debug("Invalid type field (0x%x) in container file section header.\n",
+				patch_type);
+
+		return false;
+	}
+
+	if (patch_size < sizeof(struct microcode_header_amd)) {
+		if (!early)
+			pr_debug("Patch of size %u too short.\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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-11-19 10:13 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-19 18:47 [PATCH v7 0/9] x86/microcode/AMD: Check microcode file sanity before loading it Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 1/9] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length Maciej S. Szmigiero
2018-11-19 10:12   ` [tip:x86/microcode] " tip-bot for Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 2/9] x86/microcode/AMD: Add microcode container data checking functions Maciej S. Szmigiero
2018-11-19 10:13   ` [tip:x86/microcode] x86/microcode/AMD: Add microcode container verification tip-bot for Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch() Maciej S. Szmigiero
2018-06-21  8:36   ` Borislav Petkov
2018-06-22 22:32     ` [PATCH 1/2] " Maciej S. Szmigiero
2018-06-22 22:33     ` [PATCH 2/2] x86/microcode/AMD: Check patch size for all known CPU families Maciej S. Szmigiero
2018-06-25 18:37       ` Borislav Petkov
2018-06-25 22:18         ` Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 4/9] x86/microcode/AMD: Check microcode container data in the early loader Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 5/9] x86/microcode/AMD: Split status from data to skip in verify_and_add_patch() Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 6/9] x86/microcode/AMD: Check microcode container data in the late loader Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 7/9] x86/microcode/AMD: Add a reminder about PATCH_MAX_SIZE macro Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 8/9] x86/microcode/AMD: Convert CPU equivalence table variable into a struct Maciej S. Szmigiero
2018-06-19 18:47 ` [PATCH v7 9/9] x86/microcode/AMD: Check the equivalence table size when scanning it Maciej S. Szmigiero

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.