x86/microcode: Do not select FW_LOADER
diff mbox series

Message ID 20200610042911.GA20058@gondor.apana.org.au
State New, archived
Headers show
Series
  • x86/microcode: Do not select FW_LOADER
Related show

Commit Message

Herbert Xu June 10, 2020, 4:29 a.m. UTC
The x86 microcode support works just fine without FW_LOADER.  In
fact these days most people load them early in boot so FW_LOADER
never gets into the picture anyway.

People who need the FW_LOADER capability can still enable it.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Borislav Petkov June 10, 2020, 8:16 a.m. UTC | #1
On Wed, Jun 10, 2020 at 02:29:11PM +1000, Herbert Xu wrote:
> The x86 microcode support works just fine without FW_LOADER.  In
> fact these days most people load them early in boot so FW_LOADER
> never gets into the picture anyway.

What's the use case here?

$ git grep -E "select.*FW_LOADER" | wc -l
132

so this thing gets selected anyway, practically.

Also, I'm working on removing that homegrown get_builtin_firmware() and
use the one in the fw loader:

https://lkml.kernel.org/r/20200408094526.GC24663@zn.tnic

Leaving in the rest for reference.

> People who need the FW_LOADER capability can still enable it.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1d6104ea8af0..8aac7a65bfbb 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1296,7 +1296,6 @@ config MICROCODE
>  	bool "CPU microcode loading support"
>  	default y
>  	depends on CPU_SUP_AMD || CPU_SUP_INTEL
> -	select FW_LOADER
>  	---help---
>  	  If you say Y here, you will be able to update the microcode on
>  	  Intel and AMD processors. The Intel support is for the IA32 family,
> @@ -1318,7 +1317,6 @@ config MICROCODE_INTEL
>  	bool "Intel microcode loading support"
>  	depends on MICROCODE
>  	default MICROCODE
> -	select FW_LOADER
>  	---help---
>  	  This options enables microcode patch loading support for Intel
>  	  processors.
> @@ -1330,7 +1328,6 @@ config MICROCODE_INTEL
>  config MICROCODE_AMD
>  	bool "AMD microcode loading support"
>  	depends on MICROCODE
> -	select FW_LOADER
>  	---help---
>  	  If you select this option, microcode patch loading support for AMD
>  	  processors will be enabled.
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 7019d4b2df0c..5524ea15b3df 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -145,7 +145,6 @@ extern struct builtin_fw __end_builtin_fw[];
>  
>  bool get_builtin_firmware(struct cpio_data *cd, const char *name)
>  {
> -#ifdef CONFIG_FW_LOADER
>  	struct builtin_fw *b_fw;
>  
>  	for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
> @@ -155,7 +154,6 @@ bool get_builtin_firmware(struct cpio_data *cd, const char *name)
>  			return true;
>  		}
>  	}
> -#endif
>  	return false;
>  }
>  
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu June 10, 2020, 10:28 a.m. UTC | #2
On Wed, Jun 10, 2020 at 10:16:09AM +0200, Borislav Petkov wrote:
> On Wed, Jun 10, 2020 at 02:29:11PM +1000, Herbert Xu wrote:
> > The x86 microcode support works just fine without FW_LOADER.  In
> > fact these days most people load them early in boot so FW_LOADER
> > never gets into the picture anyway.
> 
> What's the use case here?

Because MICROCODE is the only thing on my system that pulls it
into the kernel.

> Also, I'm working on removing that homegrown get_builtin_firmware() and
> use the one in the fw loader:
> 
> https://lkml.kernel.org/r/20200408094526.GC24663@zn.tnic

That shouldn't be a problem.  That function can simply move under
another (hidden) Kconfig option that gets selected by both MICROCODE
and FW_LOADER.

Cheers,
Borislav Petkov June 10, 2020, 10:34 a.m. UTC | #3
On Wed, Jun 10, 2020 at 08:28:51PM +1000, Herbert Xu wrote:
> Because MICROCODE is the only thing on my system that pulls it
> into the kernel.

That's a very interesting system you have.

> That shouldn't be a problem.  That function can simply move under
> another (hidden) Kconfig option that gets selected by both MICROCODE
> and FW_LOADER.

Out Kconfig symbols space is a mess and I'd prefer not to add another
one if there's no good reason for it.
Herbert Xu June 10, 2020, 10:41 a.m. UTC | #4
On Wed, Jun 10, 2020 at 12:34:18PM +0200, Borislav Petkov wrote:
> 
> Out Kconfig symbols space is a mess and I'd prefer not to add another
> one if there's no good reason for it.

Adding two thousand lines of code to the kernel when you only need
a few lines is ridiculous.  Worse those two thousand lines increase
the attack surface to the kernel because they're exposed to user-
space.

Adding a hidden Kconfig symbol for the sake of reducing the kernel
attack surface would seem worthwhile.  In fact this isn't even an
issue right now because you are still using the custom function.

