[PATCH 1/2] MODSIGN: check hash of kernel module in blacklist

[joeyli]

Hi Josh, 

Thanks for your review and suggestions!

於 三，2013-12-11 於 11:07 -0500，Josh Boyer 提到：
> On Wed, Dec 11, 2013 at 03:26:16PM +0800, Lee, Chun-Yi wrote:
> > This patch introduces a blacklist list of kernel module's hash. It check
> > the blacklist before checking kernel module signature.
> > It didn't limit what hash algorithm used but the module of hash algorithm
> > need build-in or put in initrd for verify kernel module in initrd.
> > 
...
> > +	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> > +
> > +	/* truncate the module to discard the signature when it signed */
> > +	if (modlen > markerlen &&
> > +	    memcmp(mod + modlen - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> > +		modlen -= markerlen;
> > +		if (modlen <= sizeof(ms))
> > +			return -EBADMSG;
> > +		memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
> > +		modlen -= sizeof(ms);
> > +		sig_len = be32_to_cpu(ms.sig_len);
> > +		if (sig_len >= modlen)
> > +			return -EBADMSG;
> > +		modlen -= sig_len;
> > +		if ((size_t)ms.signer_len + ms.key_id_len >= modlen)
> > +			return -EBADMSG;
> > +		modlen -= (size_t)ms.signer_len + ms.key_id_len;
> > +	}
> 
> Hm.  Why do we discard the signature before we calculate the hash?  It
> seems we might need to check for a hash of both the signed and unsigned
> module, correct?
> 

Yes, the reason of blacklisted a kernel module is there have security
weakness in the code of module. So, no matter who signed the kernel
module or even the module didn't sign, we don't want it loaded by
kernel.

For another situation, if we want revoke a KEY, then just direct import
the public key to MOKx but not add hash of signed kernel module one by
one.

> > +
> > +	list_for_each_entry(module_hash, &module_hash_blacklist, list) {
> > +		tfm = crypto_alloc_shash(module_hash->hash_name, 0, 0);
> > +		if (IS_ERR(tfm)) {
> > +			printk_once(KERN_WARNING "The %s hash algorithm did "
> > +				"not load for check blacklisted module hash: "
> > +				"%*phN\n", module_hash->hash_name,
> > +				(int) module_hash->size,
> > +				module_hash->hash_data);
> 
> "Cannot check for blacklisted module hash.  The %s hash algorithm did
> not load." might read better.
> 

Thanks for your suggestion, I will change the log statement.

> > +
> > +		ret = crypto_shash_finup(desc, mod, modlen, digest);
> > +		if (ret < 0)
> > +			goto error_shash;
> > +
> > +		if (!memcmp(digest, module_hash->hash_data, digest_size)) {
> > +			ret = -EKEYREJECTED;
> > +			pr_info("Module Hash is in MOKx blacklisted: %*phN\n",
> > +				(int) module_hash->size, module_hash->hash_data);
> 
> Maybe change the message to "Module hash is in the module hash blacklist:".
> 
> josh
> 

Thanks, I will also change the log statement here.


Thanks a lot!
Joey Lee