[PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The 'init_keyring' variable actually just gave the value of
CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
directly instead.  No change in behavior; this just simplifies the code.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/integrity/digsig.c    | 11 ++---------
 security/integrity/integrity.h |  9 +++++----
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 879396fa3be0..9e6adbd1ad42 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -37,12 +37,6 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
 	"_module",
 };
 
-#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
-static bool init_keyring __initdata = true;
-#else
-static bool init_keyring __initdata;
-#endif
-
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
 #define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
 #else
@@ -79,15 +73,13 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 	return -EOPNOTSUPP;
 }
 
+#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
 int __init integrity_init_keyring(const unsigned int id)
 {
 	const struct cred *cred = current_cred();
 	struct key_restriction *restriction;
 	int err = 0;
 
-	if (!init_keyring)
-		return 0;
-
 	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
 	if (!restriction)
 		return -ENOMEM;
@@ -109,6 +101,7 @@ int __init integrity_init_keyring(const unsigned int id)
 	}
 	return err;
 }
+#endif /* CONFIG_INTEGRITY_TRUSTED_KEYRING */
 
 int __init integrity_load_x509(const unsigned int id, const char *path)
 {
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e60473b13a8d..37ab908cfb6e 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -147,26 +147,27 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 extern struct dentry *integrity_dir;
 
 #ifdef CONFIG_INTEGRITY_SIGNATURE
-
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 			    const char *digest, int digestlen);
 
-int __init integrity_init_keyring(const unsigned int id);
 int __init integrity_load_x509(const unsigned int id, const char *path);
 #else
-
 static inline int integrity_digsig_verify(const unsigned int id,
 					  const char *sig, int siglen,
 					  const char *digest, int digestlen)
 {
 	return -EOPNOTSUPP;
 }
+#endif /* CONFIG_INTEGRITY_SIGNATURE */
 
+#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
+int __init integrity_init_keyring(const unsigned int id);
+#else
 static inline int integrity_init_keyring(const unsigned int id)
 {
 	return 0;
 }
-#endif /* CONFIG_INTEGRITY_SIGNATURE */
+#endif
 
 #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
 int asymmetric_verify(struct key *keyring, const char *sig,
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Mimi Zohar]

On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> The 'init_keyring' variable actually just gave the value of
> CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> directly instead.  No change in behavior; this just simplifies the code.

We try to minimize as much as possible "ifdefs" in C code.  This
change is moving in the wrong direction.

Mimi

> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  security/integrity/digsig.c    | 11 ++---------
>  security/integrity/integrity.h |  9 +++++----
>  2 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 879396fa3be0..9e6adbd1ad42 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -37,12 +37,6 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
>  	"_module",
>  };
> 
> -#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
> -static bool init_keyring __initdata = true;
> -#else
> -static bool init_keyring __initdata;
> -#endif
> -
>  #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>  #define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
>  #else
> @@ -79,15 +73,13 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>  	return -EOPNOTSUPP;
>  }
> 
> +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
>  int __init integrity_init_keyring(const unsigned int id)
>  {
>  	const struct cred *cred = current_cred();
>  	struct key_restriction *restriction;
>  	int err = 0;
> 
> -	if (!init_keyring)
> -		return 0;
> -
>  	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
>  	if (!restriction)
>  		return -ENOMEM;
> @@ -109,6 +101,7 @@ int __init integrity_init_keyring(const unsigned int id)
>  	}
>  	return err;
>  }
> +#endif /* CONFIG_INTEGRITY_TRUSTED_KEYRING */
> 
>  int __init integrity_load_x509(const unsigned int id, const char *path)
>  {
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index e60473b13a8d..37ab908cfb6e 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -147,26 +147,27 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>  extern struct dentry *integrity_dir;
> 
>  #ifdef CONFIG_INTEGRITY_SIGNATURE
> -
>  int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>  			    const char *digest, int digestlen);
> 
> -int __init integrity_init_keyring(const unsigned int id);
>  int __init integrity_load_x509(const unsigned int id, const char *path);
>  #else
> -
>  static inline int integrity_digsig_verify(const unsigned int id,
>  					  const char *sig, int siglen,
>  					  const char *digest, int digestlen)
>  {
>  	return -EOPNOTSUPP;
>  }
> +#endif /* CONFIG_INTEGRITY_SIGNATURE */
> 
> +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
> +int __init integrity_init_keyring(const unsigned int id);
> +#else
>  static inline int integrity_init_keyring(const unsigned int id)
>  {
>  	return 0;
>  }
> -#endif /* CONFIG_INTEGRITY_SIGNATURE */
> +#endif
> 
>  #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
>  int asymmetric_verify(struct key *keyring, const char *sig,

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Eric Biggers]

On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > The 'init_keyring' variable actually just gave the value of
> > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > directly instead.  No change in behavior; this just simplifies the code.
> 
> We try to minimize as much as possible "ifdefs" in C code.  This
> change is moving in the wrong direction.
> 
> Mimi

So your preferred approach is to store the values of Kconfig options in
variables?  That defeats much of the point of having Kconfig options...

- Eric

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Mimi Zohar]