Cheers,
Borislav Petkov June 10, 2020, 10:48 a.m. UTC | #5
On Wed, Jun 10, 2020 at 08:41:13PM +1000, Herbert Xu wrote:
> Adding two thousand lines of code to the kernel when you only need
> a few lines is ridiculous.  Worse those two thousand lines increase
> the attack surface to the kernel because they're exposed to user-
> space.

Why isn't *this* in your commit message?

> Adding a hidden Kconfig symbol for the sake of reducing the kernel
> attack surface would seem worthwhile.

And that.

> In fact this isn't even an issue right now because you are still using
> the custom function.

Actually, I'd prefer your version which doesn't depend on FW_LOADER at
all for the above reasons.

Please resubmit with amended commit message, adding the justification
for the change.

Thx.
Luis Chamberlain June 10, 2020, 1:12 p.m. UTC | #6
On Wed, Jun 10, 2020 at 10:16:09AM +0200, Borislav Petkov wrote:
> 
> Also, I'm working on removing that homegrown get_builtin_firmware() and
> use the one in the fw loader:
> 
> https://lkml.kernel.org/r/20200408094526.GC24663@zn.tnic

I would like to still encourage this, even with this patch in place,
as I think it makes this a proper call, and reflects better how the
firmware loader is used exactly.

FWIW, firmware loader will be changed soon to not be modular, and just
built-in or disabled.

  Luis
Borislav Petkov June 10, 2020, 1:46 p.m. UTC | #7
On Wed, Jun 10, 2020 at 01:12:09PM +0000, Luis Chamberlain wrote:
> On Wed, Jun 10, 2020 at 10:16:09AM +0200, Borislav Petkov wrote:
> > 
> > Also, I'm working on removing that homegrown get_builtin_firmware() and
> > use the one in the fw loader:
> > 
> > https://lkml.kernel.org/r/20200408094526.GC24663@zn.tnic
> 
> I would like to still encourage this, even with this patch in place,
> as I think it makes this a proper call, and reflects better how the
> firmware loader is used exactly.
> 
> FWIW, firmware loader will be changed soon to not be modular, and just
> built-in or disabled.

I don't mind doing the work but Herbert has a point - there's no need to
require a bunch of code for a trivial function.

What I could do in addition is move that trivial function into a
fw-specific header and have it defined unconditionally so that the
microcode loader can use it without needing the fw loader.

The testcases stuff then goes ontop.

Yes, no?
Luis Chamberlain June 10, 2020, 2:01 p.m. UTC | #8
On Wed, Jun 10, 2020 at 03:46:50PM +0200, Borislav Petkov wrote:
> On Wed, Jun 10, 2020 at 01:12:09PM +0000, Luis Chamberlain wrote:
> > On Wed, Jun 10, 2020 at 10:16:09AM +0200, Borislav Petkov wrote:
> > > 
> > > Also, I'm working on removing that homegrown get_builtin_firmware() and
> > > use the one in the fw loader:
> > > 
> > > https://lkml.kernel.org/r/20200408094526.GC24663@zn.tnic
> > 
> > I would like to still encourage this, even with this patch in place,
> > as I think it makes this a proper call, and reflects better how the
> > firmware loader is used exactly.
> > 
> > FWIW, firmware loader will be changed soon to not be modular, and just
> > built-in or disabled.
> 
> I don't mind doing the work but Herbert has a point - there's no need to
> require a bunch of code for a trivial function.
> 
> What I could do in addition is move that trivial function into a
> fw-specific header and have it defined unconditionally so that the
> microcode loader can use it without needing the fw loader.

Would just have to see this part. But sounds like a better place than
today.

> The testcases stuff then goes ontop.

Sure!

  Luis

Patch
diff mbox series

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 1d6104ea8af0..8aac7a65bfbb 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1296,7 +1296,6 @@  config MICROCODE
 	bool "CPU microcode loading support"
 	default y
 	depends on CPU_SUP_AMD || CPU_SUP_INTEL
-	select FW_LOADER
 	---help---
 	  If you say Y here, you will be able to update the microcode on
 	  Intel and AMD processors. The Intel support is for the IA32 family,
@@ -1318,7 +1317,6 @@  config MICROCODE_INTEL
 	bool "Intel microcode loading support"
 	depends on MICROCODE
 	default MICROCODE
-	select FW_LOADER
 	---help---
 	  This options enables microcode patch loading support for Intel
 	  processors.
@@ -1330,7 +1328,6 @@  config MICROCODE_INTEL
 config MICROCODE_AMD
 	bool "AMD microcode loading support"
 	depends on MICROCODE
-	select FW_LOADER
 	---help---
 	  If you select this option, microcode patch loading support for AMD
 	  processors will be enabled.
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 7019d4b2df0c..5524ea15b3df 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -145,7 +145,6 @@  extern struct builtin_fw __end_builtin_fw[];
 
 bool get_builtin_firmware(struct cpio_data *cd, const char *name)
 {
-#ifdef CONFIG_FW_LOADER
 	struct builtin_fw *b_fw;
 
 	for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
@@ -155,7 +154,6 @@  bool get_builtin_firmware(struct cpio_data *cd, const char *name)
 			return true;
 		}
 	}
-#endif
 	return false;
 }