[coreboot-gerrit] Patch set updated for coreboot: 70aed62 Intel microcode: Return when `microcode_updates` is `NULL`

[Paul Menzel (paulepanter@users.sourceforge.net)]

Paul Menzel (paulepanter at users.sourceforge.net) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/2990

-gerrit

commit 70aed6232764e3f9c9231e648db19fb50c2b55c2
Author: Vladimir Serbinenko <phcoder at gmail.com>
Date:   Sat Mar 30 12:04:23 2013 +0100

    Intel microcode: Return when `microcode_updates` is `NULL`
    
    Add a safety check in function `intel_update_microcode` to return when
    accidentally `NULL` is passed as `microcode_updates`, which would lead
    to a null pointer dereference later on.
    
        for (c = microcode_updates; m->hdrver; m = (const struct microcode *)c) {
    
    While at it, use `return NULL` for clarity in function
    `intel_microcode_find` and include the header file `stddef.h`. for it.
    
    The review of this patch had some more discussion on adding more
    comments and more detailed error messages. But this should be done in
    a separate patch.
    
    For clarity here some history, on how this was found and what caused
    the discussion and confusion.
    
    Originally when Vladimir made this improvement, selecting
    `CPU_MICROCODE_IN_CBFS` in Kconfig but not having the microcode blob
    `cpu_microcode_blob.bin` in CBFS resulted in a null pointer dereference
    later on causing a crash.
    
        for (c = microcode_updates; m->hdrver; m = (const struct microcode *)c) {
    
    Vladimir fixed this by returning if `microcode_updates` is `NULL`,
    that means no file is found and successfully tested this on his
    Lenovo X201.
    
    When pushing the patch to Gerrit for review, the code was rewritten
    though by Aaron in commit »intel microcode: split up microcode loading
    stages« (98ffb426) [1], which also returns when no file is found. So
    the other parts of the code were checked and the safety check as
    described above is added.
    
    [1] http://review.coreboot.org/2778
    
    Change-Id: I6e18fd37256910bf047061e4633a66cf29ad7b69
    Signed-off-by: Vladimir Serbinenko <phcoder at gmail.com>
    Signed-off-by: Paul Menzel <paulepanter at users.sourceforge.net>
---
 src/cpu/intel/microcode/microcode.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/cpu/intel/microcode/microcode.c b/src/cpu/intel/microcode/microcode.c
index d908c25..1991ed8 100644
--- a/src/cpu/intel/microcode/microcode.c
+++ b/src/cpu/intel/microcode/microcode.c
@@ -21,6 +21,7 @@
 /* Microcode update for Intel PIII and later CPUs */
 
 #include <stdint.h>
+#include <stddef.h>
 #if !defined(__ROMCC__)
 #include <console/console.h>
 #endif
@@ -131,7 +132,7 @@ const void *intel_microcode_find(void)
 #endif
 
 	if (!microcode_updates)
-		return microcode_updates;
+		return NULL;
 
 	/* CPUID sets MSR 0x8B iff a microcode update has been loaded. */
 	msr.lo = 0;
@@ -202,6 +203,13 @@ void intel_update_microcode(const void *microcode_updates)
 	const char *c;
 	msr_t msr;
 
+	if (!microcode_updates) {
+#if !defined(__ROMCC__)
+		printk(BIOS_WARNING, "No microcode updates found.\n");
+#endif
+		return;
+	}
+
 	/* CPUID sets MSR 0x8B iff a microcode update has been loaded. */
 	msr.lo = 0;
 	msr.hi = 0;