[PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[Nayna Jain]

Every time a new architecture defines the IMA architecture specific
functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA
include file needs to be updated. To avoid this "noise", this patch
defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing
the different architectures to select it.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Philipp Rudo <prudo@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
v2:
* Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael for
discussing the fix.

 arch/powerpc/Kconfig           | 1 +
 arch/s390/Kconfig              | 1 +
 arch/x86/Kconfig               | 1 +
 include/linux/ima.h            | 3 +--
 security/integrity/ima/Kconfig | 9 +++++++++
 5 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 497b7d0b2d7e..a5cfde432983 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -979,6 +979,7 @@ config PPC_SECURE_BOOT
 	bool
 	depends on PPC_POWERNV
 	depends on IMA_ARCH_POLICY
+	select IMA_SECURE_AND_OR_TRUSTED_BOOT
 	help
 	  Systems with firmware secure boot enabled need to define security
 	  policies to extend secure boot to the OS. This config allows a user
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 8abe77536d9d..4a502fbcb800 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -195,6 +195,7 @@ config S390
 	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
 	select SWIOTLB
 	select GENERIC_ALLOCATOR
+	select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY
 
 
 config SCHED_OMIT_FRAME_POINTER
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index beea77046f9b..7f5bfaf0cbd2 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -230,6 +230,7 @@ config X86
 	select VIRT_TO_BUS
 	select X86_FEATURE_NAMES		if PROC_FS
 	select PROC_PID_ARCH_STATUS		if PROC_FS
+	select IMA_SECURE_AND_OR_TRUSTED_BOOT	if EFI && IMA_ARCH_POLICY
 
 config INSTRUCTION_DECODER
 	def_bool y
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 1659217e9b60..aefe758f4466 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int size);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
-#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
-	|| defined(CONFIG_PPC_SECURE_BOOT)
+#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
 extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 3f3ee4e2eb0d..d17972aa413a 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
 	depends on IMA_MEASURE_ASYMMETRIC_KEYS
 	depends on SYSTEM_TRUSTED_KEYRING
 	default y
+
+config IMA_SECURE_AND_OR_TRUSTED_BOOT
+	bool
+	depends on IMA
+	depends on IMA_ARCH_POLICY
+	default n
+	help
+	   This option is selected by architectures to enable secure and/or
+	   trusted boot based on IMA runtime policies.
-- 
2.13.6

Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[Ard Biesheuvel]

On Wed, 4 Mar 2020 at 03:34, Nayna Jain <nayna@linux.ibm.com> wrote:
>
> Every time a new architecture defines the IMA architecture specific
> functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA
> include file needs to be updated. To avoid this "noise", this patch
> defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing
> the different architectures to select it.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Philipp Rudo <prudo@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

for the x86 bits, but I'm not an x86 maintainer. Also, you may need to
split this if you want to permit arch maintainers to pick up their
parts individually.


> ---
> v2:
> * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael for
> discussing the fix.
>
>  arch/powerpc/Kconfig           | 1 +
>  arch/s390/Kconfig              | 1 +
>  arch/x86/Kconfig               | 1 +
>  include/linux/ima.h            | 3 +--
>  security/integrity/ima/Kconfig | 9 +++++++++
>  5 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 497b7d0b2d7e..a5cfde432983 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT
>         bool
>         depends on PPC_POWERNV
>         depends on IMA_ARCH_POLICY
> +       select IMA_SECURE_AND_OR_TRUSTED_BOOT
>         help
>           Systems with firmware secure boot enabled need to define security
>           policies to extend secure boot to the OS. This config allows a user
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 8abe77536d9d..4a502fbcb800 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -195,6 +195,7 @@ config S390
>         select ARCH_HAS_FORCE_DMA_UNENCRYPTED
>         select SWIOTLB
>         select GENERIC_ALLOCATOR
> +       select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY
>
>
>  config SCHED_OMIT_FRAME_POINTER
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index beea77046f9b..7f5bfaf0cbd2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -230,6 +230,7 @@ config X86
>         select VIRT_TO_BUS
>         select X86_FEATURE_NAMES                if PROC_FS
>         select PROC_PID_ARCH_STATUS             if PROC_FS
> +       select IMA_SECURE_AND_OR_TRUSTED_BOOT   if EFI && IMA_ARCH_POLICY
>
>  config INSTRUCTION_DECODER
>         def_bool y
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 1659217e9b60..aefe758f4466 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int size);
>  extern void ima_add_kexec_buffer(struct kimage *image);
>  #endif
>
> -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
> -       || defined(CONFIG_PPC_SECURE_BOOT)
> +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
>  extern bool arch_ima_get_secureboot(void);
>  extern const char * const *arch_get_ima_policy(void);
>  #else
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 3f3ee4e2eb0d..d17972aa413a 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
>         depends on IMA_MEASURE_ASYMMETRIC_KEYS
>         depends on SYSTEM_TRUSTED_KEYRING
>         default y
> +
> +config IMA_SECURE_AND_OR_TRUSTED_BOOT
> +       bool
> +       depends on IMA
> +       depends on IMA_ARCH_POLICY

