Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode file since it does very little
consistency checking on data loaded from such file.
This commit introduces various checks, mostly on length-type fields, so
all corrupted microcode files are (hopefully) correctly rejected instead.
Signed-off-by: Maciej S. Szmigiero <[email protected]>
---
Test files are at https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
arch/x86/include/asm/microcode_amd.h | 6 ++
arch/x86/kernel/cpu/microcode/amd.c | 111 ++++++++++++++++++++++++++---------
2 files changed, 89 insertions(+), 28 deletions(-)
diff --git a/arch/x86/include/asm/microcode_amd.h b/arch/x86/include/asm/microcode_amd.h
index 209492849566..e797d5d939d7 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -19,6 +19,9 @@ struct equiv_cpu_entry {
u16 res;
} __attribute__((packed));
+/* 4k */
+#define UCODE_EQUIV_CPU_TABLE_MAX_SIZE (256 * sizeof(struct equiv_cpu_entry))
+
struct microcode_header_amd {
u32 data_code;
u32 patch_id;
@@ -41,6 +44,9 @@ struct microcode_amd {
unsigned int mpb[0];
};
+/* if a patch is larger than this the microcode file is surely corrupted */
+#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)
+
#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 a998e1a7d46f..f798bd4004aa 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_size;
/*
* This points to the current valid container of microcode patches which we will
@@ -63,12 +64,17 @@ 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_size, 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_size && equiv_table[i].installed_cpu; i++)
+ if (sig == equiv_table[i].installed_cpu)
+ return equiv_table[i].equiv_cpu;
return 0;
}
@@ -80,29 +86,41 @@ 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;
+ u32 eqsize;
u16 eq_id;
u8 *buf;
+ if (size < CONTAINER_HDR_SZ)
+ return 0;
+
/* Am I looking at an equivalence table header? */
if (hdr[0] != UCODE_MAGIC ||
hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
hdr[2] == 0)
return CONTAINER_HDR_SZ;
+ eqsize = hdr[2];
+ if (eqsize < sizeof(*eq) ||
+ eqsize > UCODE_EQUIV_CPU_TABLE_MAX_SIZE)
+ return 0;
+
+ if (size < CONTAINER_HDR_SZ + eqsize)
+ return 0;
+
buf = ucode;
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, eqsize / sizeof(*eq), desc->cpuid_1_eax);
- buf += hdr[2] + CONTAINER_HDR_SZ;
- size -= hdr[2] + CONTAINER_HDR_SZ;
+ buf += eqsize + CONTAINER_HDR_SZ;
+ size -= eqsize + CONTAINER_HDR_SZ;
/*
* Scan through the rest of the container to find where it ends. We do
@@ -112,6 +130,9 @@ static ssize_t parse_container(u8 *ucode, ssize_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)
@@ -126,6 +147,10 @@ static ssize_t parse_container(u8 *ucode, ssize_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;
@@ -159,15 +184,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;
}
}
@@ -364,20 +387,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_size,
+ 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_size &&
+ 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;
}
@@ -540,15 +564,31 @@ 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 bufsize)
{
unsigned int *ibuf = (unsigned int *)buf;
- unsigned int type = ibuf[1];
- unsigned int size = ibuf[2];
+ unsigned int type, size;
- if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
- pr_err("empty section/"
- "invalid type field in container file section header\n");
+ if (bufsize < 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) ||
+ size > UCODE_EQUIV_CPU_TABLE_MAX_SIZE) {
+ pr_err("equivalent CPU table size invalid\n");
+ return -EINVAL;
+ }
+
+ if (bufsize < CONTAINER_HDR_SZ + size) {
+ pr_err("equivalent CPU table truncated\n");
return -EINVAL;
}
@@ -559,6 +599,7 @@ static int install_equiv_cpu_table(const u8 *buf)
}
memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
+ equiv_cpu_table_size = size / sizeof(struct equiv_cpu_entry);
/* add header length */
return size + CONTAINER_HDR_SZ;
@@ -568,6 +609,7 @@ static void free_equiv_cpu_table(void)
{
vfree(equiv_cpu_table);
equiv_cpu_table = NULL;
+ equiv_cpu_table_size = 0;
}
static void cleanup(void)
@@ -591,7 +633,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_ABSOLUTE) {
+ pr_err("mammoth patch size %u\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;
@@ -613,7 +663,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;
@@ -654,7 +705,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;
@@ -745,6 +796,10 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
}
ret = UCODE_ERROR;
+ if (fw->size < sizeof(u32)) {
+ pr_err("microcode 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;
On Sat, Mar 10, 2018 at 01:34:45AM +0100, Maciej S. Szmigiero wrote:
> Currently, it is very easy to make the AMD microcode update driver crash
> or spin on a malformed microcode file since it does very little
> consistency checking on data loaded from such file.
Sorry but if one has enough permissions to install malformed microcode,
crashing the loader is the least of your problems. IOW, I don't see the
justification for the unnecessary complication with all those checks.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 10.03.2018 10:18, Borislav Petkov wrote:
> On Sat, Mar 10, 2018 at 01:34:45AM +0100, Maciej S. Szmigiero wrote:
>> Currently, it is very easy to make the AMD microcode update driver crash
>> or spin on a malformed microcode file since it does very little
>> consistency checking on data loaded from such file.
>
> Sorry but if one has enough permissions to install malformed microcode,
> crashing the loader is the least of your problems. IOW, I don't see the
> justification for the unnecessary complication with all those checks.
While I agree this is not a security problem, I cannot agree that these
checks are unnecessary driver complication.
First, these checks are really just very basic checks like "check
whether the loaded file is long enough to actually contain some
structure before accessing it" or "don't iterate an array in file
without checking if it actually has a terminating element" or "check
whether microcode patch length isn't something like 2GB before allocating
memory for it".
Without them, it is easy to crash the driver when just playing with
microcode files (and it turns out that AMD-released microcode files in
linux-firmware actually don't contain the newest microcode versions,
even for older CPUs).
Second, since these checks happen only on a microcode file load
(something that 99.9% of systems probably will do just once at boot
time) it is hardly a performance-critical path.
Third, we still do check consistency of data provided to various
root-only syscalls (and these might be much more performance-critical
than this code).
> Thx.
>
Thanks,
Maciej
On Sat, Mar 10, 2018 at 01:26:00PM +0100, Maciej S. Szmigiero wrote:
> Without them, it is easy to crash the driver when just playing with
> microcode files
You're not supposed to play with the microcode files. If you do and
something breaks, you get to keep the pieces.
If the official microcode files have a problem, then I'm all ears.
Anything else contrived which doesn't actually happen unless someone
manipulates them is not an issue the microcode loader should protect
against.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 10.03.2018 14:12, Borislav Petkov wrote:
> On Sat, Mar 10, 2018 at 01:26:00PM +0100, Maciej S. Szmigiero wrote:
>> Without them, it is easy to crash the driver when just playing with
>> microcode files
>
> You're not supposed to play with the microcode files. If you do and
> something breaks, you get to keep the pieces.
>
> If the official microcode files have a problem, then I'm all ears.
> Anything else contrived which doesn't actually happen unless someone
> manipulates them is not an issue the microcode loader should protect
> against.
>
So, shall we remove data consistency checks of various root-only
syscalls then? :)
Maciej
On 10.03.2018 01:34, Maciej S. Szmigiero wrote:
> Currently, it is very easy to make the AMD microcode update driver crash
> or spin on a malformed microcode file since it does very little
> consistency checking on data loaded from such file.
>
> This commit introduces various checks, mostly on length-type fields, so
> all corrupted microcode files are (hopefully) correctly rejected instead.
>
> Signed-off-by: Maciej S. Szmigiero <[email protected]>
To make sure that it is clear what this patch is about:
It *isn't* about verifying the actual microcode update, that is, the
blob that gets sent to the CPU as the new microcode.
Such verification is (hopefully) done by the CPU itself.
It is about verifying *this driver-specific* container file that
includes many microcode updates for different CPUs of a particular CPU
family, along with metadata that helps the driver select the right
microcode update to actually sent to the CPU.
The microcode container files in linux-firmware don't contain the
newest microcode versions, even for older CPUs.
They are generally updated VERY rarely - I can see only three updates
for them: on 2016-03-18, 2014-11-30 and 2013-07-11.
There is no container file at all for family 17h (Zen) so
distributions like OpenSUSE that include this file must have gotten it
from some other source (or made from raw updates themselves).
That's why to get things like IBPB it is sometimes necessary to use
a newer microcode version than included in linux-firmware, sourced for
example from a BIOS update.
Since BIOS updates contain only actual (raw) microcode updates one
has to place it in a microcode container file so this driver can parse
it.
As far I know there is no tool to automate this work so one has to
manually tweak the container metadata.
And this is prone to mistakes this patch protects against.
Sorry for not making this 100% clear in the commit log.
Maciej
On Sat, Mar 10, 2018 at 05:16:40PM +0100, Maciej S. Szmigiero wrote:
> To make sure that it is clear what this patch is about:
I know what you're trying to do but it seems you don't want to listen.
So let me try one last time to clear your confusion.
> It *isn't* about verifying the actual microcode update, that is, the
> blob that gets sent to the CPU as the new microcode.
> Such verification is (hopefully) done by the CPU itself.
Yes, it is done by the CPU. Microcode is encrypted.
> There is no container file at all for family 17h (Zen) so
> distributions like OpenSUSE that include this file must have gotten it
> from some other source
Or maybe they've gotten it from AMD directly. Don't you think that
getting microcode from the CPU vendor directly is the logical thing?
> That's why to get things like IBPB it is sometimes necessary to use
> a newer microcode version than included in linux-firmware, sourced for
> example from a BIOS update.
linux-firmware will get F17h microcode soon.
> Since BIOS updates contain only actual (raw) microcode updates one
> has to place it in a microcode container file so this driver can parse
> it.
>
> As far I know there is no tool to automate this work so one has to
> manually tweak the container metadata.
Let me get this straight: am I reading this correctly that you've tried
to carve out the F17h microcode from a BIOS update blob and you're
trying to load that?!?
If so, you could've simply taken a distro microcode package and used
F17h microcode from there - they are all the same.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 10.03.2018 17:46, Borislav Petkov wrote:
> On Sat, Mar 10, 2018 at 05:16:40PM +0100, Maciej S. Szmigiero wrote:
(..)
>> There is no container file at all for family 17h (Zen) so
>> distributions like OpenSUSE that include this file must have gotten it
>> from some other source
>
> Or maybe they've gotten it from AMD directly. Don't you think that
> getting microcode from the CPU vendor directly is the logical thing?
"some other source" than linux-firmware includes the CPU vendor.
Also please note that while OpenSUSE can get the microcode directly
from the CPU vendor there seems to be no official AMD web site that
distributes microcode.
And it looks like other distros simply get it from OpenSUSE:
https://bugs.archlinux.org/task/56951
>> That's why to get things like IBPB it is sometimes necessary to use
>> a newer microcode version than included in linux-firmware, sourced for
>> example from a BIOS update.
>
> linux-firmware will get F17h microcode soon.
Great!
Hope it will include latest production versions for the whole family
17h.
>> Since BIOS updates contain only actual (raw) microcode updates one
>> has to place it in a microcode container file so this driver can parse
>> it.
>>
>> As far I know there is no tool to automate this work so one has to
>> manually tweak the container metadata.
>
> Let me get this straight: am I reading this correctly that you've tried
> to carve out the F17h microcode from a BIOS update blob and you're
> trying to load that?!?
>
> If so, you could've simply taken a distro microcode package and used
> F17h microcode from there - they are all the same.
>
"microcode_amd_fam17h.bin" from both my distro (Gentoo) and OpenSUSE
only contains family 23 model 2 microcode while my Ryzen is model 1.
And my motherboard BIOS-loaded microcode is too old to contain IBPB
support.
Maciej
* Maciej S. Szmigiero <[email protected]> wrote:
> Currently, it is very easy to make the AMD microcode update driver crash
> or spin on a malformed microcode file since it does very little
> consistency checking on data loaded from such file.
>
> This commit introduces various checks, mostly on length-type fields, so
> all corrupted microcode files are (hopefully) correctly rejected instead.
>
> Signed-off-by: Maciej S. Szmigiero <[email protected]>
> ---
> Test files are at https://pastebin.com/XkKUSmMp
> One has to enable KASAN in the kernel config and rename a particular
> test file to name appropriate to the running CPU family to test its
> loading.
>
> arch/x86/include/asm/microcode_amd.h | 6 ++
> arch/x86/kernel/cpu/microcode/amd.c | 111 ++++++++++++++++++++++++++---------
> 2 files changed, 89 insertions(+), 28 deletions(-)
Treating microcode update files as external input and sanity checking the data
makes sense I suppose, but there's various small uglies in the patch:
> +/* if a patch is larger than this the microcode file is surely corrupted */
> +#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)
Please capitalize comments.
> * Returns the amount of bytes consumed while scanning. @desc contains all the
> * data we're going to use in later stages of the application.
> */
> -static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
> +static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
> {
> struct equiv_cpu_entry *eq;
> - ssize_t orig_size = size;
> + size_t orig_size = size;
> u32 *hdr = (u32 *)ucode;
> + u32 eqsize;
> u16 eq_id;
> u8 *buf;
So we have 'eq_id', but 'eqsize'? Why not 'eq_size' to have fewer random
variations of coding style?
>
> + if (size < CONTAINER_HDR_SZ)
> + return 0;
The comment about CONTAINER_HDR_SZ better belongs here, where we use it.
> /* Am I looking at an equivalence table header? */
> if (hdr[0] != UCODE_MAGIC ||
> hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
> hdr[2] == 0)
> return CONTAINER_HDR_SZ;
>
> + eqsize = hdr[2];
> + if (eqsize < sizeof(*eq) ||
> + eqsize > UCODE_EQUIV_CPU_TABLE_MAX_SIZE)
> + return 0;
> +
> + if (size < CONTAINER_HDR_SZ + eqsize)
> + return 0;
> +
> buf = ucode;
>
> 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, eqsize / sizeof(*eq), desc->cpuid_1_eax);
Does eq_size have to be a multiple of sizeof(*eq)? If yes then we should probably
check that too.
> -static int install_equiv_cpu_table(const u8 *buf)
> +static int install_equiv_cpu_table(const u8 *buf, size_t bufsize)
s/bufsize/buf_size
> {
> unsigned int *ibuf = (unsigned int *)buf;
> - unsigned int type = ibuf[1];
> - unsigned int size = ibuf[2];
> + unsigned int type, size;
>
> - if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
> - pr_err("empty section/"
> - "invalid type field in container file section header\n");
> + if (bufsize < 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) ||
> + size > UCODE_EQUIV_CPU_TABLE_MAX_SIZE) {
> + pr_err("equivalent CPU table size invalid\n");
> + return -EINVAL;
> + }
> +
> + if (bufsize < CONTAINER_HDR_SZ + size) {
> + pr_err("equivalent CPU table truncated\n");
> return -EINVAL;
> }
>
> @@ -559,6 +599,7 @@ static int install_equiv_cpu_table(const u8 *buf)
> }
>
> memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
> + equiv_cpu_table_size = size / sizeof(struct equiv_cpu_entry);
Btw., 'equiv_cpu_table_size' is an ambiguous name: often _size variables are in
byte units - but this is number of entries. So the name should be
'equiv_cpu_table_entries' or so.
Thanks,
Ingo
On 11.03.2018 10:59, Ingo Molnar wrote:
>
> * Maciej S. Szmigiero <[email protected]> wrote:
>
>> Currently, it is very easy to make the AMD microcode update driver crash
>> or spin on a malformed microcode file since it does very little
>> consistency checking on data loaded from such file.
>>
>> This commit introduces various checks, mostly on length-type fields, so
>> all corrupted microcode files are (hopefully) correctly rejected instead.
>>
>> Signed-off-by: Maciej S. Szmigiero <[email protected]>
>> ---
>> Test files are at https://pastebin.com/XkKUSmMp
>> One has to enable KASAN in the kernel config and rename a particular
>> test file to name appropriate to the running CPU family to test its
>> loading.
>>
>> arch/x86/include/asm/microcode_amd.h | 6 ++
>> arch/x86/kernel/cpu/microcode/amd.c | 111 ++++++++++++++++++++++++++---------
>> 2 files changed, 89 insertions(+), 28 deletions(-)
>
> Treating microcode update files as external input and sanity checking the data
> makes sense I suppose, but there's various small uglies in the patch:
>
>> +/* if a patch is larger than this the microcode file is surely corrupted */
>> +#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)
>
> Please capitalize comments.
Will do.
>
>> * Returns the amount of bytes consumed while scanning. @desc contains all the
>> * data we're going to use in later stages of the application.
>> */
>> -static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
>> +static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
>> {
>> struct equiv_cpu_entry *eq;
>> - ssize_t orig_size = size;
>> + size_t orig_size = size;
>> u32 *hdr = (u32 *)ucode;
>> + u32 eqsize;
>> u16 eq_id;
>> u8 *buf;
>
> So we have 'eq_id', but 'eqsize'? Why not 'eq_size' to have fewer random
> variations of coding style?
Will change.
>>
>> + if (size < CONTAINER_HDR_SZ)
>> + return 0;
>
> The comment about CONTAINER_HDR_SZ better belongs here, where we use it.
Will move it.
>> /* Am I looking at an equivalence table header? */
>> if (hdr[0] != UCODE_MAGIC ||
>> hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
>> hdr[2] == 0)
>> return CONTAINER_HDR_SZ;
>>
>> + eqsize = hdr[2];
>> + if (eqsize < sizeof(*eq) ||
>> + eqsize > UCODE_EQUIV_CPU_TABLE_MAX_SIZE)
>> + return 0;
>> +
>> + if (size < CONTAINER_HDR_SZ + eqsize)
>> + return 0;
>> +
>> buf = ucode;
>>
>> 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, eqsize / sizeof(*eq), desc->cpuid_1_eax);
>
> Does eq_size have to be a multiple of sizeof(*eq)? If yes then we should probably
> check that too.
In principle yes, but having garbage at the end of the equivalence
table in a microcode container file is a non-fatal problem.
I have mixed feelings whether we should be really strict here, especially
that the existing driver would accept such files without any error.
>> -static int install_equiv_cpu_table(const u8 *buf)
>> +static int install_equiv_cpu_table(const u8 *buf, size_t bufsize)
>
> s/bufsize/buf_size
Will change.
>> {
>> unsigned int *ibuf = (unsigned int *)buf;
>> - unsigned int type = ibuf[1];
>> - unsigned int size = ibuf[2];
>> + unsigned int type, size;
>>
>> - if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size) {
>> - pr_err("empty section/"
>> - "invalid type field in container file section header\n");
>> + if (bufsize < 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) ||
>> + size > UCODE_EQUIV_CPU_TABLE_MAX_SIZE) {
>> + pr_err("equivalent CPU table size invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (bufsize < CONTAINER_HDR_SZ + size) {
>> + pr_err("equivalent CPU table truncated\n");
>> return -EINVAL;
>> }
>>
>> @@ -559,6 +599,7 @@ static int install_equiv_cpu_table(const u8 *buf)
>> }
>>
>> memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
>> + equiv_cpu_table_size = size / sizeof(struct equiv_cpu_entry);
>
> Btw., 'equiv_cpu_table_size' is an ambiguous name: often _size variables are in
> byte units - but this is number of entries. So the name should be
> 'equiv_cpu_table_entries' or so.
Will rename.
> Thanks,
>
> Ingo
>
Thanks,
Maciej