x86/microcode: Do not select FW_LOADER

1254416 diff mbox series

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>

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

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,

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.

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,

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.

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

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?

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