Doesn't the latter already depend on the former?

> +       default n
> +       help
> +          This option is selected by architectures to enable secure and/or
> +          trusted boot based on IMA runtime policies.
> --
> 2.13.6
>

Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[James Bottomley]

On Tue, 2020-03-03 at 21:33 -0500, Nayna Jain wrote:
> Every time a new architecture defines the IMA architecture specific
> functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the
> IMA
> include file needs to be updated. To avoid this "noise", this patch
> defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option,
> allowing
> the different architectures to select it.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Philipp Rudo <prudo@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v2:
> * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and
> Michael for
> discussing the fix.
> 
>  arch/powerpc/Kconfig           | 1 +
>  arch/s390/Kconfig              | 1 +
>  arch/x86/Kconfig               | 1 +
>  include/linux/ima.h            | 3 +--
>  security/integrity/ima/Kconfig | 9 +++++++++
>  5 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 497b7d0b2d7e..a5cfde432983 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT
>  	bool
>  	depends on PPC_POWERNV
>  	depends on IMA_ARCH_POLICY
> +	select IMA_SECURE_AND_OR_TRUSTED_BOOT
>  	help
>  	  Systems with firmware secure boot enabled need to define
> security
>  	  policies to extend secure boot to the OS. This config
> allows a user
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 8abe77536d9d..4a502fbcb800 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -195,6 +195,7 @@ config S390
>  	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
>  	select SWIOTLB
>  	select GENERIC_ALLOCATOR
> +	select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY
>  
>  
>  config SCHED_OMIT_FRAME_POINTER
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index beea77046f9b..7f5bfaf0cbd2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -230,6 +230,7 @@ config X86
>  	select VIRT_TO_BUS
>  	select X86_FEATURE_NAMES		if PROC_FS
>  	select PROC_PID_ARCH_STATUS		if PROC_FS
> +	select IMA_SECURE_AND_OR_TRUSTED_BOOT	if EFI &&
> IMA_ARCH_POLICY
>  
>  config INSTRUCTION_DECODER
>  	def_bool y
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 1659217e9b60..aefe758f4466 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int
> size);
>  extern void ima_add_kexec_buffer(struct kimage *image);
>  #endif
>  
> -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) ||
> defined(CONFIG_S390) \
> -	|| defined(CONFIG_PPC_SECURE_BOOT)
> +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
>  extern bool arch_ima_get_secureboot(void);
>  extern const char * const *arch_get_ima_policy(void);
>  #else
> diff --git a/security/integrity/ima/Kconfig
> b/security/integrity/ima/Kconfig
> index 3f3ee4e2eb0d..d17972aa413a 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
>  	depends on IMA_MEASURE_ASYMMETRIC_KEYS
>  	depends on SYSTEM_TRUSTED_KEYRING
>  	default y
> +
> +config IMA_SECURE_AND_OR_TRUSTED_BOOT
> +	bool
> +	depends on IMA
> +	depends on IMA_ARCH_POLICY
> +	default n

You can't do this: a symbol designed to be selected can't depend on
other symbols because Kconfig doesn't see the dependencies during
select.  We even have a doc for this now:

Documentation/kbuild/Kconfig.select-break

The only way to get this to work would be to have the long name symbol
select both IMA and IMA_ARCH_POLICY, which doesn't seem to be what you
want either.

