[PATCH v3 1/9] x86/microcode/AMD: subtract SECTION_HDR_SIZE from file leftover length

[PATCH v3 1/9] 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index a998e1a7d46f..ffe0d0ce57fc 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -613,7 +613,8 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
 		return crnt_size;
 	}
 
-	ret = verify_patch_size(family, patch_size, leftover);
+	ret = verify_patch_size(family, patch_size,
+				leftover - SECTION_HDR_SIZE);
 	if (!ret) {
 		pr_err("Patch-ID 0x%08x: size mismatch.\n", mc_hdr->patch_id);
 		return crnt_size;

Re: [PATCH v3 1/9] x86/microcode/AMD: subtract SECTION_HDR_SIZE from file leftover length

[Borislav Petkov]

On Tue, Mar 13, 2018 at 10:06:10PM +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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index a998e1a7d46f..ffe0d0ce57fc 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -613,7 +613,8 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
>  		return crnt_size;
>  	}
>  
> -	ret = verify_patch_size(family, patch_size, leftover);
> +	ret = verify_patch_size(family, patch_size,
> +				leftover - SECTION_HDR_SIZE);

Pls add a sentence or two as a comment above it explaining why the
section header size needs to be subtracted and do not break the line
even if it is > 80 cols.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v3 2/9] x86/microcode/AMD: check whether the equivalence table fits in the file

