[PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[Maciej S. Szmigiero]

verify_patch_size() function verifies whether the microcode container file
remaining size 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>
---
 arch/x86/kernel/cpu/microcode/amd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index a998e1a7d46f..6a93be0f771c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -613,7 +613,16 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
 		return crnt_size;
 	}
 
-	ret = verify_patch_size(family, patch_size, leftover);
+	/*
+	 * verify_patch_size() checks whether the passed remaining file size
+	 * is large enough to contain a patch of the indicated size (and also
+	 * whether this size does not exceed the family maximum).
+	 *
+	 * The section header length is not included in this indicated size
+	 * but it is present in the leftover file length so we need to subtract
+	 * it before passing this value to that function.
+	 */
+	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;

Re: [PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[Borislav Petkov]

On Fri, Mar 16, 2018 at 12:07:42AM +0100, Maciej S. Szmigiero wrote:
> verify_patch_size() function verifies whether the microcode container file
> remaining size 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>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

I split the comment and applied this:

---
From: "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
Date: Fri, 16 Mar 2018 00:07:42 +0100
Subject: [PATCH] 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().

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 48179928ff38..ffce949409cc 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;
-- 
2.7.3

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[Maciej S. Szmigiero]

On 18.03.2018 17:12, Borislav Petkov wrote:
> On Fri, Mar 16, 2018 at 12:07:42AM +0100, Maciej S. Szmigiero wrote:
>> verify_patch_size() function verifies whether the microcode container file
>> remaining size 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>
>> ---
>>  arch/x86/kernel/cpu/microcode/amd.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> I split the comment and applied this:
> 
> ---
> From: "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
> Date: Fri, 16 Mar 2018 00:07:42 +0100
> Subject: [PATCH] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file
>  leftover length

Can't really find this commit in any tree I have looked at (bp.git and
tip.git at kernel.org).
Was it pushed somewhere else?

Maciej

Re: [PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[Borislav Petkov]

On Wed, Apr 18, 2018 at 02:39:27PM +0200, Maciej S. Szmigiero wrote:
> Can't really find this commit in any tree I have looked at (bp.git and
> tip.git at kernel.org).
> Was it pushed somewhere else?

No. Still waiting for the rest.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[Maciej S. Szmigiero]

On 18.04.2018 15:53, Borislav Petkov wrote:
> On Wed, Apr 18, 2018 at 02:39:27PM +0200, Maciej S. Szmigiero wrote:
>> Can't really find this commit in any tree I have looked at (bp.git and
>> tip.git at kernel.org).
>> Was it pushed somewhere else?
> 
> No. Still waiting for the rest.
> 

So, should this patch be included in a respin or not?

Maciej

Re: [PATCH v4 01/10] x86/microcode/AMD: Subtract SECTION_HDR_SIZE from file leftover length

[Borislav Petkov]

On Wed, Apr 18, 2018 at 03:57:23PM +0200, Maciej S. Szmigiero wrote:
> So, should this patch be included in a respin or not?

Yes please. Do them all ontop of tip/master.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4 02/10] x86/microcode/AMD: Check equivalence table length in the early loader

[Maciej S. Szmigiero]

Before loading a CPU equivalence table from a microcode container file we
need to verify whether this file is actually large enough to contain the
table of a size indicated in this file.
If it is not, there is no point of continuing with loading it since
microcode patches are located after the equivalence table anyway.

This patch adds these checks to the early loader.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6a93be0f771c..138c9fb983f2 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -80,20 +80,33 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
  * 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;
+	unsigned int cont_magic, cont_type;
+	size_t equiv_tbl_len;
 	u16 eq_id;
 	u8 *buf;
 
+	if (size < CONTAINER_HDR_SZ)
+		return 0;
+
+	cont_magic	= hdr[0];
+	cont_type	= hdr[1];
+	equiv_tbl_len	= hdr[2];
+
 	/* Am I looking at an equivalence table header? */
-	if (hdr[0] != UCODE_MAGIC ||
-	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
-	    hdr[2] == 0)
+	if (cont_magic != UCODE_MAGIC ||
+	    cont_type != UCODE_EQUIV_CPU_TABLE_TYPE ||
+	    equiv_tbl_len == 0)
 		return CONTAINER_HDR_SZ;
 
+	if (equiv_tbl_len < sizeof(*eq) ||
+	    size - CONTAINER_HDR_SZ < equiv_tbl_len)
+		return size;
+
 	buf = ucode;
 
 	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