Looking at what you're trying to do, I think making the symbol
independent of IMA and IMA_ARCH_POLICY is the correct thing, then
enforce the dependencies inside the outer #ifdef, but I haven't looked
deeply at the code.

James

Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[Mimi Zohar]

On Tue, 2020-03-03 at 23:43 -0800, James Bottomley wrote:
> On Tue, 2020-03-03 at 21:33 -0500, Nayna Jain wrote:

> > diff --git a/security/integrity/ima/Kconfig
> > b/security/integrity/ima/Kconfig
> > index 3f3ee4e2eb0d..d17972aa413a 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
> >  	depends on IMA_MEASURE_ASYMMETRIC_KEYS
> >  	depends on SYSTEM_TRUSTED_KEYRING
> >  	default y
> > +
> > +config IMA_SECURE_AND_OR_TRUSTED_BOOT
> > +	bool
> > +	depends on IMA
> > +	depends on IMA_ARCH_POLICY
> > +	default n
> 
> You can't do this: a symbol designed to be selected can't depend on
> other symbols because Kconfig doesn't see the dependencies during
> select.  We even have a doc for this now:
> 
> Documentation/kbuild/Kconfig.select-break

The document is discussing a circular dependency, where C selects B.
 IMA_SECURE_AND_OR_TRUSTED_BOOT is not selecting anything, but is
being selected.  All of the Kconfig's are now dependent on
IMA_ARCH_POLICY being enabled before selecting
IMA_SECURE_AND_OR_TRUSTED_BOOT.

As Ard pointed out, both IMA and IMA_ARCH_POLICY are not needed, as
IMA_ARCH_POLICY is already dependent on IMA.

Mimi

Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[Mimi Zohar]

[Cc'ing Thomas Gleixner and x86 mailing list]

On Wed, 2020-03-04 at 08:14 +0100, Ard Biesheuvel wrote:
> On Wed, 4 Mar 2020 at 03:34, Nayna Jain <nayna@linux.ibm.com> wrote:
> >
> > Every time a new architecture defines the IMA architecture specific
> > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA
> > include file needs to be updated. To avoid this "noise", this patch
> > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing
> > the different architectures to select it.
> >
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Philipp Rudo <prudo@linux.ibm.com>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> 
> Acked-by: Ard Biesheuvel <ardb@kernel.org>

Thanks, Ard.
> 
> for the x86 bits, but I'm not an x86 maintainer. Also, you may need to
> split this if you want to permit arch maintainers to pick up their
> parts individually.

Michael, Philipp, Thomas, do you prefer separate patches?

> 
> > ---
> > v2:
> > * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael for
> > discussing the fix.
> >
> >  arch/powerpc/Kconfig           | 1 +
> >  arch/s390/Kconfig              | 1 +
> >  arch/x86/Kconfig               | 1 +
> >  include/linux/ima.h            | 3 +--
> >  security/integrity/ima/Kconfig | 9 +++++++++
> >  5 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 497b7d0b2d7e..a5cfde432983 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT
> >         bool
> >         depends on PPC_POWERNV
> >         depends on IMA_ARCH_POLICY
> > +       select IMA_SECURE_AND_OR_TRUSTED_BOOT
> >         help
> >           Systems with firmware secure boot enabled need to define security
> >           policies to extend secure boot to the OS. This config allows a user
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 8abe77536d9d..4a502fbcb800 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -195,6 +195,7 @@ config S390
> >         select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> >         select SWIOTLB
> >         select GENERIC_ALLOCATOR
> > +       select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY
> >
> >
> >  config SCHED_OMIT_FRAME_POINTER
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index beea77046f9b..7f5bfaf0cbd2 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -230,6 +230,7 @@ config X86
> >         select VIRT_TO_BUS
> >         select X86_FEATURE_NAMES                if PROC_FS
> >         select PROC_PID_ARCH_STATUS             if PROC_FS
> > +       select IMA_SECURE_AND_OR_TRUSTED_BOOT   if EFI && IMA_ARCH_POLICY
> >
> >  config INSTRUCTION_DECODER
> >         def_bool y
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 1659217e9b60..aefe758f4466 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int size);
> >  extern void ima_add_kexec_buffer(struct kimage *image);
> >  #endif
> >
> > -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
> > -       || defined(CONFIG_PPC_SECURE_BOOT)
> > +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
> >  extern bool arch_ima_get_secureboot(void);
> >  extern const char * const *arch_get_ima_policy(void);
> >  #else
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index 3f3ee4e2eb0d..d17972aa413a 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
> >         depends on IMA_MEASURE_ASYMMETRIC_KEYS
> >         depends on SYSTEM_TRUSTED_KEYRING
> >         default y
> > +
> > +config IMA_SECURE_AND_OR_TRUSTED_BOOT
> > +       bool
> > +       depends on IMA
> > +       depends on IMA_ARCH_POLICY
> 
> Doesn't the latter already depend on the former?