[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.

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index ffe0d0ce57fc..ac06e2819f26 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -80,20 +80,29 @@ 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;
+	size_t eq_size;
 	u16 eq_id;
 	u8 *buf;
 
 	/* Am I looking at an equivalence table header? */
+	if (size < CONTAINER_HDR_SZ)
+		return 0;
+
 	if (hdr[0] != UCODE_MAGIC ||
 	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
 	    hdr[2] == 0)
 		return CONTAINER_HDR_SZ;
 
+	eq_size = hdr[2];
+	if (eq_size < sizeof(*eq) ||
+	    size - CONTAINER_HDR_SZ < eq_size)
+		return 0;
+
 	buf = ucode;
 
 	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
@@ -101,8 +110,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  += eq_size + CONTAINER_HDR_SZ;
+	size -= eq_size + CONTAINER_HDR_SZ;
 
 	/*
 	 * Scan through the rest of the container to find where it ends. We do
@@ -159,15 +168,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;
 	}
 }
 
@@ -540,15 +547,30 @@ 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, size;
+
+	if (buf_size < CONTAINER_HDR_SZ) {
+		pr_err("no container header\n");
+		return -EINVAL;
+	}
+
+	type = ibuf[1];
+	if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
+		pr_err("invalid type field in container file section header\n");
+		return -EINVAL;
+	}
+
+	size = ibuf[2];
+	if (size < sizeof(struct equiv_cpu_entry)) {
+		pr_err("equivalent CPU table too short\n");
+		return -EINVAL;
+	}
 
-	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 < size) {
+		pr_err("equivalent CPU table truncated\n");
 		return -EINVAL;
 	}
 
@@ -655,7 +677,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 v3 2/9] x86/microcode/AMD: check whether the equivalence table fits in the file

[Borislav Petkov]

On Tue, Mar 13, 2018 at 10:06:23PM +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.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 54 ++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index ffe0d0ce57fc..ac06e2819f26 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -80,20 +80,29 @@ 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;
> +	size_t eq_size;
>  	u16 eq_id;
>  	u8 *buf;
>  
>  	/* Am I looking at an equivalence table header? */

That comment becomes wrong when you add this check here.

> +	if (size < CONTAINER_HDR_SZ)
> +		return 0;
> +
>  	if (hdr[0] != UCODE_MAGIC ||
>  	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
>  	    hdr[2] == 0)
>  		return CONTAINER_HDR_SZ;
>  
> +	eq_size = hdr[2];

If we're going to have special local vars for the container header, then
do it right:

	cont_magic	= hdr[0];
	cont_type  	= hdr[1];
	equiv_tbl_len	= hdr[2];

and then use those from now on.

> +	if (eq_size < sizeof(*eq) ||
> +	    size - CONTAINER_HDR_SZ < eq_size)
> +		return 0;

I think you want

	if (size < eqiv_tbl_len + CONTAINER_HDR_SZ)
		return size;

here to skip over the next, hopefully not truncated container.

> +
>  	buf = ucode;
>  
>  	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
> @@ -101,8 +110,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  += eq_size + CONTAINER_HDR_SZ;
> +	size -= eq_size + CONTAINER_HDR_SZ;
>  
>  	/*
>  	 * Scan through the rest of the container to find where it ends. We do
> @@ -159,15 +168,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;
>  	}
>  }
>  

All changes upto here need to be a separate patch.
install_equiv_cpu_table() changes below are the second patch.

> @@ -540,15 +547,30 @@ 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, size;

unsigned int type, equiv_tbl_len;

> +
> +	if (buf_size < CONTAINER_HDR_SZ) {

		     <= is ok too.

> +		pr_err("no container header\n");

More descriptive error messages:

			"Truncated microcode container header.\n"

> +		return -EINVAL;
> +	}
> +
> +	type = ibuf[1];
> +	if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
> +		pr_err("invalid type field in container file section header\n");

			"Wrong microcode container equivalence table type: %d.\n"

> +		return -EINVAL;
> +	}
> +
> +	size = ibuf[2];
> +	if (size < sizeof(struct equiv_cpu_entry)) {
> +		pr_err("equivalent CPU table too short\n");

			"Truncated equivalence table.\n"

> +		return -EINVAL;
> +	}
>  
> -	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 < size) {
> +		pr_err("equivalent CPU table truncated\n");

Combine that test with the above one and use the same error message.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 2/9] x86/microcode/AMD: check whether the equivalence table fits in the file

[Maciej S. Szmigiero]

On 14.03.2018 18:04, Borislav Petkov wrote:
> On Tue, Mar 13, 2018 at 10:06:23PM +0100, Maciej S. Szmigiero wrote:
(..)
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -80,20 +80,29 @@ 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;
>> +	size_t eq_size;
>>  	u16 eq_id;
>>  	u8 *buf;
>>  
>>  	/* Am I looking at an equivalence table header? */
> 
> That comment becomes wrong when you add this check here.

It was moved there because on the first (monolithic) iteration of this
change there was a review comment that it better belongs here.

No problem to move it back, however.

>> +	if (size < CONTAINER_HDR_SZ)
>> +		return 0;
>> +
>>  	if (hdr[0] != UCODE_MAGIC ||
>>  	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
>>  	    hdr[2] == 0)
>>  		return CONTAINER_HDR_SZ;
>>  
>> +	eq_size = hdr[2];
> 
> If we're going to have special local vars for the container header, then
> do it right:
> 
> 	cont_magic	= hdr[0];
> 	cont_type  	= hdr[1];
> 	equiv_tbl_len	= hdr[2];
> 
> and then use those from now on.

Will do.

>> +	if (eq_size < sizeof(*eq) ||
>> +	    size - CONTAINER_HDR_SZ < eq_size)
>> +		return 0;
> 
> I think you want
> 
> 	if (size < eqiv_tbl_len + CONTAINER_HDR_SZ)
> 		return size;
> 
> here to skip over the next, hopefully not truncated container.

'size' here is the length of the whole CPIO blob containing all
containers combined (well, the remaining part of it).

If we skip over 'size' bytes we'll have nothing left to parse.

>> @@ -159,15 +168,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;
>>  	}
>>  }
>>  
> 
> All changes upto here need to be a separate patch.
> install_equiv_cpu_table() changes below are the second patch.

OK, will split this into two patches.

>> @@ -540,15 +547,30 @@ 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, size;
> 
> unsigned int type, equiv_tbl_len;

Will do.

>> +
>> +	if (buf_size < CONTAINER_HDR_SZ) {
> 
> 		     <= is ok too.

Will do.

>> +		pr_err("no container header\n");
> 
> More descriptive error messages:
> 
> 			"Truncated microcode container header.\n"

Will do.

>> +		return -EINVAL;
>> +	}
>> +
>> +	type = ibuf[1];
>> +	if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
>> +		pr_err("invalid type field in container file section header\n");
> 
> 			"Wrong microcode container equivalence table type: %d.\n"

Will do.