@@ -101,8 +114,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 	/* 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  += equiv_tbl_len + CONTAINER_HDR_SZ;
+	size -= equiv_tbl_len + CONTAINER_HDR_SZ;
 
 	/*
 	 * Scan through the rest of the container to find where it ends. We do
@@ -159,15 +172,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;
 	}
 }
 

Re: [PATCH v4 02/10] x86/microcode/AMD: Check equivalence table length in the early loader

[Borislav Petkov]

On Fri, Mar 16, 2018 at 12:07:50AM +0100, Maciej S. Szmigiero wrote:
> Before loading a CPU equivalence table from a microcode container file we
> need to verify whether this file is actually large enough to contain the
> table of a size indicated in this file.
> If it is not, there is no point of continuing with loading it since
> microcode patches are located after the equivalence table anyway.
> 
> This patch adds these checks to the early loader.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 6a93be0f771c..138c9fb983f2 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -80,20 +80,33 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
>   * 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;
> +	unsigned int cont_magic, cont_type;
> +	size_t equiv_tbl_len;
>  	u16 eq_id;
>  	u8 *buf;
>  
> +	if (size < CONTAINER_HDR_SZ)
> +		return 0;
> +
> +	cont_magic	= hdr[0];
> +	cont_type	= hdr[1];
> +	equiv_tbl_len	= hdr[2];

All three are u32.

>  	/* Am I looking at an equivalence table header? */
> -	if (hdr[0] != UCODE_MAGIC ||
> -	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
> -	    hdr[2] == 0)
> +	if (cont_magic != UCODE_MAGIC ||
> +	    cont_type != UCODE_EQUIV_CPU_TABLE_TYPE ||
> +	    equiv_tbl_len == 0)
>  		return CONTAINER_HDR_SZ;
>  
> +	if (equiv_tbl_len < sizeof(*eq) ||

If you do this, then the above == 0 check can go.

> +	    size - CONTAINER_HDR_SZ < equiv_tbl_len)
> +		return size;
> +
>  	buf = ucode;
>  
>  	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4 03/10] x86/microcode/AMD: Check equivalence table length in the late loader

[Maciej S. Szmigiero]

Before loading a CPU equivalence table from a microcode container file we
need to verify whether this file is actually large enough to contain the
table of a size indicated in this file.
If it is not, there is no point of continuing with loading it since
microcode patches are located after the equivalence table anyway.

This patch adds these checks to the late loader.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 138c9fb983f2..ed24200cf936 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -551,28 +551,40 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	return UCODE_UPDATED;
 }
 
-static int install_equiv_cpu_table(const u8 *buf)
+static 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];
+	unsigned int type, equiv_tbl_len;
 
-	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
-		pr_err("empty section/"
-		       "invalid type field in container file section header\n");
+	if (buf_size <= CONTAINER_HDR_SZ) {
+		pr_err("Truncated microcode container header.\n");
 		return -EINVAL;
 	}
 
-	equiv_cpu_table = vmalloc(size);
+	type = ibuf[1];
+	if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
+		pr_err("Wrong microcode container equivalence table type: %u.\n",
+		       type);
+		return -EINVAL;
+	}
+
+	equiv_tbl_len = ibuf[2];
+	if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
+	    buf_size - CONTAINER_HDR_SZ < equiv_tbl_len) {
+		pr_err("Truncated equivalence table.\n");
+		return -EINVAL;
+	}
+
+	equiv_cpu_table = vmalloc(equiv_tbl_len);
 	if (!equiv_cpu_table) {
 		pr_err("failed to allocate equivalent CPU table\n");
 		return -ENOMEM;
 	}
 
-	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 + CONTAINER_HDR_SZ;
 }
 
 static void free_equiv_cpu_table(void)
@@ -674,7 +686,7 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 	int crnt_size = 0;
 	int offset;
 
-	offset = install_equiv_cpu_table(data);
+	offset = install_equiv_cpu_table(data, size);
 	if (offset < 0) {
 		pr_err("failed to create equivalent cpu table\n");
 		return ret;

Re: [PATCH v4 03/10] x86/microcode/AMD: Check equivalence table length in the late loader

[Borislav Petkov]

On Fri, Mar 16, 2018 at 12:08:04AM +0100, Maciej S. Szmigiero wrote:
> Before loading a CPU equivalence table from a microcode container file we
> need to verify whether this file is actually large enough to contain the
> table of a size indicated in this file.
> If it is not, there is no point of continuing with loading it since
> microcode patches are located after the equivalence table anyway.
> 
> This patch adds these checks to the late loader.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 138c9fb983f2..ed24200cf936 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -551,28 +551,40 @@ static enum ucode_state apply_microcode_amd(int cpu)
>  	return UCODE_UPDATED;
>  }
>  
> -static int install_equiv_cpu_table(const u8 *buf)
> +static 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];
> +	unsigned int type, equiv_tbl_len;

