Re: [PATCH v0 1/1] KEYS: LSM Hook for key_create_or_update

[Mimi Zohar <zohar@linux.ibm.com>]

On Tue, 2019-10-15 at 16:17 -0700, Lakshmi Ramasubramanian wrote:
> Add a LSM Hook for key_create_or_update function. The motive behind
> this change is enable subsystems to receive notification when
> a new key is created or updated.

As per Documentation/process/submitting-patches.rst section "2)
Describe your changes", please begin the patch description by
describing the problem.

> 
> IMA will be one of the subsystems that will use this hook to measure
> the newly created key.
> 
> This change set includes helper functions to check if the keyring
> (to which a new key is being added, for example) is one of 
> the trusted keys keyring (builtin, secondary, or platform).
> 
> The change set also includes an IMA function that will be called
> from the LSM hook to notify of the newly created key and the keyring
> to which the key is being added to.
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

This patch should be broken up even further.[1]  In this case to
simplify review, separate defining the new LSM hook from any IMA code.
 Different maintainers need to Ack/sign off on these patches.

The new LSM hook patch, with a clear well written patch description,
should be posted on the LSM mailing list as well.

[1] Refer to Documentation/process/submitting-patches.rst section 3)
"Separate your changes". 
  
> ---
>  certs/system_keyring.c            | 23 +++++++++++++++++++++++
>  include/keys/system_keyring.h     |  4 ++++
>  include/linux/ima.h               |  8 ++++++++
>  include/linux/lsm_hooks.h         | 14 ++++++++++++++
>  include/linux/security.h          | 10 ++++++++++
>  security/integrity/ima/ima.h      |  1 +
>  security/integrity/ima/ima_main.c | 19 +++++++++++++++++++
>  security/keys/key.c               |  8 ++++++++
>  security/security.c               | 11 +++++++++++
>  9 files changed, 98 insertions(+)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 1eba08a1af82..a89d23fb5d9d 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -283,3 +283,26 @@ void __init set_platform_trusted_keys(struct key *keyring)
>  	platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +inline bool is_builtin_trusted_keyring(struct key *keyring)
> +{
> +	return (keyring == builtin_trusted_keys);
> +}
> +
> +inline bool is_secondary_trusted_keyring(struct key *keyring)
> +{
> +	#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +	return (keyring == secondary_trusted_keys);
> +	#else
> +	return false;
> +	#endif
> +}
> +
> +inline bool is_platform_trusted_keyring(struct key *keyring)
> +{
> +	#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> +	return (keyring == platform_trusted_keys);
> +	#else
> +	return false;
> +	#endif
> +}

Why are these functions defined in a new LSM hook patch?  Before
posting a patch, please review the patch line by line, making sure
that there isn't anything extraneous.

> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index c1a96fdf598b..2517181d8d6c 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -66,4 +66,8 @@ static inline void set_platform_trusted_keys(struct key *keyring)
>  }
>  #endif
>  
> +extern bool is_builtin_trusted_keyring(struct key *keyring);
> +extern bool is_secondary_trusted_keyring(struct key *keyring);
> +extern bool is_platform_trusted_keyring(struct key *keyring);
> +

Not needed in this patch.

Mimi