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
