Re: [PATCH] x86/ima: require signed kernel modules

[Luis Chamberlain <mcgrof@kernel.org>]

On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> diff --git a/kernel/module.c b/kernel/module.c
> index 2ad1b5239910..70a9709d19eb 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -275,16 +275,23 @@ static void module_assert_mutex_or_preempt(void)
>  
>  static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
>  module_param(sig_enforce, bool_enable_only, 0644);
> +static bool sig_required;
>  
>  /*
>   * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
>   * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.

But the docs were't updated.

>   */
> -bool is_module_sig_enforced(void)
> +bool is_module_sig_enforced_or_required(void)
>  {
> -	return sig_enforce;
> +	return sig_enforce || sig_required;
>  }
> -EXPORT_SYMBOL(is_module_sig_enforced);
> +EXPORT_SYMBOL(is_module_sig_enforced_or_required);

Meh, this is getting sloppy, the module signing infrastructure should
just be LSM'ified now that we have stacked LSMs. That would
compartamentaliz that code and make this much easier to read / understand
and mantain.

Can you take a look at doing it that way instead?

> +
> +void set_module_sig_required(void)
> +{
> +	sig_required = true;
> +}
> +EXPORT_SYMBOL(set_module_sig_required);

Since this is a *new* symbol, not yet used, and only used by IMA I'd
prefer this to be EXPORT_SYMBOL_GPL().

>  /* Block module loading/unloading? */
>  int modules_disabled = 0;
> @@ -2789,7 +2796,7 @@ static int module_sig_check(struct load_info *info, int flags)
>  	}
>  
>  	/* Not having a signature is only an error if we're strict. */
> -	if (err == -ENOKEY && !is_module_sig_enforced())
> +	if (err == -ENOKEY && !is_module_sig_enforced_or_required())

This is where I think a proper LSM hook would make sense. I think
that these "questions" model for signing don't work well on the LSM
hook model, perhaps just:

kernel_module_signed()

Suffices, therefore if not enforced or required its signed. If its
enforced or required and really signed, then it signed.

>  		err = 0;
>  
>  	return err;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 357edd140c09..bbaf87f688be 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -563,7 +563,7 @@ int ima_load_data(enum kernel_load_data_id id)
>  		}
>  		break;
>  	case LOADING_MODULE:
> -		sig_enforce = is_module_sig_enforced();
> +		sig_enforce = is_module_sig_enforced_or_required();

Yet another user.

>  		if (ima_enforce && (!sig_enforce
>  				    && (ima_appraise & IMA_APPRAISE_MODULES))) {
> -- 
> 2.7.5

Plus I think LSM'ifying module signing may help cleaning up some of the
#ifdery and config options around module signing. I'm suggestin this now 
as this has been on my mental TODO list for a while, and just not sure
when we'd get to it, if not you, not sure when it'd get done.

Then, do we have proper unit tests for the mixture of options to ensure
we can easily ensure we don't regress?

  Luis