>> +		return -EINVAL;
>> +	}
>> +
>> +	size = ibuf[2];
>> +	if (size < sizeof(struct equiv_cpu_entry)) {
>> +		pr_err("equivalent CPU table too short\n");
> 
> 			"Truncated equivalence table.\n"

Will do.

>> +		return -EINVAL;
>> +	}
>>  
>> -	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 < size) {
>> +		pr_err("equivalent CPU table truncated\n");
> 
> Combine that test with the above one and use the same error message.

Will do.
 
> Thx.
> 

Thanks,
Maciej

Re: [PATCH v3 2/9] x86/microcode/AMD: check whether the equivalence table fits in the file

[Borislav Petkov]

On Thu, Mar 15, 2018 at 12:34:09AM +0100, Maciej S. Szmigiero wrote:
> 'size' here is the length of the whole CPIO blob containing all
> containers combined (well, the remaining part of it).
> 
> If we skip over 'size' bytes we'll have nothing left to parse.

Well, if

	size < eqiv_tbl_len + CONTAINER_HDR_SZ

then you really have nothing else to parse.

Come to think of it, if the whole blob is truncated like that, we
shouldn't trust it at all and stop looking at it. So yes, "return size"
is the right thing to do but for a different reason.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int

[Maciej S. Szmigiero]

The maximum possible value returned by install_equiv_cpu_table() is
UINT_MAX + CONTAINER_HDR_SZ (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 a size_t instead.

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

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

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index ac06e2819f26..d20c327c960b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -547,37 +547,38 @@ 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 size_t install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 {
 	unsigned int *ibuf = (unsigned int *)buf;
-	unsigned int type, size;
+	unsigned int type;
+	size_t size;
 
 	if (buf_size < CONTAINER_HDR_SZ) {
 		pr_err("no container header\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	type = ibuf[1];
 	if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
 		pr_err("invalid type field in container file section header\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	size = ibuf[2];
 	if (size < sizeof(struct equiv_cpu_entry)) {
 		pr_err("equivalent CPU table too short\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	if (buf_size - CONTAINER_HDR_SZ < size) {
 		pr_err("equivalent CPU table truncated\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	equiv_cpu_table = vmalloc(size);
 	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);
@@ -672,13 +673,13 @@ 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;
+	size_t offset;
 
 	offset = install_equiv_cpu_table(data, size);
-	if (offset < 0) {
+	if (!offset) {
 		pr_err("failed to create equivalent cpu table\n");
 		return ret;
 	}

Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int

[Borislav Petkov]

On Tue, Mar 13, 2018 at 10:06:34PM +0100, Maciej S. Szmigiero wrote:
> The maximum possible value returned by install_equiv_cpu_table() is
> UINT_MAX + CONTAINER_HDR_SZ (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 a size_t instead.

I'm trying to parse this but I'm not really sure.

All I know is:

	unsigned int size = ibuf[2];

and that is really a 4-byte unsigned quantity so anything less is an
arbitrary limitation.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int

[Maciej S. Szmigiero]

On 14.03.2018 18:58, Borislav Petkov wrote:
> On Tue, Mar 13, 2018 at 10:06:34PM +0100, Maciej S. Szmigiero wrote:
>> The maximum possible value returned by install_equiv_cpu_table() is
>> UINT_MAX + CONTAINER_HDR_SZ (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 a size_t instead.
> 
> I'm trying to parse this but I'm not really sure.
> 
> All I know is:
> 
> 	unsigned int size = ibuf[2];
> 
> and that is really a 4-byte unsigned quantity so anything less is an
> arbitrary limitation.

There is no limit on CPU equivalence table length in this patch series
like it was in the previous version.

The maximum possible value returned by install_equiv_cpu_table() of
UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size'
variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ.
This won't fit in 'int' type, hence this patch.

That's because this functions tells its caller how much bytes to skip
from the beginning of a microcode container file to the first patch
section contained in it.

Maciej

Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int

[Borislav Petkov]

On Thu, Mar 15, 2018 at 12:46:05AM +0100, Maciej S. Szmigiero wrote:
> The maximum possible value returned by install_equiv_cpu_table() of
> UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size'
> variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ.
> This won't fit in 'int' type, hence this patch.

So make it fit by returning an unsigned int.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int

[Maciej S. Szmigiero]

On 15.03.2018 00:58, Borislav Petkov wrote:
> On Thu, Mar 15, 2018 at 12:46:05AM +0100, Maciej S. Szmigiero wrote:
>> The maximum possible value returned by install_equiv_cpu_table() of
>> UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size'
>> variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ.
>> This won't fit in 'int' type, hence this patch.
> 
> So make it fit by returning an unsigned int.
> 

This can be done if this function is modified to return only the CPU
equivalence table length (without the container header length), leaving
its single caller the job of adding the container header length to skip
to the fist patch section.

Otherwise we introduce a equivalence table length limit of
UINT_MAX - CONTAINER_HDR_SZ, as anything more will overflow an
unsigned int variable on a 64-bit kernel (on 32-bit this will be caught
by the equivalence table truncation check).

Maciej

Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int

[Borislav Petkov]

On Thu, Mar 15, 2018 at 01:13:07AM +0100, Maciej S. Szmigiero wrote:
> This can be done if this function is modified to return only the CPU
> equivalence table length (without the container header length), leaving
> its single caller the job of adding the container header length to skip
> to the fist patch section.

Sure, it leaves the function to deal with the equiv table length only
and the caller then adds the header length. Which is actually cleaner.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 3/9] x86/microcode/AMD: install_equiv_cpu_table() should not return (signed) int

[Maciej S. Szmigiero]

On 15.03.2018 01:56, Borislav Petkov wrote:
> On Thu, Mar 15, 2018 at 01:13:07AM +0100, Maciej S. Szmigiero wrote:
>> This can be done if this function is modified to return only the CPU
>> equivalence table length (without the container header length), leaving
>> its single caller the job of adding the container header length to skip
>> to the fist patch section.
> 
> Sure, it leaves the function to deal with the equiv table length only
> and the caller then adds the header length. Which is actually cleaner.
> 

OK, will do then.

Maciej

[PATCH v3 4/9] x86/microcode/AMD: automatically compute the PATCH_MAX_SIZE macro

[Maciej S. Szmigiero]

The PATCH_MAX_SIZE macro should contain the maximum of all family patch
sizes, computed automatically so that future changes there don't cause an
inconsistency.

Unfortunately, we can't use a standard max{,3} macros for this since they
only work inside a function (they use a compound statement as an
expression) and we have a static array using this macro value as its
length.

This macro is specific to amd.c file so let's move it there (the max sizes
for families are in this file already).

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

diff --git a/arch/x86/include/asm/microcode_amd.h b/arch/x86/include/asm/microcode_amd.h
index 209492849566..365bfa85a06b 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -41,8 +41,6 @@ struct microcode_amd {
 	unsigned int			mpb[0];
 };
 
-#define PATCH_MAX_SIZE PAGE_SIZE
-
 #ifdef CONFIG_MICROCODE_AMD
 extern void __init load_ucode_amd_bsp(unsigned int family);
 extern void load_ucode_amd_ap(unsigned int family);
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index d20c327c960b..b9f6c06bdc16 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -40,6 +40,22 @@
 
 static struct equiv_cpu_entry *equiv_cpu_table;
 
+/* Maximum patch size for a particular family */
+#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
+
+/* Can't use a standard max{,3} since they only work inside a function */
+#define SIMPLE_MAX(x, y) ((x) > (y) ? (x) : (y))
+#define SIMPLE_MAX3(x, y, z) SIMPLE_MAX(SIMPLE_MAX(x, y), z)
+
+/* Maximum of all the above families */
+#define PATCH_MAX_SIZE SIMPLE_MAX3(F1XH_MPB_MAX_SIZE, F14H_MPB_MAX_SIZE, \
+		       SIMPLE_MAX3(F15H_MPB_MAX_SIZE, F16H_MPB_MAX_SIZE, \
+				   F17H_MPB_MAX_SIZE))
+
 /*
  * This points to the current valid container of microcode patches which we will
  * save from the initrd/builtin before jettisoning its contents. @mc is the
@@ -473,12 +489,6 @@ static unsigned int verify_patch_size(u8 family, u32 patch_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;

Re: [PATCH v3 4/9] x86/microcode/AMD: automatically compute the PATCH_MAX_SIZE macro

[Borislav Petkov]

On Tue, Mar 13, 2018 at 10:06:48PM +0100, Maciej S. Szmigiero wrote:
> +/* Maximum of all the above families */
> +#define PATCH_MAX_SIZE SIMPLE_MAX3(F1XH_MPB_MAX_SIZE, F14H_MPB_MAX_SIZE, \

Nope, it should be

#define PATCH_MAX_SIZE (max_t(unsigned int, FXXH...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 4/9] x86/microcode/AMD: automatically compute the PATCH_MAX_SIZE macro

[Maciej S. Szmigiero]

On 14.03.2018 19:02, Borislav Petkov wrote:
> On Tue, Mar 13, 2018 at 10:06:48PM +0100, Maciej S. Szmigiero wrote:
>> +/* Maximum of all the above families */
>> +#define PATCH_MAX_SIZE SIMPLE_MAX3(F1XH_MPB_MAX_SIZE, F14H_MPB_MAX_SIZE, \
> 
> Nope, it should be
> 
> #define PATCH_MAX_SIZE (max_t(unsigned int, FXXH...

Unfortunately, this does not work:
> ./include/linux/kernel.h:806:41: error: braced-group within expression allowed only inside a function
>  #define __max(t1, t2, max1, max2, x, y) ({  \

That's because we have a static array containing the chosen microcode
patch for the current CPU (amd_ucode_patch) using this macro value as its
length.

Comments in the code say we can't use vmalloc() during early microcode
load so we can't allocate this array dynamically.

And if we hardcode its length we don't get the benefits of automatically
computing this length as maximum of all family patch sizes (as it should
be).

Maciej

Re: [PATCH v3 4/9] x86/microcode/AMD: automatically compute the PATCH_MAX_SIZE macro

[Borislav Petkov]

On Thu, Mar 15, 2018 at 01:05:14AM +0100, Maciej S. Szmigiero wrote:
> Unfortunately, this does not work:
> > ./include/linux/kernel.h:806:41: error: braced-group within expression allowed only inside a function
> >  #define __max(t1, t2, max1, max2, x, y) ({  \

Ok, let's drop this line then. It is simply too much monkey business
just to end up with max size of 4K which is F15H_MPB_MAX_SIZE,
coincidentally.

Just put a comment in verify_patch_size() stating that PATCH_MAX_SIZE
needs to be adjusted when a bigger per-family patch size gets added.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v3 5/9] x86/microcode/AMD: check patch size in verify_and_add_patch()

[Maciej S. Szmigiero]

Now that we have the PATCH_MAX_SIZE macro correctly computed we can verify
properly the indicated size of a patch in a microcode container file and
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 b9f6c06bdc16..0f78200f2f6c 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -485,7 +485,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;
 
@@ -507,7 +507,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;
 	}
@@ -616,7 +616,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;
@@ -624,7 +624,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;

[PATCH v3 6/9] 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 | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 0f78200f2f6c..3ad23e72c2b0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -627,6 +627,11 @@ static int verify_and_add_patch(u8 family, u8 *fw, size_t leftover)
 	if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
 		return leftover;
 
+	if (*(u32 *)fw != 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);
@@ -704,12 +709,6 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 	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;
-	}
-
 	while (leftover) {
 		crnt_size = verify_and_add_patch(family, fw, leftover);
 		if (crnt_size < 0)

Re: [PATCH v3 6/9] x86/microcode/AMD: verify patch section type for every such section

[Borislav Petkov]

On Tue, Mar 13, 2018 at 10:07:01PM +0100, Maciej S. Szmigiero wrote:
> 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 | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 0f78200f2f6c..3ad23e72c2b0 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -627,6 +627,11 @@ static int verify_and_add_patch(u8 family, u8 *fw, size_t leftover)
>  	if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
>  		return leftover;
>  
> +	if (*(u32 *)fw != UCODE_UCODE_TYPE) {

Store that in a local var patch_type for better readability, while
you're touching that.

Also, for all your patch subject names for your next submission, start
the patch subject sentence with a capital letter:

Subject: [PATCH v3 6/9] x86/microcode/AMD: Verify patch section type for every such section

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v3 7/9] 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 3ad23e72c2b0..63bd1a63f98a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -137,6 +137,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)
@@ -151,6 +154,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;
@@ -786,6 +793,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;

[PATCH v3 8/9] 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 | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 63bd1a63f98a..78e698fcd3ce 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;
 
 /* Maximum patch size for a particular family */
 #define F1XH_MPB_MAX_SIZE 2048
@@ -79,12 +80,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;
 }
@@ -124,7 +131,7 @@ 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, eq_size / sizeof(*eq), desc->cpuid_1_eax);
 
 	buf  += eq_size + CONTAINER_HDR_SZ;
 	size -= eq_size + CONTAINER_HDR_SZ;
@@ -394,20 +401,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;
 }
 
@@ -599,6 +607,7 @@ static size_t install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 	}
 
 	memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
+	equiv_cpu_table_entries = size / sizeof(struct equiv_cpu_entry);
 
 	/* add header length */
 	return size + CONTAINER_HDR_SZ;
@@ -608,6 +617,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 v3 9/9] 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 78e698fcd3ce..a098780b0847 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -729,13 +729,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