[PATCH] x86/microcode: Do not select FW_LOADER

[PATCH] x86/microcode: Do not select FW_LOADER

[Herbert Xu]

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>

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

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Borislav Petkov]

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

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Herbert Xu]

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,
-- 
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

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Herbert Xu]

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,
-- 
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

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[v2 PATCH] x86/microcode: Do not select FW_LOADER

[Herbert Xu]

On Wed, Jun 10, 2020 at 12:48:45PM +0200, Borislav Petkov wrote:
>
> Why isn't *this* in your commit message?

Sure, here it is.

---8<---
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.

As almost everyone on x86 needs to enable MICROCODE this by extension
means that FW_LOADER is always built into the kernel even if nothing
uses it.  The FW_LOADER system is about two thousand lines long and
contains user-space facing interfaces that could potentially provide
an entry point into the kernel (or beyond).

This patch removes the unnecessary select of FW_LOADER by MICROCODE.
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

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Luis Chamberlain]

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

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Do not select FW_LOADER

[Luis Chamberlain]

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

[tip: x86/microcode] x86/microcode: Do not select FW_LOADER

[tip-bot2 for Herbert Xu]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     c8a59a4d8e3c9e609fa915e39c3628c6dd08aeea
Gitweb:        https://git.kernel.org/tip/c8a59a4d8e3c9e609fa915e39c3628c6dd08aeea
Author:        Herbert Xu <herbert@gondor.apana.org.au>
AuthorDate:    Wed, 10 Jun 2020 21:05:13 +10:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Mon, 15 Jun 2020 11:59:19 +02:00

x86/microcode: Do not select FW_LOADER

The x86 microcode support works just fine without FW_LOADER. In fact,
these days most people load microcode early during boot so FW_LOADER
never gets into the picture anyway.

As almost everyone on x86 needs to enable MICROCODE, this by extension
means that FW_LOADER is always built into the kernel even if nothing
uses it. The FW_LOADER system is about two thousand lines long and
contains user-space facing interfaces that could potentially provide an
entry point into the kernel (or beyond).

Remove the unnecessary select of FW_LOADER by MICROCODE. People who need
the FW_LOADER capability can still enable it.

 [ bp: Massage a bit. ]

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20200610042911.GA20058@gondor.apana.org.au
---
 arch/x86/Kconfig                     | 3 ---
 arch/x86/kernel/cpu/microcode/core.c | 2 --
 2 files changed, 5 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 6a0cc52..5c44eac 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1292,7 +1292,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,
@@ -1314,7 +1313,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.
@@ -1326,7 +1324,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 baec68b..ec6f041 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;
 }