linux-kernel - Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.20.1701172105180.3645@nanos>
Date:   Tue, 17 Jan 2017 21:29:05 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>
cc:     X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing

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