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]>
---
Changes from v1: Capitalize a comment, rename 'eqsize' and 'bufsize'
to 'eq_size' and 'buf_size', respectively, attach a comment about
checking the equivalence table header to its first size check, rename
'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.
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 | 112 ++++++++++++++++++++++++++---------
2 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/arch/x86/include/asm/microcode_amd.h b/arch/x86/include/asm/microcode_amd.h
index 209492849566..373a202ea569 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..1cbccf79ff68 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -39,6 +39,7 @@
#include <asm/msr.h>
static struct equiv_cpu_entry *equiv_cpu_table;
+static unsigned int equiv_cpu_table_entries;
/*
* This points to the current valid container of microcode patches which we will
@@ -63,12 +64,18 @@ static u8 amd_ucode_patch[PATCH_MAX_SIZE];
static const char
ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
-static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
+static u16 find_equiv_id(const struct equiv_cpu_entry *equiv_table,
+ unsigned int equiv_table_entries, u32 sig)
{
- for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
- if (sig == equiv_table->installed_cpu)
- return equiv_table->equiv_cpu;
- }
+ unsigned int i;
+
+ if (!equiv_table)
+ return 0;
+
+ for (i = 0; i < equiv_table_entries && equiv_table[i].installed_cpu;
+ i++)
+ if (sig == equiv_table[i].installed_cpu)
+ return equiv_table[i].equiv_cpu;
return 0;
}
@@ -80,29 +87,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 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) ||
+ eq_size > UCODE_EQUIV_CPU_TABLE_MAX_SIZE)
+ return 0;
+
+ if (size < CONTAINER_HDR_SZ + eq_size)
+ 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, eq_size / sizeof(*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
@@ -112,6 +131,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 +148,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 +185,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 +388,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;
}
@@ -540,15 +565,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 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 (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) ||
+ size > UCODE_EQUIV_CPU_TABLE_MAX_SIZE) {
+ pr_err("equivalent CPU table size invalid\n");
+ return -EINVAL;
+ }
+
+ if (buf_size < CONTAINER_HDR_SZ + size) {
+ pr_err("equivalent CPU table truncated\n");
return -EINVAL;
}
@@ -559,6 +600,7 @@ static int install_equiv_cpu_table(const u8 *buf)
}
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;
@@ -568,6 +610,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)
@@ -591,7 +634,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 +664,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 +706,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 +797,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 Sun, Mar 11, 2018 at 04:27:03PM +0100, Maciej S. Szmigiero wrote:
> +/* 4k */
> +#define UCODE_EQUIV_CPU_TABLE_MAX_SIZE (256 * sizeof(struct equiv_cpu_entry))
And you came up with that max size how exactly?
> +/* If a patch is larger than this the microcode file is surely corrupted */
> +#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)a
Same question here.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 12.03.2018 10:53, Borislav Petkov wrote:
> On Sun, Mar 11, 2018 at 04:27:03PM +0100, Maciej S. Szmigiero wrote:
>> +/* 4k */
>> +#define UCODE_EQUIV_CPU_TABLE_MAX_SIZE (256 * sizeof(struct equiv_cpu_entry))
>
> And you came up with that max size how exactly?
The equivalent CPU table is allocated using vmalloc() so it is nice
when the maximum size is an integer multiple of the page size.
Since the maximum entry count in current microcode files is 18 the
maximum size of 256 entries (or one page) gives us plenty of headroom.
Also, looking in the past, there probably won't be more than 256 AMD CPU
types in one CPU family.
>
>> +/* If a patch is larger than this the microcode file is surely corrupted */
>> +#define PATCH_MAX_SIZE_ABSOLUTE (16 * 1024 * 1024)
>
> Same question here.
>
This limit is an absolute upper cap of a patch size.
This number is high so it won't have to be changed in the future, it
only serves as a plausibility check before we consider other data
contained in the particular patch.
Current patches are 4k max size, but since the size field is
32 bit-wide one can theoretically specify sizes up to 4GB.
Since these two maximum sizes are somewhat arbitrary if anybody wants
to propose other values the patch can be updated, naturally.
Maciej
On Mon, Mar 12, 2018 at 01:56:59PM +0100, Maciej S. Szmigiero wrote:
> The equivalent CPU table is allocated using vmalloc() so it is nice
> when the maximum size is an integer multiple of the page size.
Arbitrary.
> Since the maximum entry count in current microcode files is 18 the
Where did you dream up that 18?
> maximum size of 256 entries (or one page) gives us plenty of headroom.
Arbitrary.
> Also, looking in the past, there probably won't be more than 256 AMD CPU
> types in one CPU family.
Wrong.
The only limitation on the equivalence table size we have is the 32-bit
unsigned length field at offset 8 in the equivalence table header.
> This limit is an absolute upper cap of a patch size.
More dreamt up crap.
See verify_patch_size() for the actual patch sizes.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 12.03.2018 14:06, Borislav Petkov wrote:
> On Mon, Mar 12, 2018 at 01:56:59PM +0100, Maciej S. Szmigiero wrote:
(..)
>> Since the maximum entry count in current microcode files is 18 the
>
> Where did you dream up that 18?
"microcode_amd.bin" in linux-firmware.
>> Also, looking in the past, there probably won't be more than 256 AMD CPU
>> types in one CPU family.
>
> Wrong.
There is no problem raising this value in that (future) case.
As I wrote previously, currently the maximum used count is 18.
> The only limitation on the equivalence table size we have is the 32-bit
> unsigned length field at offset 8 in the equivalence table header.
Not really, since even in the existing code CONTAINER_HDR_SZ (12) gets
added to this size, then the sum is cast to a (signed) int.
If this value is negative then the file get rejected.
>> This limit is an absolute upper cap of a patch size.
>
> More dreamt up crap.
>
> See verify_patch_size() for the actual patch sizes.
>
It can be changed to the current maximum across sizes for particular
families, but then the limit will need to be raised when adding a new
family (if it uses a larger patch).
Maciej
On Mon, Mar 12, 2018 at 02:32:30PM +0100, Maciej S. Szmigiero wrote:
> "microcode_amd.bin" in linux-firmware.
That is the microcode container for all families < 0x15. And it
*happens* to have 18 entries.
So purely arbitrary:
Equivalence table (magic: AMD, type: 0, length: 288 (0x120))
============================================================
| inst_cpu | err_msk | err_cmp | eq_cpu | res |
==========================================================
| 0x00100f80 | 0x00000000 | 0x00000000 | 0x1080 | 0x0000 |
| 0x00100f81 | 0x00000000 | 0x00000000 | 0x1081 | 0x0000 |
| 0x00100f62 | 0x00000000 | 0x00000000 | 0x1062 | 0x0000 |
| 0x00100f23 | 0x00000000 | 0x00000000 | 0x1022 | 0x0000 |
| 0x00100f43 | 0x00000000 | 0x00000000 | 0x1043 | 0x0000 |
| 0x00100f91 | 0x00000000 | 0x00000000 | 0x1081 | 0x0000 |
| 0x00100f2a | 0x00000000 | 0x00000000 | 0x1020 | 0x0000 |
| 0x00100f63 | 0x00000000 | 0x00000000 | 0x1043 | 0x0000 |
| 0x00100f42 | 0x00000000 | 0x00000000 | 0x1041 | 0x0000 |
| 0x00300f10 | 0x00000000 | 0x00000000 | 0x3010 | 0x0000 |
| 0x00200f31 | 0x00000000 | 0x00000000 | 0x2031 | 0x0000 |
| 0x00100f52 | 0x00000000 | 0x00000000 | 0x1041 | 0x0000 |
| 0x00100fa0 | 0x00000000 | 0x00000000 | 0x10a0 | 0x0000 |
| 0x00100f53 | 0x00000000 | 0x00000000 | 0x1043 | 0x0000 |
| 0x00100f22 | 0x00000000 | 0x00000000 | 0x1022 | 0x0000 |
| 0x00500f10 | 0x00000000 | 0x00000000 | 0x5010 | 0x0000 |
| 0x00500f20 | 0x00000000 | 0x00000000 | 0x5020 | 0x0000 |
| 0x00000000 | 0x00000000 | 0x00000000 | 0x0000 | 0x0000 |
> There is no problem raising this value in that (future) case.
> As I wrote previously, currently the maximum used count is 18.
There is a problem because not everyone can upgrade their kernels
like you. Distros and big deployments can't just up and update their
kernels at a whim just because you imposed an arbitrary limit which you
determined would be ok.
> Not really, since even in the existing code CONTAINER_HDR_SZ (12) gets
> added to this size, then the sum is cast to a (signed) int.
> If this value is negative then the file get rejected.
That is a bug in install_equiv_cpu_table() - it should return unsigned int.
> It can be changed to the current maximum across sizes for particular
What is the "current maximum across sizes"?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 12.03.2018 14:48, Borislav Petkov wrote:
> On Mon, Mar 12, 2018 at 02:32:30PM +0100, Maciej S. Szmigiero wrote:
>> "microcode_amd.bin" in linux-firmware.
>
> That is the microcode container for all families < 0x15. And it
> *happens* to have 18 entries.
>
> So purely arbitrary:
>
> Equivalence table (magic: AMD, type: 0, length: 288 (0x120))
(the long table was cut)
>
>> There is no problem raising this value in that (future) case.
>> As I wrote previously, currently the maximum used count is 18.
>
> There is a problem because not everyone can upgrade their kernels
> like you. Distros and big deployments can't just up and update their
> kernels at a whim just because you imposed an arbitrary limit which you
> determined would be ok.
First, this limit is more than 14 times higher than the current maximum
count.
And this current maximum was reached by CPU types added in
families < 15h during last 10+ years (the oldest supported CPU family in
this container is 10h, which - according to Wikipedia - was released
September 10, 2007).
At that rate exceeding this limit will take 130+ additional years (and
this assumes that AMD will introduce new CPU types in these families
at the same rate as in the last 10 years - I sincerely doubt it).
If somebody does not update their kernel for 130 years (even to -stable
versions) then he or she has a much bigger problem than incompatibility
with the current microcode update file.
>
>> Not really, since even in the existing code CONTAINER_HDR_SZ (12) gets
>> added to this size, then the sum is cast to a (signed) int.
>> If this value is negative then the file get rejected.
>
> That is a bug in install_equiv_cpu_table() - it should return unsigned int.
>
>> It can be changed to the current maximum across sizes for particular
>
> What is the "current maximum across sizes"?
>
This is the current maximum patch size across families:
#define F15H_MPB_MAX_SIZE 4096
Maciej
On Mon, Mar 12, 2018 at 03:10:47PM +0100, Maciej S. Szmigiero wrote:
> And this current maximum was reached by CPU types added in
> families < 15h during last 10+ years (the oldest supported CPU family in
You're assuming that the rate of adding patches to the microcode
container won't change. You have a crystal ball which shows you the
future?
Ok, enough with the bullshit.
Here's what I'll take as hardening patches:
1. Check whether the equivalence table length is not exceeding the size
of the whole blob. This is the only sane limit check we can do - no
arbitrary bullshit of it'll take how many years to reach some limit.
2. Add a PATCH_MAX_SIZE macro which evaluates against the max of all
family patch sizes:
#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
so that future additions won't break the macro.
3. Fix install_equiv_cpu_table() to return an unsigned int
Make all the points above into separate patches, please, with proper
commit messages explaining why they do what they do and test them.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.