From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751187AbdAQU3d (ORCPT ); Tue, 17 Jan 2017 15:29:33 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:46335 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751106AbdAQU3N (ORCPT ); Tue, 17 Jan 2017 15:29:13 -0500 Date: Tue, 17 Jan 2017 21:29:05 +0100 (CET) From: Thomas Gleixner To: Borislav Petkov cc: X86 ML , LKML Subject: Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing In-Reply-To: <20170117173734.14251-7-bp@alien8.de> Message-ID: References: <20170117173734.14251-1-bp@alien8.de> <20170117173734.14251-7-bp@alien8.de> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 17 Jan 2017, Borislav Petkov wrote: > + /* Find the equivalence ID of our CPU in this table: */ > + eq_id = find_equiv_id(eq, eax); So here we figure out whether the blob claims to have a patch for that cpu. > + /* > + * Scan through the rest of the container to find where it ends. We do > + * some basic sanity-checking too. > + */ Stupid question. Why do we need to walk through that blob if we already know that it does not contain a patch for this cpu, i.e. eq_id == 0 ? I assume it has to do with the multiple containers glued together in the blob, but that should be mentioned in the comment. > + while (size > 0) { > + struct microcode_amd *mc; > + u32 patch_size; > > - eq_id = find_equiv_id(eq, eax); > - if (eq_id) { > - ret.size = compute_container_size(ret.data, left + offset); > + hdr = (u32 *)buf; > > - /* > - * truncate how much we need to iterate over in the > - * ucode update loop below > - */ > - left = ret.size - offset; > + if (hdr[0] != UCODE_UCODE_TYPE) > + break; > > - *desc = ret; > - return eq_id; > + /* Sanity-check patch size. */ > + patch_size = hdr[1]; > + if (patch_size > PATCH_MAX_SIZE) { > + /* Something corrupted the container, invalidate it. */ > + eq_id = 0; > + break; > } > > - /* > - * support multiple container files appended together. if this > - * one does not have a matching equivalent cpu entry, we fast > - * forward to the next container file. > - */ > - while (left > 0) { > - header = (u32 *)data; > + /* Skip patch section header: */ > + buf += SECTION_HDR_SIZE; > + size -= SECTION_HDR_SIZE; > > - if (header[0] == UCODE_MAGIC && > - header[1] == UCODE_EQUIV_CPU_TABLE_TYPE) > - break; > - > - offset = header[1] + SECTION_HDR_SIZE; > - data += offset; > - left -= offset; > + mc = (struct microcode_amd *)buf; > + if (eq_id == mc->hdr.processor_rev_id) { > + desc->psize = patch_size; > + desc->mc = mc; So here you set patch_size and mc when the eq_id is matching. I assume we continue the scan for the same reason as we do the scan for eq_id = 0, right? > } > > - /* mark where the next microcode container file starts */ > - offset = data - (u8 *)ucode; > - ucode = data; > + buf += patch_size; > + size -= patch_size; > + } > + > + /* > + * If we have found an eq_id, it means we're looking at the container > + * which has a patch for this CPU so return 0 to mean, @ucode already > + * points to it and it will be parsed later. Otherwise, we return the > + * size we scanned so that we can advance to the next container in the > + * buffer. > + */ > + if (eq_id) { Now this one is dangerous. If the blob is corrupted we might have exited the loop above due to > + if (hdr[0] != UCODE_UCODE_TYPE) > + break; before the eq_id matching happened. In that case we return success, but desc->psize and desc->mc are not set. Not what you want, right? > + desc->eq_id = eq_id; > + desc->data = ucode; > + desc->size = orig_size - size; > + > + return 0; > @@ -241,49 +232,33 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch, > #endif > > if (check_current_patch_level(&rev, true)) > - return false; So this becomes > + return ret; which is not really better than the original code. I think we really should only use the variable when there is something which can change between two points, but that's my personal preference and up to you :) > + if (!desc.eq_id) > + return ret; > > - mc = (struct microcode_amd *)(data + SECTION_HDR_SIZE); > + this_equiv_id = desc.eq_id; Why are you storing the id when you don't have an idea whether the patch is actually available and useable? There might be a proper reason, but w/o a comment or access to the microcode crystalball it's hard to tell. > static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family) > @@ -402,6 +377,7 @@ void load_ucode_amd_ap(unsigned int family) > } > > if (!apply_microcode_early_amd(cp.data, cp.size, false, &cont)) { > + cont.data = NULL; > cont.size = -1; What's the point of fiddling with the local variable at all if we return right away? > return; > } > @@ -440,7 +416,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam) > { > enum ucode_state ret; > int retval = 0; > - u16 eq_id; > > if (!cont.data) { > if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) { > @@ -456,8 +431,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam) > return -EINVAL; > } > > - eq_id = find_proper_container(cp.data, cp.size, &cont); > - if (!eq_id) { > + scan_containers(cp.data, cp.size, &cont); > + if (!cont.eq_id) { > cont.size = -1; Ditto. That might be fixed in a seperate patch because thats existing code. > return -EINVAL; Thanks, tglx