On Fri, 2018-09-21 at 11:54 -0700, Eric Biggers wrote:
> On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> > On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > The 'init_keyring' variable actually just gave the value of
> > > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > > directly instead.  No change in behavior; this just simplifies the code.
> > 
> > We try to minimize as much as possible "ifdefs" in C code.  This
> > change is moving in the wrong direction.
> 
> So your preferred approach is to store the values of Kconfig options in
> variables?  That defeats much of the point of having Kconfig options...

No, I prefer using "ifdefs" in include files, not C code, and defining
stub functions.

Mimi

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Eric Biggers]

On Fri, Sep 21, 2018 at 03:02:14PM -0400, Mimi Zohar wrote:
> On Fri, 2018-09-21 at 11:54 -0700, Eric Biggers wrote:
> > On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> > > On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > The 'init_keyring' variable actually just gave the value of
> > > > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > > > directly instead.  No change in behavior; this just simplifies the code.
> > > 
> > > We try to minimize as much as possible "ifdefs" in C code.  This
> > > change is moving in the wrong direction.
> > 
> > So your preferred approach is to store the values of Kconfig options in
> > variables?  That defeats much of the point of having Kconfig options...
> 
> No, I prefer using "ifdefs" in include files, not C code, and defining
> stub functions.
> 
> Mimi
> 

integrity_init_keyring() is already stubbed out in a header.  What are you
suggesting, exactly?

- Eric

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Mimi Zohar]

On Fri, 2018-09-21 at 12:33 -0700, Eric Biggers wrote:
> On Fri, Sep 21, 2018 at 03:02:14PM -0400, Mimi Zohar wrote:
> > On Fri, 2018-09-21 at 11:54 -0700, Eric Biggers wrote:
> > > On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> > > > On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > > > > From: Eric Biggers <ebiggers@google.com>
> > > > > 
> > > > > The 'init_keyring' variable actually just gave the value of
> > > > > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > > > > directly instead.  No change in behavior; this just simplifies the code.
> > > > 
> > > > We try to minimize as much as possible "ifdefs" in C code.  This
> > > > change is moving in the wrong direction.
> > > 
> > > So your preferred approach is to store the values of Kconfig options in
> > > variables?  That defeats much of the point of having Kconfig options...
> > 
> > No, I prefer using "ifdefs" in include files, not C code, and defining
> > stub functions.
> > 
> > Mimi
> > 
> 
> integrity_init_keyring() is already stubbed out in a header.  What are you
> suggesting, exactly?

Refer to section "20) Conditional Compilation" of
Documentation/process/coding-style.rst.

Mimi

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Eric Biggers]

On Fri, Sep 21, 2018 at 03:55:33PM -0400, Mimi Zohar wrote:
> On Fri, 2018-09-21 at 12:33 -0700, Eric Biggers wrote:
> > On Fri, Sep 21, 2018 at 03:02:14PM -0400, Mimi Zohar wrote:
> > > On Fri, 2018-09-21 at 11:54 -0700, Eric Biggers wrote:
> > > > On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> > > > > On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > > > > > From: Eric Biggers <ebiggers@google.com>
> > > > > > 
> > > > > > The 'init_keyring' variable actually just gave the value of
> > > > > > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > > > > > directly instead.  No change in behavior; this just simplifies the code.
> > > > > 
> > > > > We try to minimize as much as possible "ifdefs" in C code.  This
> > > > > change is moving in the wrong direction.
> > > > 
> > > > So your preferred approach is to store the values of Kconfig options in
> > > > variables?  That defeats much of the point of having Kconfig options...
> > > 
> > > No, I prefer using "ifdefs" in include files, not C code, and defining
> > > stub functions.
> > > 
> > > Mimi
> > > 
> > 
> > integrity_init_keyring() is already stubbed out in a header.  What are you
> > suggesting, exactly?
> 
> Refer to section "20) Conditional Compilation" of
> Documentation/process/coding-style.rst.
> 
> Mimi
> 

I'm already familiar with that.  Unfortunately, you haven't clearly indicated
what alternative you prefer, and it's unclear whether you've even read my patch,
given that you're apparently saying to define a stub function which actually
already exists.  Maybe you want the 'init_keyring' bool replaced
IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING)?  That doesn't really make sense
though, because integrity_init_keyring() is already stubbed out in some
configurations; it makes more sense to fix the condition for stubbing it out...

- Eric

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Mimi Zohar]

