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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