Yes, there's no need for the first.

Mimi
> 
> > +       default n
> > +       help
> > +          This option is selected by architectures to enable secure and/or
> > +          trusted boot based on IMA runtime policies.
> > --
> > 2.13.6
> >

Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[Philipp Rudo]

On Wed, 04 Mar 2020 07:55:38 -0500
Mimi Zohar <zohar@linux.ibm.com> wrote:

> [Cc'ing Thomas Gleixner and x86 mailing list]
> 
> On Wed, 2020-03-04 at 08:14 +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Mar 2020 at 03:34, Nayna Jain <nayna@linux.ibm.com> wrote:  
> > >
> > > Every time a new architecture defines the IMA architecture specific
> > > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA
> > > include file needs to be updated. To avoid this "noise", this patch
> > > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing
> > > the different architectures to select it.
> > >
> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > Cc: Philipp Rudo <prudo@linux.ibm.com>
> > > Cc: Michael Ellerman <mpe@ellerman.id.au>  
> > 
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>  
> 
> Thanks, Ard.
> > 
> > for the x86 bits, but I'm not an x86 maintainer. Also, you may need to
> > split this if you want to permit arch maintainers to pick up their
> > parts individually.  
> 
> Michael, Philipp, Thomas, do you prefer separate patches?

I don't think splitting this patch makes sense. Otherwise you would break the
build for all architectures until they picked up their line of code.

I'm fine with the patch as is.

Thanks
Philipp

> >   
> > > ---
> > > v2:
> > > * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael for
> > > discussing the fix.
> > >
> > >  arch/powerpc/Kconfig           | 1 +
> > >  arch/s390/Kconfig              | 1 +
> > >  arch/x86/Kconfig               | 1 +
> > >  include/linux/ima.h            | 3 +--
> > >  security/integrity/ima/Kconfig | 9 +++++++++
> > >  5 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > > index 497b7d0b2d7e..a5cfde432983 100644
> > > --- a/arch/powerpc/Kconfig
> > > +++ b/arch/powerpc/Kconfig
> > > @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT
> > >         bool
> > >         depends on PPC_POWERNV
> > >         depends on IMA_ARCH_POLICY
> > > +       select IMA_SECURE_AND_OR_TRUSTED_BOOT
> > >         help
> > >           Systems with firmware secure boot enabled need to define security
> > >           policies to extend secure boot to the OS. This config allows a user
> > > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > > index 8abe77536d9d..4a502fbcb800 100644
> > > --- a/arch/s390/Kconfig
> > > +++ b/arch/s390/Kconfig
> > > @@ -195,6 +195,7 @@ config S390
> > >         select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> > >         select SWIOTLB
> > >         select GENERIC_ALLOCATOR
> > > +       select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY
> > >
> > >
> > >  config SCHED_OMIT_FRAME_POINTER
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index beea77046f9b..7f5bfaf0cbd2 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -230,6 +230,7 @@ config X86
> > >         select VIRT_TO_BUS
> > >         select X86_FEATURE_NAMES                if PROC_FS
> > >         select PROC_PID_ARCH_STATUS             if PROC_FS
> > > +       select IMA_SECURE_AND_OR_TRUSTED_BOOT   if EFI && IMA_ARCH_POLICY
> > >
> > >  config INSTRUCTION_DECODER
> > >         def_bool y
> > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > index 1659217e9b60..aefe758f4466 100644
> > > --- a/include/linux/ima.h
> > > +++ b/include/linux/ima.h
> > > @@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int size);
> > >  extern void ima_add_kexec_buffer(struct kimage *image);
> > >  #endif
> > >
> > > -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
> > > -       || defined(CONFIG_PPC_SECURE_BOOT)
> > > +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
> > >  extern bool arch_ima_get_secureboot(void);
> > >  extern const char * const *arch_get_ima_policy(void);
> > >  #else
> > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > > index 3f3ee4e2eb0d..d17972aa413a 100644
> > > --- a/security/integrity/ima/Kconfig
> > > +++ b/security/integrity/ima/Kconfig
> > > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
> > >         depends on IMA_MEASURE_ASYMMETRIC_KEYS
> > >         depends on SYSTEM_TRUSTED_KEYRING
> > >         default y
> > > +
> > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT
> > > +       bool
> > > +       depends on IMA
> > > +       depends on IMA_ARCH_POLICY  
> > 
> > Doesn't the latter already depend on the former?  
> 
> Yes, there's no need for the first.
> 
> Mimi
> >   
> > > +       default n
> > > +       help
> > > +          This option is selected by architectures to enable secure and/or
> > > +          trusted boot based on IMA runtime policies.
> > > --
> > > 2.13.6
> > >  
> 

Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[James Bottomley]