Ok, as a unification, let's make those u32 too.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4 04/10] x86/microcode/AMD: install_equiv_cpu_table() should not return a signed int

[Maciej S. Szmigiero]

The maximum possible value returned by install_equiv_cpu_table() is
UINT_MAX (on a 64-bit kernel).
This is more than (signed) int type currently returned by this function can
hold so this function will need to return an unsigned int instead.

In order to avoid an overflow of this type on a 64-bit kernel we'll need to
also modify this function to return only the CPU equivalence table length
(without the container header length), leaving its single caller
(__load_microcode_amd()) the job of adding the container header length to
skip to the fist patch section.

The individual (negative) error codes returned by install_equiv_cpu_table()
are of no use anyway, since they are all normalized to UCODE_ERROR by its
caller.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index ed24200cf936..8e8df37f2f1b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -551,40 +551,39 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	return UCODE_UPDATED;
 }
 
-static int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
+static unsigned int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 {
 	unsigned int *ibuf = (unsigned int *)buf;
 	unsigned int type, equiv_tbl_len;
 
 	if (buf_size <= CONTAINER_HDR_SZ) {
 		pr_err("Truncated microcode container header.\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	type = ibuf[1];
 	if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
 		pr_err("Wrong microcode container equivalence table type: %u.\n",
 		       type);
-		return -EINVAL;
+		return 0;
 	}
 
 	equiv_tbl_len = ibuf[2];
 	if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
 	    buf_size - CONTAINER_HDR_SZ < equiv_tbl_len) {
 		pr_err("Truncated equivalence table.\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	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, equiv_tbl_len);
 
-	/* add header length */
-	return equiv_tbl_len + CONTAINER_HDR_SZ;
+	return equiv_tbl_len;
 }
 
 static void free_equiv_cpu_table(void)
@@ -681,18 +680,24 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 					     size_t size)
 {
 	enum ucode_state ret = UCODE_ERROR;
-	unsigned int leftover;
+	size_t leftover;
 	u8 *fw = (u8 *)data;
 	int crnt_size = 0;
-	int offset;
+	unsigned int offset;
 
 	offset = install_equiv_cpu_table(data, size);
-	if (offset < 0) {
+	if (!offset) {
 		pr_err("failed to create equivalent cpu table\n");
 		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 - offset;
+	leftover = size - CONTAINER_HDR_SZ - offset;
 
 	if (*(u32 *)fw != UCODE_UCODE_TYPE) {
 		pr_err("invalid type field in container file section header\n");

[PATCH v4 05/10] 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 8e8df37f2f1b..eba9e3c8aa17 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -477,6 +477,10 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
 {
 	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 v4 06/10] x86/microcode/AMD: Check patch size in verify_and_add_patch()

[Maciej S. Szmigiero]

We should verify the indicated size of a patch in a microcode container
file (whether it does not exceed the PATCH_MAX_SIZE value) and also whether
this file is actually large enough to contain it in the late loader
verify_and_add_patch() function.

The early loader already does the PATCH_MAX_SIZE check in parse_container()
function.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index eba9e3c8aa17..096cb58a563f 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -473,7 +473,7 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 }
 
 static unsigned int verify_patch_size(u8 family, u32 patch_size,
-				      unsigned int size)
+				      size_t size)
 {
 	u32 max_size;
 
@@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
 		break;
 	}
 
-	if (patch_size > min_t(u32, size, max_size)) {
+	if (patch_size > min_t(size_t, size, max_size)) {
 		pr_err("patch size mismatch\n");
 		return 0;
 	}
@@ -609,7 +609,7 @@ static void cleanup(void)
  * 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, size_t leftover)
 {
 	struct microcode_header_amd *mc_hdr;
 	struct ucode_patch *patch;
@@ -617,7 +617,15 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
 	u32 proc_fam;
 	u16 proc_id;
 
+	if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
+		return leftover;
+
 	patch_size  = *(u32 *)(fw + 4);
+	if (patch_size > PATCH_MAX_SIZE) {
+		pr_err("patch size %u too large\n", patch_size);
+		return -EINVAL;
+	}
+
 	crnt_size   = patch_size + SECTION_HDR_SIZE;
 	mc_hdr	    = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
 	proc_id	    = mc_hdr->processor_rev_id;

Re: [PATCH v4 06/10] x86/microcode/AMD: Check patch size in verify_and_add_patch()

[Borislav Petkov]

On Fri, Mar 16, 2018 at 12:08:17AM +0100, Maciej S. Szmigiero wrote:
> @@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
>  		break;
>  	}
>  
> -	if (patch_size > min_t(u32, size, max_size)) {
> +	if (patch_size > min_t(size_t, size, max_size)) {

So I don't like this conversion to 8-byte-width size_t's. It is not
necessary. I'm pretty sure we can do fine with signed and unsigned ints.

For example, you can convert the size to signed int (if it hasn't been
converted yet) and check for < 0 and stop further processing. And so on...

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 06/10] x86/microcode/AMD: Check patch size in verify_and_add_patch()

[Maciej S. Szmigiero]

On 22.03.2018 17:11, Borislav Petkov wrote:
> On Fri, Mar 16, 2018 at 12:08:17AM +0100, Maciej S. Szmigiero wrote:
>> @@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
>>  		break;
>>  	}
>>  
>> -	if (patch_size > min_t(u32, size, max_size)) {
>> +	if (patch_size > min_t(size_t, size, max_size)) {
> 
> So I don't like this conversion to 8-byte-width size_t's. It is not
> necessary. I'm pretty sure we can do fine with signed and unsigned ints.

It is possible to keep verify_patch_size() unmodified (with unsigned int
and u32) but microcode container files >4GB in size then may be rejected,
even if they are technically valid (while a bit unrealistic) on 64-bit
kernels.

Thanks,
Maciej

Re: [PATCH v4 06/10] x86/microcode/AMD: Check patch size in verify_and_add_patch()

[Boris Petkov]

On March 23, 2018 9:40:50 AM CDT, "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> wrote:
>It is possible to keep verify_patch_size() unmodified (with unsigned
>int
>and u32) but microcode container files >4GB in size then may be
>rejected,

If we ever have to support that - which I'm pretty sure we won't - more changes to the container format and parsing will be needed anyway.

Thx.

-- 
Sent from a small device: formatting sux and brevity is inevitable. 

[PATCH v4 07/10] x86/microcode/AMD: Verify patch section type for every such section

[Maciej S. Szmigiero]

We should check whether the patch section currently being processed is
actually a patch section for each of them (not just the first one) in the
late loader verify_and_add_patch() function, just like the early loader
already does in parse_container() function.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 096cb58a563f..4d2116d08754 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -613,13 +613,19 @@ static int verify_and_add_patch(u8 family, u8 *fw, size_t leftover)
 {
 	struct microcode_header_amd *mc_hdr;
 	struct ucode_patch *patch;
-	unsigned int patch_size, crnt_size, ret;
+	unsigned int patch_type, patch_size, crnt_size, ret;
 	u32 proc_fam;
 	u16 proc_id;
 
 	if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
 		return leftover;
 
+	patch_type = *(u32 *)fw;
+	if (patch_type != UCODE_UCODE_TYPE) {
+		pr_err("invalid type field in container file section header\n");
+		return -EINVAL;
+	}
+
 	patch_size  = *(u32 *)(fw + 4);
 	if (patch_size > PATCH_MAX_SIZE) {
 		pr_err("patch size %u too large\n", patch_size);
@@ -711,12 +717,6 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 	fw += offset;
 	leftover = size - CONTAINER_HDR_SZ - offset;
 
-	if (*(u32 *)fw != UCODE_UCODE_TYPE) {
-		pr_err("invalid type field in container file section header\n");
-		free_equiv_cpu_table();
-		return ret;
-	}
-
 	while (leftover) {
 		crnt_size = verify_and_add_patch(family, fw, leftover);
 		if (crnt_size < 0)

[PATCH v4 08/10] x86/microcode/AMD: Check microcode container file size before accessing it

[Maciej S. Szmigiero]

The early loader parse_container() function should check whether the
microcode container file is actually large enough to contain the patch of
an indicated size, just like the late loader does.

Also, the request_microcode_amd() function should check whether the
container file is actually large enough to contain the header magic value.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 4d2116d08754..dc5ed4971879 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -125,6 +125,9 @@ static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
 		struct microcode_amd *mc;
 		u32 patch_size;
 
+		if (size < SECTION_HDR_SIZE)
+			break;
+
 		hdr = (u32 *)buf;
 
 		if (hdr[0] != UCODE_UCODE_TYPE)
@@ -139,6 +142,10 @@ static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
 		buf  += SECTION_HDR_SIZE;
 		size -= SECTION_HDR_SIZE;
 
+		if (size < sizeof(*mc) ||
+		    size < patch_size)
+			break;
+
 		mc = (struct microcode_amd *)buf;
 		if (eq_id == mc->hdr.processor_rev_id) {
 			desc->psize = patch_size;
@@ -794,6 +801,10 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	}
 
 	ret = UCODE_ERROR;
+	if (fw->size < sizeof(u32)) {
+		pr_err("microcode container far too short\n");
+		goto fw_release;
+	}
 	if (*(u32 *)fw->data != UCODE_MAGIC) {
 		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
 		goto fw_release;

Re: [PATCH v4 08/10] x86/microcode/AMD: Check microcode container file size before accessing it

[Borislav Petkov]

On Fri, Mar 16, 2018 at 12:08:24AM +0100, Maciej S. Szmigiero wrote:
> The early loader parse_container() function should check whether the
> microcode container file is actually large enough to contain the patch of
> an indicated size, just like the late loader does.
> 
> Also, the request_microcode_amd() function should check whether the
> container file is actually large enough to contain the header magic value.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 4d2116d08754..dc5ed4971879 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -125,6 +125,9 @@ static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
>  		struct microcode_amd *mc;
>  		u32 patch_size;
>  
> +		if (size < SECTION_HDR_SIZE)
> +			break;
> +
>  		hdr = (u32 *)buf;
>  
>  		if (hdr[0] != UCODE_UCODE_TYPE)
> @@ -139,6 +142,10 @@ static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
>  		buf  += SECTION_HDR_SIZE;
>  		size -= SECTION_HDR_SIZE;
>  
> +		if (size < sizeof(*mc) ||
> +		    size < patch_size)
> +			break;

If you're going to do this here, then call verify_patch_size() but move
the pr_err("patch size mismatch\n") outside of the function because
printk doesn't work that early.

> +
>  		mc = (struct microcode_amd *)buf;
>  		if (eq_id == mc->hdr.processor_rev_id) {
>  			desc->psize = patch_size;
> @@ -794,6 +801,10 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
>  	}
>  
>  	ret = UCODE_ERROR;
> +	if (fw->size < sizeof(u32)) {
> +		pr_err("microcode container far too short\n");
> +		goto fw_release;
> +	}

Instead of doing that here, do the SECTION_HDR_SIZE check above here
directly.

In general, the code is getting interspersed with a lot of checks and
thus becoming unreadable. So instead of doing that, I'd suggest you add
functions doing that checking separately:

verify_container()
verify_equivalence_table()
verify_patch()

and you call those functions in both paths, first when you get a
container, you do verify_container(), then you verify the equivalence
table and then you verify each patch one after the other. And so on.

The early path will not printk because it is too early but you can state
that with a "bool early" argument to those functions.

This way you'll pull all that checking before the code looks at the
binary data and the paths will remain unencumbered by the checking code.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4 09/10] 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 dc5ed4971879..6e25a63a0a3d 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;
 }
@@ -112,7 +119,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  += equiv_tbl_len + CONTAINER_HDR_SZ;
 	size -= equiv_tbl_len + CONTAINER_HDR_SZ;
@@ -382,20 +390,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;
 }
 
@@ -593,6 +602,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;
 }
@@ -601,6 +611,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)

[PATCH v4 10/10] x86/microcode/AMD: Be more tolerant of late parse failures in late loader

[Maciej S. Szmigiero]

The early loader just ends its microcode container file processing when it
is unable to parse some patch section, but keeps the already read patches
from this file for their eventual application.

We can do the same in the late loader - we'll just return an error if we
are unable to parse any patches.
Note that we already do silently skip patches in the late loader for
smaller issues like lack of an equivalence table entry, family-size
mismatch or an unsupported chipset match type.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6e25a63a0a3d..f9278d9a2f9b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -738,13 +738,15 @@ 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)
-			return ret;
+			break;
 
 		fw	 += crnt_size;
 		leftover -= crnt_size;
+
+		ret = UCODE_OK;
 	}
 
-	return UCODE_OK;
+	return ret;
 }
 
 static enum ucode_state