[GIT PULL] microcode loader updates

[GIT PULL] microcode loader updates

[Borislav Petkov]

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
      x86/microcode/intel: Get rid of last arg to load_ucode_intel_bsp()
      x86/microcode/intel: Simplify load_ucode_intel_bsp()
      x86/microcode/intel: Make _save_mc() return the updated saved count
      x86/microcode/intel: Sanitize _save_mc()
      x86/microcode/intel: Rename update_match_revision()
      x86/microcode: Consolidate family,model, ... code
      x86/microcode/intel: Simplify generic_load_microcode_early()
      x86/microcode/intel: Move mc arg last in get_matching_{microcode|sig}
      x86/microcode/intel: Sanitize microcode_pointer()
      x86/microcode/intel: Check scan_microcode()'s retval
      x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc()

Quentin Casasnovas (1):
      x86/microcode/intel: Fix out of bounds memory access to the extended header

 arch/x86/include/asm/microcode.h            |  73 ++++++
 arch/x86/include/asm/microcode_intel.h      |  13 +-
 arch/x86/kernel/cpu/microcode/core_early.c  |  75 +-----
 arch/x86/kernel/cpu/microcode/intel.c       |   4 +-
 arch/x86/kernel/cpu/microcode/intel_early.c | 344 +++++++++++++---------------
 arch/x86/kernel/cpu/microcode/intel_lib.c   |  22 +-
 6 files changed, 259 insertions(+), 272 deletions(-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [GIT PULL] microcode loader updates

[Quentin Casasnovas]

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

Re: [GIT PULL] microcode loader updates

[Borislav Petkov]

On Mon, Mar 02, 2015 at 02:03:36PM +0100, Quentin Casasnovas wrote:
> 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.

You're correct, but(!)...

Practically, this is not a problem because @mc_saved_src being handed
down to save_microcode() is at both call sites initialized up to
mc_saved_count elements and the loop in save_microcode() only inspects
this far.

So actually, this test is not really needed:

                if (!mc_saved_src[i]) {
                        ret = -EINVAL;
                        goto err;
                }

AFAICT and if I'm not missing anything else, of course.

In any case, I'd like to keep this series cleanup-only (well, except
this one) and address your comments later. Don't worry, I haven't
forgotten them - I want to *not* fix everything in one go.

Agreed?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [GIT PULL] microcode loader updates

[Quentin Casasnovas]

On Mon, Mar 02, 2015 at 02:29:50PM +0100, Borislav Petkov wrote:
> On Mon, Mar 02, 2015 at 02:03:36PM +0100, Quentin Casasnovas wrote:
> > 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.
> 
> You're correct, but(!)...
> 
> Practically, this is not a problem because @mc_saved_src being handed
> down to save_microcode() is at both call sites initialized up to
> mc_saved_count elements and the loop in save_microcode() only inspects
> this far.
> 
> So actually, this test is not really needed:
> 
>                 if (!mc_saved_src[i]) {
>                         ret = -EINVAL;
>                         goto err;
>                 }
> 
> AFAICT and if I'm not missing anything else, of course.
> 
> In any case, I'd like to keep this series cleanup-only (well, except
> this one) and address your comments later. Don't worry, I haven't
> forgotten them - I want to *not* fix everything in one go.
> 
> Agreed?
> 

Hey up to you, really :)

It's just that this potential-but-very-very-likely-impossible kfree() on
garbage wasn't present in the original code - so I thought changing the
kmalloc() => kcalloc() was small enough to add in your serie.  I'd also be
fine removing the early loop termination condition if you think it's dead
code since that'll make sure this will never happen.  A static analyzer or
maybe some cocinnelle semantic patches are likely to start complaining
about this otherwise, I think.

Quentin

Re: [GIT PULL] microcode loader updates

[Borislav Petkov]

On Mon, Mar 02, 2015 at 02:42:12PM +0100, Quentin Casasnovas wrote:
> It's just that this potential-but-very-very-likely-impossible kfree() on
> garbage wasn't present in the original code - so I thought changing the
> kmalloc() => kcalloc() was small enough to add in your serie.  I'd also be
> fine removing the early loop termination condition if you think it's dead
> code since that'll make sure this will never happen.  A static analyzer or
> maybe some cocinnelle semantic patches are likely to start complaining
> about this otherwise, I think.

Ok, ok, you got me persuaded.

---
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 3fd583b4f576..2f49ab4ac0ae 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -189,8 +189,7 @@ save_microcode(struct mc_saved_data *mc_saved_data,
 	/*
 	 * Copy new microcode data.
 	 */
-	saved_ptr = kmalloc(mc_saved_count * sizeof(struct microcode_intel *),
-			     GFP_KERNEL);
+	saved_ptr = kcalloc(mc_saved_count, sizeof(struct microcode_intel *), GFP_KERNEL);
 	if (!saved_ptr)
 		return -ENOMEM;
--

Better?

:-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [GIT PULL] microcode loader updates

[Quentin Casasnovas]

On Mon, Mar 02, 2015 at 04:04:28PM +0100, Borislav Petkov wrote:
> 
> Ok, ok, you got me persuaded.

Oh. that's unexpected :)

> 
> Better?
> 
> :-)
> 

I prefer it, thanks!

Quentin

Re: [GIT PULL] microcode loader updates

[Borislav Petkov]

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:

Ok tag updated: top commit is now 92d6ad62332712b229c8b752e8989f2f0c5b6e9a.

Ingo, you can pull now :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [GIT PULL] microcode loader updates

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> 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
>       x86/microcode/intel: Get rid of last arg to load_ucode_intel_bsp()
>       x86/microcode/intel: Simplify load_ucode_intel_bsp()
>       x86/microcode/intel: Make _save_mc() return the updated saved count
>       x86/microcode/intel: Sanitize _save_mc()
>       x86/microcode/intel: Rename update_match_revision()
>       x86/microcode: Consolidate family,model, ... code
>       x86/microcode/intel: Simplify generic_load_microcode_early()
>       x86/microcode/intel: Move mc arg last in get_matching_{microcode|sig}
>       x86/microcode/intel: Sanitize microcode_pointer()
>       x86/microcode/intel: Check scan_microcode()'s retval
>       x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc()
> 
> Quentin Casasnovas (1):
>       x86/microcode/intel: Fix out of bounds memory access to the extended header
> 
>  arch/x86/include/asm/microcode.h            |  73 ++++++
>  arch/x86/include/asm/microcode_intel.h      |  13 +-
>  arch/x86/kernel/cpu/microcode/core_early.c  |  75 +-----
>  arch/x86/kernel/cpu/microcode/intel.c       |   4 +-
>  arch/x86/kernel/cpu/microcode/intel_early.c | 344 +++++++++++++---------------
>  arch/x86/kernel/cpu/microcode/intel_lib.c   |  22 +-
>  6 files changed, 259 insertions(+), 272 deletions(-)

Pulled the updated version into tip:x86/microcode, thanks Boris!

	Ingo