Messages in this thread | | | Date | Mon, 2 Mar 2015 14:03:36 +0100 | From | Quentin Casasnovas <> | Subject | Re: [GIT PULL] microcode loader updates |
| |
Hi Boris!
On Mon, Mar 02, 2015 at 01:34:41PM +0100, Borislav Petkov wrote: > Hi guys, > > here's the first pile of microcode loader cleanups for 4.1. > > Please pull, > thanks. > > --- > The following changes since commit c517d838eb7d07bbe9507871fab3931deccff539: > > Linux 4.0-rc1 (2015-02-22 18:21:14 -0800) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git tags/intel_microcode_cleanup_p1 > > for you to fetch changes up to 94a7cbf93996711a32e286488b8523740d6a19f0: > > x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc() (2015-03-02 10:07:36 +0100) > > ---------------------------------------------------------------- > The first part of the scrubbing of the intel early microcode loader. > There's more work to come but let's unload this pile first. > > ---------------------------------------------------------------- > Borislav Petkov (13): > x86/microcode/intel: Check if microcode was found before applying > x86/microcode/intel: Do the mc_saved_src NULL check first
I don't know if you missed a comment I had on my initial review but I think you might be introducing a kfree() on garbage data in one edge case here. Sorry if I'm missing something, as usual :)
In save_microcode(), if at some loop iteration mc_saved_src[i] == NULL, we'll jump to label `err`:
> saved_ptr = kmalloc(mc_saved_count * sizeof(struct microcode_intel *), > GFP_KERNEL); > if (!saved_ptr) > return -ENOMEM; > > for (i = 0; i < mc_saved_count; i++) { > struct microcode_header_intel *mc_hdr; > struct microcode_intel *mc; > unsigned long size; > > if (!mc_saved_src[i]) { > ret = -EINVAL; > goto err; > } > mc = mc_saved_src[i]; > mc_hdr = &mc->hdr; > size = get_totalsize(mc_hdr); > > saved_ptr[i] = kmalloc(size, GFP_KERNEL); > if (!saved_ptr[i]) { > ret = -ENOMEM; > goto err; > } > > memcpy(saved_ptr[i], mc, size); > }
which does:
> for (j = 0; j <= i; j++) > kfree(saved_ptr[j]); > kfree(saved_ptr);
So at the last loop iteration for j == i, we'll do kfree(saved_ptr[j]) which AFAICT hasn't been initialized yet. Using a kcalloc() your first allocation for saved_ptr should just work since the memory will be cleared and kfree(NULL) doesn't do anything.
Quentin
| |