Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 14 Feb 2019 09:58:23 -0800
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Mimi Zohar <zohar@linux.ibm.com>
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jessica Yu <jeyu@kernel.org>,
        David Howells <dhowells@redhat.com>,
        Seth Forshee <seth.forshee@canonical.com>,
        "Bruno E . O . Meneguele" <bmeneg@redhat.com>
Subject: Re: [PATCH v2] x86/ima: require signed kernel modules
Message-ID: <20190214175823.GG11489@garbanzo.do-not-panic.com>
References: <1550060279-8624-1-git-send-email-zohar@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1550060279-8624-1-git-send-email-zohar@linux.ibm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 13, 2019 at 07:17:59AM -0500, Mimi Zohar wrote:
> Require signed kernel modules on systems with secure boot mode enabled.
> 
> Requiring appended kernel module signatures may be configured, enabled
> on the boot command line, or with this patch enabled in secure boot
> mode. 

But only if IMA is enabled? If so, should this statement be true if
IMA is disabled?

Either way, this is not clear from the commit log and code, can the
commit log be clear if set_module_sig_enforced() will be set if
IMA is disabled but secure boot mode enabled?

> This patch defines set_module_sig_enforced().
> 
> To coordinate between appended kernel module signatures and IMA
> signatures, only define an IMA MODULE_CHECK policy rule if
> CONFIG_MODULE_SIG is not enabled.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> 
> Changelog:
> - Removed new "sig_required" flag and associated functions, directly set
>   sig_enforce.
> 
>  arch/x86/kernel/ima_arch.c | 9 ++++++++-
>  include/linux/module.h     | 1 +
>  kernel/module.c            | 5 +++++
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
> index e47cd9390ab4..3fb9847f1cad 100644
> --- a/arch/x86/kernel/ima_arch.c
> +++ b/arch/x86/kernel/ima_arch.c
> @@ -64,12 +64,19 @@ static const char * const sb_arch_rules[] = {
>  	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig",
>  #endif /* CONFIG_KEXEC_VERIFY_SIG */
>  	"measure func=KEXEC_KERNEL_CHECK",
> +#if !IS_ENABLED(CONFIG_MODULE_SIG)
> +	"appraise func=MODULE_CHECK appraise_type=imasig",
> +#endif
> +	"measure func=MODULE_CHECK",
>  	NULL
>  };
>  
>  const char * const *arch_get_ima_policy(void)
>  {
> -	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
> +	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot()) {
> +		if (IS_ENABLED(CONFIG_MODULE_SIG))
> +			set_module_sig_enforced();
>  		return sb_arch_rules;
> +	}
>  	return NULL;
>  }
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 8fa38d3e7538..75e2a5c24a2b 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -660,6 +660,7 @@ static inline bool is_livepatch_module(struct module *mod)
>  #endif /* CONFIG_LIVEPATCH */
>  
>  bool is_module_sig_enforced(void);
> +void set_module_sig_enforced(void);
>  
>  #else /* !CONFIG_MODULES... */

I think you need the !CONFIG_MODULES definition of set_module_sig_enforced()
then...

> diff --git a/kernel/module.c b/kernel/module.c
> index 2ad1b5239910..4cb5b733fb18 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -286,6 +286,11 @@ bool is_module_sig_enforced(void)
>  }
>  EXPORT_SYMBOL(is_module_sig_enforced);
>  
> +void set_module_sig_enforced(void)
> +{
> +       sig_enforce = true;
> +}

The export is not needed as it is bool eh?

  Luis