It's base on the kernel patches of Fedora 20.
This patch set add the support to MODSIGN mechanism for revoke kernel module by hash or public key. As MokListRT, EFI bootloader(e.g. shim) should maintain the MokListXRT container the format the same with dbx. The patches will check the hash of kernel module before load it.
Lee, Chun-Yi (2): MODSIGN: check hash of kernel module in blacklist MODSIGN: load hash blacklist of modules from MOKx
include/linux/efi.h | 12 ++++ kernel/modsign_uefi.c | 150 +++++++++++++++++++++++++++++++++++++++++++++- kernel/module-internal.h | 14 ++++ kernel/module.c | 9 +++- kernel/module_signing.c | 79 ++++++++++++++++++++++++ 5 files changed, 261 insertions(+), 3 deletions(-)
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.
Signed-off-by: Lee, Chun-Yi jlee@suse.com --- kernel/module-internal.h | 14 ++++++++ kernel/module.c | 9 +++++- kernel/module_signing.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 1 deletions(-)
diff --git a/kernel/module-internal.h b/kernel/module-internal.h index 915e123..f1b6477 100644 --- a/kernel/module-internal.h +++ b/kernel/module-internal.h @@ -9,4 +9,18 @@ * 2 of the Licence, or (at your option) any later version. */
+/* + * Module hash. + */ +struct module_hash { + struct list_head list; /* list of all hashs */ + u8 hash; /* Hash algorithm [enum pkey_hash_algo] */ + char *hash_name; /* nams string of hash */ + size_t size; /* size of hash */ + u8 hash_data[]; /* Hash data */ +}; + +extern struct list_head module_hash_blacklist; + +extern int mod_verify_hash(const void *mod, unsigned long modlen); extern int mod_verify_sig(const void *mod, unsigned long *_modlen); diff --git a/kernel/module.c b/kernel/module.c index 3305511..dd06d8a 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2562,10 +2562,16 @@ static inline void kmemleak_load_module(const struct module *mod, #ifdef CONFIG_MODULE_SIG static int module_sig_check(struct load_info *info) { - int err = -ENOKEY; + int err; const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1; const void *mod = info->hdr;
+ /* check hash of module in blacklist */ + err = mod_verify_hash(mod, info->len); + if (err) + goto match_blacklist_hash; + + err = -ENOKEY; if (info->len > markerlen && memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) { /* We truncate the module to discard the signature */ @@ -2578,6 +2584,7 @@ static int module_sig_check(struct load_info *info) return 0; }
+match_blacklist_hash: /* Not having a signature is only an error if we're strict. */ if (err < 0 && fips_enabled) panic("Module verification failed with error %d in FIPS mode\n", diff --git a/kernel/module_signing.c b/kernel/module_signing.c index 0a29b40..cd7f441 100644 --- a/kernel/module_signing.c +++ b/kernel/module_signing.c @@ -11,12 +11,15 @@
#include <linux/kernel.h> #include <linux/err.h> +#include <linux/module.h> #include <crypto/public_key.h> #include <crypto/hash.h> #include <keys/asymmetric-type.h> #include <keys/system_keyring.h> #include "module-internal.h"
+LIST_HEAD(module_hash_blacklist); + /* * Module signature information block. * @@ -193,6 +196,82 @@ static struct key *request_asymmetric_key(const char *signer, size_t signer_len, return key_ref_to_ptr(key); }
+int mod_verify_hash(const void *mod, unsigned long modlen) +{ + struct module_signature ms; + struct module_hash *module_hash; + struct crypto_shash *tfm; + struct shash_desc *desc; + size_t digest_size, desc_size; + size_t sig_len; + u8 *digest; + int ret = 0; + + 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; + } + + 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); + continue; + } + + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc); + digest_size = crypto_shash_digestsize(tfm); + digest = kzalloc(digest_size + desc_size, GFP_KERNEL); + if (!digest) { + pr_err("digest memory buffer allocate fail\n"); + ret = -ENOMEM; + goto error_digest; + } + desc = (void *)digest + digest_size; + desc->tfm = tfm; + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP; + ret = crypto_shash_init(desc); + if (ret < 0) + goto error_shash; + + 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); + } +error_shash: + kfree(digest); +error_digest: + crypto_free_shash(tfm); + if (ret) + break; + } + + return ret; +} + /* * Verify the signature on a module. */
Lee, Chun-Yi joeyli.kernel@gmail.com wrote:
desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);digest_size = crypto_shash_digestsize(tfm);digest = kzalloc(digest_size + desc_size, GFP_KERNEL);if (!digest) {pr_err("digest memory buffer allocate fail\n");ret = -ENOMEM;goto error_digest;}desc = (void *)digest + digest_size;desc->tfm = tfm;desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;ret = crypto_shash_init(desc);if (ret < 0)goto error_shash;ret = crypto_shash_finup(desc, mod, modlen, digest);if (ret < 0)goto error_shash;
Can you use the digest generated by mod_make_digest() to avoid computing the hash twice?
David
Hi David,
First, thanks for your review and suggestion!
於 三，2013-12-11 於 14:49 +0000，David Howells 提到：
Lee, Chun-Yi joeyli.kernel@gmail.com wrote:
desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);digest_size = crypto_shash_digestsize(tfm);digest = kzalloc(digest_size + desc_size, GFP_KERNEL);if (!digest) {pr_err("digest memory buffer allocate fail\n");ret = -ENOMEM;goto error_digest;}desc = (void *)digest + digest_size;desc->tfm = tfm;desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;ret = crypto_shash_init(desc);if (ret < 0)goto error_shash;ret = crypto_shash_finup(desc, mod, modlen, digest);if (ret < 0)goto error_shash;Can you use the digest generated by mod_make_digest() to avoid computing the hash twice?
David
My original thinking is the algorithm of blacklisted hashes may not the same with the hash of kernel module signature. But as you point out, now I think maybe don't need this flexibility.
In next version, I will use the same hash algorithm as mod_make_digest() to void computing it twice.
Thanks a lot! Joey Lee
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.
Signed-off-by: Lee, Chun-Yi jlee@suse.com
kernel/module-internal.h | 14 ++++++++ kernel/module.c | 9 +++++- kernel/module_signing.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 1 deletions(-)
diff --git a/kernel/module-internal.h b/kernel/module-internal.h index 915e123..f1b6477 100644 --- a/kernel/module-internal.h +++ b/kernel/module-internal.h @@ -9,4 +9,18 @@
- 2 of the Licence, or (at your option) any later version.
*/
+/*
- Module hash.
- */
+struct module_hash {
- struct list_head list; /* list of all hashs */
- u8 hash; /* Hash algorithm [enum pkey_hash_algo] */
- char *hash_name; /* nams string of hash */
- size_t size; /* size of hash */
- u8 hash_data[]; /* Hash data */
+};
+extern struct list_head module_hash_blacklist;
+extern int mod_verify_hash(const void *mod, unsigned long modlen); extern int mod_verify_sig(const void *mod, unsigned long *_modlen); diff --git a/kernel/module.c b/kernel/module.c index 3305511..dd06d8a 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2562,10 +2562,16 @@ static inline void kmemleak_load_module(const struct module *mod, #ifdef CONFIG_MODULE_SIG static int module_sig_check(struct load_info *info) {
- int err = -ENOKEY;
int err; const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1; const void *mod = info->hdr;
/* check hash of module in blacklist */
err = mod_verify_hash(mod, info->len);
if (err)
goto match_blacklist_hash;err = -ENOKEY; if (info->len > markerlen && memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) { /* We truncate the module to discard the signature */
@@ -2578,6 +2584,7 @@ static int module_sig_check(struct load_info *info) return 0; }
+match_blacklist_hash: /* Not having a signature is only an error if we're strict. */ if (err < 0 && fips_enabled) panic("Module verification failed with error %d in FIPS mode\n", diff --git a/kernel/module_signing.c b/kernel/module_signing.c index 0a29b40..cd7f441 100644 --- a/kernel/module_signing.c +++ b/kernel/module_signing.c @@ -11,12 +11,15 @@
#include <linux/kernel.h> #include <linux/err.h> +#include <linux/module.h> #include <crypto/public_key.h> #include <crypto/hash.h> #include <keys/asymmetric-type.h> #include <keys/system_keyring.h> #include "module-internal.h"
+LIST_HEAD(module_hash_blacklist);
/*
- Module signature information block.
@@ -193,6 +196,82 @@ static struct key *request_asymmetric_key(const char *signer, size_t signer_len, return key_ref_to_ptr(key); }
+int mod_verify_hash(const void *mod, unsigned long modlen) +{
- struct module_signature ms;
- struct module_hash *module_hash;
- struct crypto_shash *tfm;
- struct shash_desc *desc;
- size_t digest_size, desc_size;
- size_t sig_len;
- u8 *digest;
- int ret = 0;
- 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?
- 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.
continue;}desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);digest_size = crypto_shash_digestsize(tfm);digest = kzalloc(digest_size + desc_size, GFP_KERNEL);if (!digest) {pr_err("digest memory buffer allocate fail\n");ret = -ENOMEM;goto error_digest;}desc = (void *)digest + digest_size;desc->tfm = tfm;desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;ret = crypto_shash_init(desc);if (ret < 0)goto error_shash;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
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
On Thu, Dec 12, 2013 at 11:46:22AM +0800, joeyli wrote:
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.
That is all true, but we don't necessarily control what hash is actually stored in dbx/MokXRT. If a user (or in the case of dbx, the CA) happened to hash the module with the signature attached and enrolled that hash into UEFI/Mok, then doing a comparison with the signature stripped against that will fail, won't it? That is why I was suggesting we needed to compare against both.
I agree that the ideal situation would be for the enrolled hash to be free of signatures, but there's nothing that guarantees that will be the case.
(I also think the vast majority of blacklisting will be with certs, not with individual modules so this is somewhat minor. I think that even small build-time variances will make module blacklisting difficult to actually make viable.)
josh
於 四，2013-12-12 於 08:33 -0500，Josh Boyer 提到：
On Thu, Dec 12, 2013 at 11:46:22AM +0800, joeyli wrote:
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.
That is all true, but we don't necessarily control what hash is actually stored in dbx/MokXRT. If a user (or in the case of dbx, the CA) happened to hash the module with the signature attached and enrolled that hash into UEFI/Mok, then doing a comparison with the signature stripped against that will fail, won't it? That is why I was suggesting we needed to compare against both.
I agree that the ideal situation would be for the enrolled hash to be free of signatures, but there's nothing that guarantees that will be the case.
OK, I will also computing the hash with signature and compare.
(I also think the vast majority of blacklisting will be with certs, not with individual modules so this is somewhat minor. I think that even small build-time variances will make module blacklisting difficult to actually make viable.)
josh
For the situation we don't want revoke key of certificate, it's the way we need carry out.
Thanks a lot! Joey Lee
On Fri, Dec 13, 2013 at 12:34:11PM +0800, joeyli wrote:
於 四，2013-12-12 於 08:33 -0500，Josh Boyer 提到：
On Thu, Dec 12, 2013 at 11:46:22AM +0800, joeyli wrote:
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.
That is all true, but we don't necessarily control what hash is actually stored in dbx/MokXRT. If a user (or in the case of dbx, the CA) happened to hash the module with the signature attached and enrolled that hash into UEFI/Mok, then doing a comparison with the signature stripped against that will fail, won't it? That is why I was suggesting we needed to compare against both.
I agree that the ideal situation would be for the enrolled hash to be free of signatures, but there's nothing that guarantees that will be the case.
OK, I will also computing the hash with signature and compare.
OK. I realize it's unfortunate to have to do that, but I don't see another way at the moment.
(I also think the vast majority of blacklisting will be with certs, not with individual modules so this is somewhat minor. I think that even small build-time variances will make module blacklisting difficult to actually make viable.)
josh
For the situation we don't want revoke key of certificate, it's the way we need carry out.
Yes, agreed. I suspect it will be most useful for 3rd party drivers rather than individual modules a distribution ships.
Thanks again!
josh
This patch add the support to load blacklisted hash or public key from MOKx that's maintained by bootloader.
Signed-off-by: Lee, Chun-Yi jlee@suse.com --- include/linux/efi.h | 12 ++++ kernel/modsign_uefi.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 2 deletions(-)
diff --git a/include/linux/efi.h b/include/linux/efi.h index 9e96ab0..54154f8 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -392,6 +392,18 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long si #define EFI_CERT_SHA256_GUID \ EFI_GUID( 0xc1c41626, 0x504c, 0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 )
+#define EFI_CERT_SHA1_GUID \ + EFI_GUID( 0x826ca512, 0xcf10, 0x4ac9, 0xb1, 0x87, 0xbe, 0x1, 0x49, 0x66, 0x31, 0xbd ) + +#define EFI_CERT_SHA512_GUID \ + EFI_GUID( 0x93e0fae, 0xa6c4, 0x4f50, 0x9f, 0x1b, 0xd4, 0x1e, 0x2b, 0x89, 0xc1, 0x9a ) + +#define EFI_CERT_SHA224_GUID \ + EFI_GUID( 0xb6e5233, 0xa65c, 0x44c9, 0x94, 0x7, 0xd9, 0xab, 0x83, 0xbf, 0xc8, 0xbd ) + +#define EFI_CERT_SHA384_GUID \ + EFI_GUID( 0xff3e5307, 0x9fd0, 0x48c9, 0x85, 0xf1, 0x8a, 0xd5, 0x6c, 0x70, 0x1e, 0x1 ) + #define EFI_CERT_X509_GUID \ EFI_GUID( 0xa5c059a1, 0x94e4, 0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72 )
diff --git a/kernel/modsign_uefi.c b/kernel/modsign_uefi.c index ae28b97..c509fa0 100644 --- a/kernel/modsign_uefi.c +++ b/kernel/modsign_uefi.c @@ -4,10 +4,27 @@ #include <linux/err.h> #include <linux/efi.h> #include <linux/slab.h> +#include <crypto/public_key.h> #include <keys/asymmetric-type.h> #include <keys/system_keyring.h> #include "module-internal.h"
+struct efi_hash_type { + u8 hash; /* Hash algorithm [enum pkey_hash_algo] */ + efi_guid_t efi_cert_guid; /* EFI_CERT_*_GUID */ + char *hash_name; /* nams string of hash */ + size_t size; /* size of hash */ +}; + +static const struct efi_hash_type efi_hash_types[] = { + {PKEY_HASH_SHA256, EFI_CERT_SHA256_GUID, "sha256", 32}, + {PKEY_HASH_SHA1, EFI_CERT_SHA1_GUID, "sha1", 20}, + {PKEY_HASH_SHA512, EFI_CERT_SHA512_GUID, "sha512", 64}, + {PKEY_HASH_SHA224, EFI_CERT_SHA224_GUID, "sha224", 28}, + {PKEY_HASH_SHA384, EFI_CERT_SHA384_GUID, "sha384", 48}, + {PKEY_HASH__LAST} +}; + static __init int check_ignore_db(void) { efi_status_t status; @@ -55,6 +72,121 @@ out: return db; }
+static int __init signature_blacklist_func(efi_guid_t efi_cert_guid, + const efi_signature_data_t *elem) +{ + struct module_hash *module_hash = NULL; + int i; + + for (i = 0; efi_hash_types[i].hash < PKEY_HASH__LAST; i++) { + struct efi_hash_type type = efi_hash_types[i]; + + if (efi_guidcmp(efi_cert_guid, type.efi_cert_guid) != 0) + continue; + + module_hash = kzalloc(sizeof(*module_hash) + type.size, GFP_KERNEL); + module_hash->hash = type.hash; + module_hash->hash_name = type.hash_name; + module_hash->size = type.size; + memcpy(module_hash->hash_data, elem->signature_data, type.size); + } + + if (!module_hash) { + pr_err("Problem loading hash of blacklisted module: %pUb\n", + &efi_cert_guid); + return -ENOTSUPP; + } else { + pr_notice("Loaded %s hash %*phN to blacklisted modules\n", + module_hash->hash_name, (int) module_hash->size, + module_hash->hash_data); + } + + list_add(&module_hash->list, &module_hash_blacklist); + + return 0; +} + +static int __init parse_efi_signature_list_hash(const void *data, size_t size, + efi_guid_t efi_cert_guid, + int (*func)(efi_guid_t, const efi_signature_data_t *)) +{ + unsigned offs = 0; + size_t lsize, esize, hsize, elsize; + + pr_debug("-->%s(,%zu)\n", __func__, size); + + while (size > 0) { + efi_signature_list_t list; + const efi_signature_data_t *elem; + if (size < sizeof(list)) + return -EBADMSG; + + memcpy(&list, data, sizeof(list)); + pr_debug("LIST[%04x] guid=%pUl ls=%x hs=%x ss=%x\n", + offs, + list.signature_type.b, list.signature_list_size, + list.signature_header_size, list.signature_size); + + lsize = list.signature_list_size; + hsize = list.signature_header_size; + esize = list.signature_size; + elsize = lsize - sizeof(list) - hsize; + + if (lsize > size) { + pr_debug("<--%s() = -EBADMSG [overrun @%x]\n", + __func__, offs); + return -EBADMSG; + } + if (lsize < sizeof(list) || + lsize - sizeof(list) < hsize || + esize < sizeof(*elem) || + elsize < esize || + elsize % esize != 0) { + pr_debug("- bad size combo @%x\n", offs); + return -EBADMSG; + } + + if (efi_guidcmp(list.signature_type, efi_cert_guid) != 0) { + data += lsize; + size -= lsize; + offs += lsize; + continue; + } + + data += sizeof(list) + hsize; + size -= sizeof(list) + hsize; + offs += sizeof(list) + hsize; + + for (; elsize > 0; elsize -= esize) { + elem = data; + + pr_debug("ELEM[%04x]\n", offs); + func(efi_cert_guid, elem); + data += esize; + size -= esize; + offs += esize; + } + } + + return 0; +} + +static int __init parse_efi_signature_list_hashs(const void *data, size_t size, + int (*func)(efi_guid_t, const efi_signature_data_t *)) +{ + int i, rc = 0; + + for (i = 0; !rc && efi_hash_types[i].hash < PKEY_HASH__LAST; i++) { + struct efi_hash_type type = efi_hash_types[i]; + rc = parse_efi_signature_list_hash(data, size, type.efi_cert_guid, func); + if (rc) + pr_err("Couldn't parse signatures of %s hash: %d\n", + type.hash_name, rc); + } + + return rc; +} + /* * * Load the certs contained in the UEFI databases * */ @@ -62,8 +194,8 @@ static int __init load_uefi_certs(void) { efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID; efi_guid_t mok_var = EFI_SHIM_LOCK_GUID; - void *db = NULL, *dbx = NULL, *mok = NULL; - unsigned long dbsize = 0, dbxsize = 0, moksize = 0; + void *db = NULL, *dbx = NULL, *mok = NULL, *mokx = NULL; + unsigned long dbsize = 0, dbxsize = 0, moksize = 0, mokxsize = 0; int ignore_db, rc = 0;
/* Check if SB is enabled and just return if not */ @@ -109,6 +241,20 @@ static int __init load_uefi_certs(void) kfree(dbx); }
+ mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize); + if (!mokx) { + pr_info("MODSIGN: Couldn't get UEFI MokListXRT\n"); + } else { + rc = parse_efi_signature_list(mokx, mokxsize, + system_blacklist_keyring); + if (rc) + pr_err("Couldn't parse MokListXRT signatures: %d\n", rc); + else + parse_efi_signature_list_hashs(mokx, mokxsize, + signature_blacklist_func); + kfree(mokx); + } + return rc; } late_initcall(load_uefi_certs);
On Wed, Dec 11, 2013 at 03:26:17PM +0800, Lee, Chun-Yi wrote:
This patch add the support to load blacklisted hash or public key from MOKx that's maintained by bootloader.
Signed-off-by: Lee, Chun-Yi jlee@suse.com
include/linux/efi.h | 12 ++++ kernel/modsign_uefi.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 2 deletions(-)
diff --git a/include/linux/efi.h b/include/linux/efi.h index 9e96ab0..54154f8 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -392,6 +392,18 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long si #define EFI_CERT_SHA256_GUID \ EFI_GUID( 0xc1c41626, 0x504c, 0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 )
+#define EFI_CERT_SHA1_GUID \
- EFI_GUID( 0x826ca512, 0xcf10, 0x4ac9, 0xb1, 0x87, 0xbe, 0x1, 0x49, 0x66, 0x31, 0xbd )
+#define EFI_CERT_SHA512_GUID \
- EFI_GUID( 0x93e0fae, 0xa6c4, 0x4f50, 0x9f, 0x1b, 0xd4, 0x1e, 0x2b, 0x89, 0xc1, 0x9a )
+#define EFI_CERT_SHA224_GUID \
- EFI_GUID( 0xb6e5233, 0xa65c, 0x44c9, 0x94, 0x7, 0xd9, 0xab, 0x83, 0xbf, 0xc8, 0xbd )
+#define EFI_CERT_SHA384_GUID \
- EFI_GUID( 0xff3e5307, 0x9fd0, 0x48c9, 0x85, 0xf1, 0x8a, 0xd5, 0x6c, 0x70, 0x1e, 0x1 )
#define EFI_CERT_X509_GUID \ EFI_GUID( 0xa5c059a1, 0x94e4, 0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72 )
diff --git a/kernel/modsign_uefi.c b/kernel/modsign_uefi.c index ae28b97..c509fa0 100644 --- a/kernel/modsign_uefi.c +++ b/kernel/modsign_uefi.c @@ -4,10 +4,27 @@ #include <linux/err.h> #include <linux/efi.h> #include <linux/slab.h> +#include <crypto/public_key.h> #include <keys/asymmetric-type.h> #include <keys/system_keyring.h> #include "module-internal.h"
+struct efi_hash_type {
- u8 hash; /* Hash algorithm [enum pkey_hash_algo] */
- efi_guid_t efi_cert_guid; /* EFI_CERT_*_GUID */
- char *hash_name; /* nams string of hash */
- size_t size; /* size of hash */
+};
+static const struct efi_hash_type efi_hash_types[] = {
- {PKEY_HASH_SHA256, EFI_CERT_SHA256_GUID, "sha256", 32},
- {PKEY_HASH_SHA1, EFI_CERT_SHA1_GUID, "sha1", 20},
- {PKEY_HASH_SHA512, EFI_CERT_SHA512_GUID, "sha512", 64},
- {PKEY_HASH_SHA224, EFI_CERT_SHA224_GUID, "sha224", 28},
- {PKEY_HASH_SHA384, EFI_CERT_SHA384_GUID, "sha384", 48},
- {PKEY_HASH__LAST}
+};
static __init int check_ignore_db(void) { efi_status_t status; @@ -55,6 +72,121 @@ out: return db; }
+static int __init signature_blacklist_func(efi_guid_t efi_cert_guid,
const efi_signature_data_t *elem)+{
- struct module_hash *module_hash = NULL;
- int i;
- for (i = 0; efi_hash_types[i].hash < PKEY_HASH__LAST; i++) {
struct efi_hash_type type = efi_hash_types[i];if (efi_guidcmp(efi_cert_guid, type.efi_cert_guid) != 0)continue;module_hash = kzalloc(sizeof(*module_hash) + type.size, GFP_KERNEL);module_hash->hash = type.hash;module_hash->hash_name = type.hash_name;module_hash->size = type.size;memcpy(module_hash->hash_data, elem->signature_data, type.size);- }
- if (!module_hash) {
pr_err("Problem loading hash of blacklisted module: %pUb\n",&efi_cert_guid);return -ENOTSUPP;
Shouldn't this check be moved directly under the kzalloc call? Or I suppose a similar check should be added there.
- } else {
pr_notice("Loaded %s hash %*phN to blacklisted modules\n",module_hash->hash_name, (int) module_hash->size,module_hash->hash_data);- }
- list_add(&module_hash->list, &module_hash_blacklist);
- return 0;
+}
+static int __init parse_efi_signature_list_hash(const void *data, size_t size,
efi_guid_t efi_cert_guid,int (*func)(efi_guid_t, const efi_signature_data_t *))+{
- unsigned offs = 0;
- size_t lsize, esize, hsize, elsize;
- pr_debug("-->%s(,%zu)\n", __func__, size);
- while (size > 0) {
efi_signature_list_t list;const efi_signature_data_t *elem;if (size < sizeof(list))return -EBADMSG;memcpy(&list, data, sizeof(list));pr_debug("LIST[%04x] guid=%pUl ls=%x hs=%x ss=%x\n",offs,list.signature_type.b, list.signature_list_size,list.signature_header_size, list.signature_size);lsize = list.signature_list_size;hsize = list.signature_header_size;esize = list.signature_size;elsize = lsize - sizeof(list) - hsize;if (lsize > size) {pr_debug("<--%s() = -EBADMSG [overrun @%x]\n",__func__, offs);return -EBADMSG;}if (lsize < sizeof(list) ||lsize - sizeof(list) < hsize ||esize < sizeof(*elem) ||elsize < esize ||elsize % esize != 0) {pr_debug("- bad size combo @%x\n", offs);return -EBADMSG;}if (efi_guidcmp(list.signature_type, efi_cert_guid) != 0) {data += lsize;size -= lsize;offs += lsize;continue;}data += sizeof(list) + hsize;size -= sizeof(list) + hsize;offs += sizeof(list) + hsize;for (; elsize > 0; elsize -= esize) {elem = data;pr_debug("ELEM[%04x]\n", offs);func(efi_cert_guid, elem);
The func call here can fail but there is no return code checking.
data += esize;size -= esize;offs += esize;}- }
- return 0;
+}
+static int __init parse_efi_signature_list_hashs(const void *data, size_t size,
int (*func)(efi_guid_t, const efi_signature_data_t *))
Why do we pass around a function pointer here when there is only one place we call this and it always passes the same function?
+{
- int i, rc = 0;
- for (i = 0; !rc && efi_hash_types[i].hash < PKEY_HASH__LAST; i++) {
struct efi_hash_type type = efi_hash_types[i];rc = parse_efi_signature_list_hash(data, size, type.efi_cert_guid, func);if (rc)pr_err("Couldn't parse signatures of %s hash: %d\n",type.hash_name, rc);- }
- return rc;
+}
/*
- Load the certs contained in the UEFI databases
- */
@@ -62,8 +194,8 @@ static int __init load_uefi_certs(void) { efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID; efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
- void *db = NULL, *dbx = NULL, *mok = NULL;
- unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
void *db = NULL, *dbx = NULL, *mok = NULL, *mokx = NULL;
unsigned long dbsize = 0, dbxsize = 0, moksize = 0, mokxsize = 0; int ignore_db, rc = 0;
/* Check if SB is enabled and just return if not */
@@ -109,6 +241,20 @@ static int __init load_uefi_certs(void) kfree(dbx); }
- mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize);
- if (!mokx) {
pr_info("MODSIGN: Couldn't get UEFI MokListXRT\n");
Is there a reason we don't call the hash parsing on dbx itself?
- } else {
rc = parse_efi_signature_list(mokx, mokxsize,system_blacklist_keyring);if (rc)pr_err("Couldn't parse MokListXRT signatures: %d\n", rc);elseparse_efi_signature_list_hashs(mokx, mokxsize,signature_blacklist_func);kfree(mokx);
Do we want to only check for hashes if signature parsing fails? MokListXRT could contain both hashes and certs, couldn't it?
- }
- return rc;
} late_initcall(load_uefi_certs); -- 1.6.4.2
於 三，2013-12-11 於 11:15 -0500，Josh Boyer 提到：
On Wed, Dec 11, 2013 at 03:26:17PM +0800, Lee, Chun-Yi wrote:
This patch add the support to load blacklisted hash or public key from
...
+static int __init signature_blacklist_func(efi_guid_t efi_cert_guid,
const efi_signature_data_t *elem)+{
- struct module_hash *module_hash = NULL;
- int i;
- for (i = 0; efi_hash_types[i].hash < PKEY_HASH__LAST; i++) {
struct efi_hash_type type = efi_hash_types[i];if (efi_guidcmp(efi_cert_guid, type.efi_cert_guid) != 0)continue;module_hash = kzalloc(sizeof(*module_hash) + type.size, GFP_KERNEL);module_hash->hash = type.hash;module_hash->hash_name = type.hash_name;module_hash->size = type.size;memcpy(module_hash->hash_data, elem->signature_data, type.size);- }
- if (!module_hash) {
pr_err("Problem loading hash of blacklisted module: %pUb\n",&efi_cert_guid);return -ENOTSUPP;Shouldn't this check be moved directly under the kzalloc call? Or I suppose a similar check should be added there.
Next version patch will hook the hash algorithm of blacklist the same with kernel module signature. I will simplify this checking code.
data += sizeof(list) + hsize;size -= sizeof(list) + hsize;offs += sizeof(list) + hsize;for (; elsize > 0; elsize -= esize) {elem = data;pr_debug("ELEM[%04x]\n", offs);func(efi_cert_guid, elem);The func call here can fail but there is no return code checking.
Thank's for your point out, I will add the return code checking here.
data += esize;size -= esize;offs += esize;}- }
- return 0;
+}
+static int __init parse_efi_signature_list_hashs(const void *data, size_t size,
int (*func)(efi_guid_t, const efi_signature_data_t *))Why do we pass around a function pointer here when there is only one place we call this and it always passes the same function?
Because the code of parse_efi_signature_list_hash() is almost the same with parse_efi_signature_list(), so I want pass different functions to handle the different logic of them.
Sorry for I forgot merge this change to parse_efi_signature_list(). Thanks for your remind, I will do it in next version.
/*
- Load the certs contained in the UEFI databases
- */
@@ -62,8 +194,8 @@ static int __init load_uefi_certs(void) { efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID; efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
- void *db = NULL, *dbx = NULL, *mok = NULL;
- unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
void *db = NULL, *dbx = NULL, *mok = NULL, *mokx = NULL;
unsigned long dbsize = 0, dbxsize = 0, moksize = 0, mokxsize = 0; int ignore_db, rc = 0;
/* Check if SB is enabled and just return if not */
@@ -109,6 +241,20 @@ static int __init load_uefi_certs(void) kfree(dbx); }
- mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize);
- if (!mokx) {
pr_info("MODSIGN: Couldn't get UEFI MokListXRT\n");Is there a reason we don't call the hash parsing on dbx itself?
Yes, we can, I will add the hash parsing on dbx.
- } else {
rc = parse_efi_signature_list(mokx, mokxsize,system_blacklist_keyring);if (rc)pr_err("Couldn't parse MokListXRT signatures: %d\n", rc);elseparse_efi_signature_list_hashs(mokx, mokxsize,signature_blacklist_func);kfree(mokx);Do we want to only check for hashes if signature parsing fails? MokListXRT could contain both hashes and certs, couldn't it?
Yes, the signature parsing fail should not effect to the hash parsing. I will fix it in next version.
Thanks a lot! Joey Lee
On Wed, Dec 11, 2013 at 03:26:15PM +0800, Lee, Chun-Yi wrote:
It's base on the kernel patches of Fedora 20.
This patch set add the support to MODSIGN mechanism for revoke kernel module by hash or public key. As MokListRT, EFI bootloader(e.g. shim) should maintain the MokListXRT container the format the same with dbx. The patches will check the hash of kernel module before load it.
Thanks for doing this! A few comments/questions on the patches to follow.
josh
kernel@lists.fedoraproject.org
-
David Howells -
joeyli -
Josh Boyer -
Lee, Chun-Yi