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.

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.

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

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

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

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

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