On 11.03.2018 10:59, Ingo Molnar wrote:
>
> * Maciej S. Szmigiero <m...@maciej.szmigiero.name> 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 <m...@maciej.szmigiero.name>
>> ---
>> 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