On Wed, 2020-03-04 at 07:35 -0500, Mimi Zohar wrote:
> On Tue, 2020-03-03 at 23:43 -0800, James Bottomley wrote:
> > On Tue, 2020-03-03 at 21:33 -0500, Nayna Jain wrote:
> > > diff --git a/security/integrity/ima/Kconfig
> > > b/security/integrity/ima/Kconfig
> > > index 3f3ee4e2eb0d..d17972aa413a 100644
> > > --- a/security/integrity/ima/Kconfig
> > > +++ b/security/integrity/ima/Kconfig
> > > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
> > >  	depends on IMA_MEASURE_ASYMMETRIC_KEYS
> > >  	depends on SYSTEM_TRUSTED_KEYRING
> > >  	default y
> > > +
> > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT
> > > +	bool
> > > +	depends on IMA
> > > +	depends on IMA_ARCH_POLICY
> > > +	default n
> > 
> > You can't do this: a symbol designed to be selected can't depend on
> > other symbols because Kconfig doesn't see the dependencies during
> > select.  We even have a doc for this now:
> > 
> > Documentation/kbuild/Kconfig.select-break
> 
> The document is discussing a circular dependency, where C selects B.
>  IMA_SECURE_AND_OR_TRUSTED_BOOT is not selecting anything, but is
> being selected.  All of the Kconfig's are now dependent on
> IMA_ARCH_POLICY being enabled before selecting
> IMA_SECURE_AND_OR_TRUSTED_BOOT.
> 
> As Ard pointed out, both IMA and IMA_ARCH_POLICY are not needed, as
> IMA_ARCH_POLICY is already dependent on IMA.

Then removing them is fine, if they're not necessary ... you just can't
 select a symbol with dependencies because the two Kconfig mechanisms
don't mix.

James

Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies

[Michael Ellerman]

James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> On Wed, 2020-03-04 at 07:35 -0500, Mimi Zohar wrote:
>> On Tue, 2020-03-03 at 23:43 -0800, James Bottomley wrote:
>> > On Tue, 2020-03-03 at 21:33 -0500, Nayna Jain wrote:
>> > > diff --git a/security/integrity/ima/Kconfig
>> > > b/security/integrity/ima/Kconfig
>> > > index 3f3ee4e2eb0d..d17972aa413a 100644
>> > > --- a/security/integrity/ima/Kconfig
>> > > +++ b/security/integrity/ima/Kconfig
>> > > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
>> > >  	depends on IMA_MEASURE_ASYMMETRIC_KEYS
>> > >  	depends on SYSTEM_TRUSTED_KEYRING
>> > >  	default y
>> > > +
>> > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT
>> > > +	bool
>> > > +	depends on IMA
>> > > +	depends on IMA_ARCH_POLICY
>> > > +	default n
>> > 
>> > You can't do this: a symbol designed to be selected can't depend on
>> > other symbols because Kconfig doesn't see the dependencies during
>> > select.  We even have a doc for this now:
>> > 
>> > Documentation/kbuild/Kconfig.select-break
>> 
>> The document is discussing a circular dependency, where C selects B.
>>  IMA_SECURE_AND_OR_TRUSTED_BOOT is not selecting anything, but is
>> being selected.  All of the Kconfig's are now dependent on
>> IMA_ARCH_POLICY being enabled before selecting
>> IMA_SECURE_AND_OR_TRUSTED_BOOT.
>> 
>> As Ard pointed out, both IMA and IMA_ARCH_POLICY are not needed, as
>> IMA_ARCH_POLICY is already dependent on IMA.
>
> Then removing them is fine, if they're not necessary ... you just can't
>  select a symbol with dependencies because the two Kconfig mechanisms
> don't mix.