On Fri, 2018-09-21 at 13:13 -0700, Eric Biggers wrote:
> On Fri, Sep 21, 2018 at 03:55:33PM -0400, Mimi Zohar wrote:
> > On Fri, 2018-09-21 at 12:33 -0700, Eric Biggers wrote:
> > > On Fri, Sep 21, 2018 at 03:02:14PM -0400, Mimi Zohar wrote:
> > > > On Fri, 2018-09-21 at 11:54 -0700, Eric Biggers wrote:
> > > > > On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> > > > > > On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > > > > > > From: Eric Biggers <ebiggers@google.com>
> > > > > > > 
> > > > > > > The 'init_keyring' variable actually just gave the value of
> > > > > > > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > > > > > > directly instead.  No change in behavior; this just simplifies the code.
> > > > > > 
> > > > > > We try to minimize as much as possible "ifdefs" in C code.  This
> > > > > > change is moving in the wrong direction.
> > > > > 
> > > > > So your preferred approach is to store the values of Kconfig options in
> > > > > variables?  That defeats much of the point of having Kconfig options...
> > > > 
> > > > No, I prefer using "ifdefs" in include files, not C code, and defining
> > > > stub functions.
> > > > 
> > > > Mimi
> > > > 
> > > 
> > > integrity_init_keyring() is already stubbed out in a header.  What are you
> > > suggesting, exactly?
> > 
> > Refer to section "20) Conditional Compilation" of
> > Documentation/process/coding-style.rst.
> > 
> > Mimi
> > 
> 
> I'm already familiar with that.  Unfortunately, you haven't clearly indicated
> what alternative you prefer, and it's unclear whether you've even read my patch,
> given that you're apparently saying to define a stub function which actually
> already exists.  Maybe you want the 'init_keyring' bool replaced
> IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING)?  That doesn't really make sense
> though, because integrity_init_keyring() is already stubbed out in some
> configurations; it makes more sense to fix the condition for stubbing it out...

I read your patch and commented that "ifdefs" don't belong in .c
files, as the above referenced doc says.  My comment on using ifdefs
for defining stubs was simply an example of how to get around using
ifdefs in C.

The doc seems to be suggesting to use IS_ENABLED() to set a flag,
which is what the existing code does.  Replacing the existing code
with IS_ENABLE() would be fine, but seems unnecessary.

I don't see a need to change the existing code to add more ifdefs in
.c files.

Mimi

Re: [PATCH] security/integrity: remove unnecessary 'init_keyring' variable

[Eric Biggers]

On Fri, Sep 21, 2018 at 04:42:26PM -0400, Mimi Zohar wrote:
> On Fri, 2018-09-21 at 13:13 -0700, Eric Biggers wrote:
> > On Fri, Sep 21, 2018 at 03:55:33PM -0400, Mimi Zohar wrote:
> > > On Fri, 2018-09-21 at 12:33 -0700, Eric Biggers wrote:
> > > > On Fri, Sep 21, 2018 at 03:02:14PM -0400, Mimi Zohar wrote:
> > > > > On Fri, 2018-09-21 at 11:54 -0700, Eric Biggers wrote:
> > > > > > On Fri, Sep 21, 2018 at 02:42:38PM -0400, Mimi Zohar wrote:
> > > > > > > On Fri, 2018-09-07 at 13:25 -0700, Eric Biggers wrote:
> > > > > > > > From: Eric Biggers <ebiggers@google.com>
> > > > > > > > 
> > > > > > > > The 'init_keyring' variable actually just gave the value of
> > > > > > > > CONFIG_INTEGRITY_TRUSTED_KEYRING.  We should check the config option
> > > > > > > > directly instead.  No change in behavior; this just simplifies the code.
> > > > > > > 
> > > > > > > We try to minimize as much as possible "ifdefs" in C code.  This
> > > > > > > change is moving in the wrong direction.
> > > > > > 
> > > > > > So your preferred approach is to store the values of Kconfig options in
> > > > > > variables?  That defeats much of the point of having Kconfig options...
> > > > > 
> > > > > No, I prefer using "ifdefs" in include files, not C code, and defining
> > > > > stub functions.
> > > > > 
> > > > > Mimi
> > > > > 
> > > > 
> > > > integrity_init_keyring() is already stubbed out in a header.  What are you
> > > > suggesting, exactly?
> > > 
> > > Refer to section "20) Conditional Compilation" of
> > > Documentation/process/coding-style.rst.
> > > 
> > > Mimi
> > > 
> > 
> > I'm already familiar with that.  Unfortunately, you haven't clearly indicated
> > what alternative you prefer, and it's unclear whether you've even read my patch,
> > given that you're apparently saying to define a stub function which actually
> > already exists.  Maybe you want the 'init_keyring' bool replaced
> > IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING)?  That doesn't really make sense
> > though, because integrity_init_keyring() is already stubbed out in some
> > configurations; it makes more sense to fix the condition for stubbing it out...
> 
> I read your patch and commented that "ifdefs" don't belong in .c
> files, as the above referenced doc says.  My comment on using ifdefs
> for defining stubs was simply an example of how to get around using
> ifdefs in C.
> 
> The doc seems to be suggesting to use IS_ENABLED() to set a flag,
> which is what the existing code does.  Replacing the existing code
> with IS_ENABLE() would be fine, but seems unnecessary.
> 
> I don't see a need to change the existing code to add more ifdefs in
> .c files.
> 

There's no need to have a static variable that just holds the value of a
build-time define.  That's common sense.  And no, the coding style doc doesn't
say otherwise.  Anyway, I'll send the version that just replaces it with
IS_ENABLED() since that's still much better than the weird obfuscated thing it's
doing now, though it's still a bit silly since the function is already stubbed
out in some configurations...

- Eric