re: Brainy: bug in x86/cpu_ucode_intel.c

[matthew green <mrg%eterna.com.au@localhost>]

> It looks obvious (based upon looking at an older version, where it
> was simply an error if the buffer was not aligned), the
> 
>         wrmsr(MSR_BIOS_UPDT_TRIG, (uintptr_t)(sc->sc_blob) + 48);
> 
> should be using uh (which is the same value as sc->sc_blob if that
> was aligned satisfactorily on entry) rather than sc->sc_blob.
> 
> The code as it is is clearly broken, I'm not sure why there's even any
> discussion about this.

nicely spotted.  i don't think anyone else noticed.

> Cease the silly discussion, and just fix it

i think it's hard to test, thus hard to commit.

how about this:


Index: cpu_ucode_intel.c
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/x86/cpu_ucode_intel.c,v
retrieving revision 1.8
diff -p -u -r1.8 cpu_ucode_intel.c
--- cpu_ucode_intel.c	12 May 2015 00:00:35 -0000	1.8
+++ cpu_ucode_intel.c	4 Oct 2015 00:48:45 -0000
@@ -110,7 +110,7 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
 {
 	uint32_t ucodetarget, oucodeversion, nucodeversion;
 	int platformid;
-	struct intel1_ucode_header *uh;
+	struct intel1_ucode_header *uh, *uha;
 	size_t newbufsize = 0;
 	int rv = 0;
 
@@ -126,12 +126,12 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
 	if ((uintptr_t)(sc->sc_blob) & 15) {
 		/* Make the buffer 16 byte aligned */
 		newbufsize = sc->sc_blobsize + 15;
-		uh = kmem_alloc(newbufsize, KM_SLEEP);
-		if (uh == NULL) {
+		uha = kmem_alloc(newbufsize, KM_SLEEP);
+		if (uha == NULL) {
 			printf("%s: memory allocation failed\n", __func__);
 			return EINVAL;
 		}
-		uh = (struct intel1_ucode_header *)roundup2((uintptr_t)uh, 16);
+		uh = (struct intel1_ucode_header *)roundup2((uintptr_t)uha, 16);
 		/* Copy to the new area */
 		memcpy(uh, sc->sc_blob, sc->sc_blobsize);
 	}
@@ -144,7 +144,7 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
 		rv = EEXIST; /* ??? */
 		goto out;
 	}
-	wrmsr(MSR_BIOS_UPDT_TRIG, (uintptr_t)(sc->sc_blob) + 48);
+	wrmsr(MSR_BIOS_UPDT_TRIG, (uintptr_t)uh + 48);
 	intel_getcurrentucode(&nucodeversion, &platformid);
 
 	kpreempt_enable();
@@ -158,7 +158,7 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
 	       oucodeversion, nucodeversion);
 out:
 	if (newbufsize != 0)
-		kmem_free(uh, newbufsize);
+		kmem_free(uha, newbufsize);
 	return rv;
 }
 #endif /* ! XEN */