You can safely select something if the selector has the same or stricter
set of dependencies than the selectee. And in this case that's true.

config IMA_SECURE_AND_OR_TRUSTED_BOOT
       bool
       depends on IMA
       depends on IMA_ARCH_POLICY

powerpc:
        depends on IMA_ARCH_POLICY
        select IMA_SECURE_AND_OR_TRUSTED_BOOT

s390: select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY

x86: select IMA_SECURE_AND_OR_TRUSTED_BOOT   if EFI && IMA_ARCH_POLICY


But that's not to say it's the best solution, because you have to ensure
the arch code has the right set of dependencies.

I think this is actually a perfect case for using imply. We want the
arch code to indicate it wants IMA_SECURE_..., but only if all the IMA
related dependencies are met.

I think the patch below should work.

For example:

$ grep PPC_SECURE_BOOT .config
CONFIG_PPC_SECURE_BOOT=y
$ ./scripts/config -d CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
$ grep IMA_SECURE .config
# CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT is not set
$ make oldconfig
scripts/kconfig/conf  --oldconfig Kconfig
#
# configuration written to .config
#
$ grep IMA_SECURE .config
CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=y

$ ./scripts/config -d CONFIG_IMA_ARCH_POLICY
$ grep -e IMA_ARCH_POLICY -e IMA_SECURE .config
# CONFIG_IMA_ARCH_POLICY is not set
CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=y
$ make olddefconfig
scripts/kconfig/conf  --olddefconfig Kconfig
#
# configuration written to .config
#
$ grep -e IMA_ARCH_POLICY -e IMA_SECURE .config
# CONFIG_IMA_ARCH_POLICY is not set
$ 


cheers



diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 497b7d0b2d7e..5b9f1cba2a44 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -979,6 +979,7 @@ config PPC_SECURE_BOOT
        bool
        depends on PPC_POWERNV
        depends on IMA_ARCH_POLICY
+       imply IMA_SECURE_AND_OR_TRUSTED_BOOT
        help
          Systems with firmware secure boot enabled need to define security
          policies to extend secure boot to the OS. This config allows a user
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 8abe77536d9d..59c216af6264 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -195,6 +195,7 @@ config S390
        select ARCH_HAS_FORCE_DMA_UNENCRYPTED
        select SWIOTLB
        select GENERIC_ALLOCATOR
+       imply IMA_SECURE_AND_OR_TRUSTED_BOOT
 
 
 config SCHED_OMIT_FRAME_POINTER
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index beea77046f9b..92204a486d97 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -230,6 +230,7 @@ config X86
        select VIRT_TO_BUS
        select X86_FEATURE_NAMES                if PROC_FS
        select PROC_PID_ARCH_STATUS             if PROC_FS
+       imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
 
 config INSTRUCTION_DECODER
        def_bool y
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 1659217e9b60..aefe758f4466 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int size);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
-#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
-       || defined(CONFIG_PPC_SECURE_BOOT)
+#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
 extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 3f3ee4e2eb0d..5ba4ae040fd8 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -327,3 +327,10 @@ config IMA_QUEUE_EARLY_BOOT_KEYS
        depends on IMA_MEASURE_ASYMMETRIC_KEYS
        depends on SYSTEM_TRUSTED_KEYRING
        default y
+
+config IMA_SECURE_AND_OR_TRUSTED_BOOT
+       bool
+       depends on IMA_ARCH_POLICY
+       help
+          This option is selected by architectures to enable secure and/or
+          trusted boot based on IMA runtime policies.