Emit the magic string that indicates a module has a signature after the
signature data instead of before it. This allows module_sig_check() to be
made simpler and faster by the elimination of the search for the magic string.
Instead we just need to do a single memcmp().
This works because at the end of the signature data there is the fixed-length
signature information block. This block then falls immediately prior to the
magic number.
>From the contents of the information block, it is trivial to calculate the size
of the signature data and thus the size of the actual module data.
Signed-off-by: David Howells <[email protected]>
---
kernel/module-internal.h | 3 +--
kernel/module.c | 26 +++++++++-----------------
kernel/module_signing.c | 24 +++++++++++++++---------
scripts/sign-file | 6 +++---
4 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 6114a13..24f9247 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -11,5 +11,4 @@
extern struct key *modsign_keyring;
-extern int mod_verify_sig(const void *mod, unsigned long modlen,
- const void *sig, unsigned long siglen);
+extern int mod_verify_sig(const void *mod, unsigned long *_modlen);
diff --git a/kernel/module.c b/kernel/module.c
index 0e2da86..6085f5e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2421,25 +2421,17 @@ static inline void kmemleak_load_module(const struct module *mod,
#ifdef CONFIG_MODULE_SIG
static int module_sig_check(struct load_info *info,
- const void *mod, unsigned long *len)
+ const void *mod, unsigned long *_len)
{
int err = -ENOKEY;
- const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
- const void *p = mod, *end = mod + *len;
-
- /* Poor man's memmem. */
- while ((p = memchr(p, MODULE_SIG_STRING[0], end - p))) {
- if (p + markerlen > end)
- break;
-
- if (memcmp(p, MODULE_SIG_STRING, markerlen) == 0) {
- const void *sig = p + markerlen;
- /* Truncate module up to signature. */
- *len = p - mod;
- err = mod_verify_sig(mod, *len, sig, end - sig);
- break;
- }
- p++;
+ unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+ unsigned long len = *_len;
+
+ if (len > markerlen &&
+ memcmp(mod + len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
+ /* We truncate the module to discard the signature */
+ *_len -= markerlen;
+ err = mod_verify_sig(mod, _len);
}
if (!err) {
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 6b09f69..d492a23 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -183,27 +183,33 @@ static struct key *request_asymmetric_key(const char *signer, size_t signer_len,
/*
* Verify the signature on a module.
*/
-int mod_verify_sig(const void *mod, unsigned long modlen,
- const void *sig, unsigned long siglen)
+int mod_verify_sig(const void *mod, unsigned long *_modlen)
{
struct public_key_signature *pks;
struct module_signature ms;
struct key *key;
- size_t sig_len;
+ const void *sig;
+ size_t modlen = *_modlen, sig_len;
int ret;
- pr_devel("==>%s(,%lu,,%lu,)\n", __func__, modlen, siglen);
+ pr_devel("==>%s(,%lu)\n", __func__, modlen);
- if (siglen <= sizeof(ms))
+ if (modlen <= sizeof(ms))
return -EBADMSG;
- memcpy(&ms, sig + (siglen - sizeof(ms)), sizeof(ms));
- siglen -= sizeof(ms);
+ memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
+ modlen -= sizeof(ms);
sig_len = be32_to_cpu(ms.sig_len);
- if (sig_len >= siglen ||
- siglen - sig_len != (size_t)ms.signer_len + ms.key_id_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;
+
+ *_modlen = modlen;
+ sig = mod + modlen;
/* For the moment, only support RSA and X.509 identifiers */
if (ms.algo != PKEY_ALGO_RSA ||
diff --git a/scripts/sign-file b/scripts/sign-file
index d37d130..87ca59d 100755
--- a/scripts/sign-file
+++ b/scripts/sign-file
@@ -403,11 +403,11 @@ my $info = pack("CCCCCxxxN",
if ($verbose) {
print "Size of unsigned module: ", length($unsigned_module), "\n";
- print "Size of magic number : ", length($magic_number), "\n";
print "Size of signer's name : ", length($signers_name), "\n";
print "Size of key identifier : ", length($key_identifier), "\n";
print "Size of signature : ", length($signature), "\n";
print "Size of informaton : ", length($info), "\n";
+ print "Size of magic number : ", length($magic_number), "\n";
print "Signer's name : '", $signers_name, "'\n";
print "Digest : $dgst\n";
}
@@ -416,11 +416,11 @@ open(FD, ">$dest") || die $dest;
binmode FD;
print FD
$unsigned_module,
- $magic_number,
$signers_name,
$key_identifier,
$signature,
- $info
+ $info,
+ $magic_number
;
close FD || die $dest;
David Howells <[email protected]> writes:
> Emit the magic string that indicates a module has a signature after the
> signature data instead of before it. This allows module_sig_check() to be
> made simpler and faster by the elimination of the search for the magic string.
> Instead we just need to do a single memcmp().
>
> This works because at the end of the signature data there is the fixed-length
> signature information block. This block then falls immediately prior to the
> magic number.
>
>>From the contents of the information block, it is trivial to calculate the size
> of the signature data and thus the size of the actual module data.
Meh, I really wanted to separate the module signature locating (my
problem) from the decoding and checking (your problem).
If we're going to require such a header, we can simplify further (untested!).
BTW, I'm not convinced your use of bitfields here is portable; it may be
portable enough for Linux though.
Cheers,
Rusty.
diff --git a/include/linux/module.h b/include/linux/module.h
index 7760c6d..bfd1b2d 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,9 +21,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
@@ -656,4 +653,29 @@ static inline void module_bug_finalize(const Elf_Ehdr *hdr,
static inline void module_bug_cleanup(struct module *mod) {}
#endif /* CONFIG_GENERIC_BUG */
+#ifdef CONFIG_MODULE_SIG
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+/*
+ * Appended module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ enum pkey_algo algo : 8; /* Public-key crypto algorithm */
+ enum pkey_hash_algo hash : 8; /* Digest algorithm */
+ enum pkey_id_type id_type : 8; /* Key identifier type */
+ u8 signer_len; /* Length of signer's name */
+ u8 key_id_len; /* Length of key identifier */
+ u8 __pad[3];
+ __be32 sig_len; /* Length of signature data */
+ char marker[sizeof(MODULE_SIG_STRING)-1];
+};
+#endif /* CONFIG_MODULE_SIG */
+
#endif /* _LINUX_MODULE_H */
diff --git a/kernel/module.c b/kernel/module.c
index 6085f5e..254d6a3 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2424,14 +2424,17 @@ static int module_sig_check(struct load_info *info,
const void *mod, unsigned long *_len)
{
int err = -ENOKEY;
- unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+ struct module_signature sig;
unsigned long len = *_len;
- if (len > markerlen &&
- memcmp(mod + len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
- /* We truncate the module to discard the signature */
- *_len -= markerlen;
- err = mod_verify_sig(mod, _len);
+ if (len > sizeof(sig)) {
+ memcpy(&sig, mod + len - sizeof(sig), sizeof(sig));
+
+ if (!memcmp(sig.marker, MODULE_SIG_STRING, sizeof(sig.marker))) {
+ /* We truncate the module to discard the signature */
+ *_len -= sizeof(sig);
+ err = mod_verify_sig(mod, &sig, _len);
+ }
}
if (!err) {
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index d492a23..db8e836 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -17,26 +17,6 @@
#include "module-internal.h"
/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
- *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
- */
-struct module_signature {
- enum pkey_algo algo : 8; /* Public-key crypto algorithm */
- enum pkey_hash_algo hash : 8; /* Digest algorithm */
- enum pkey_id_type id_type : 8; /* Key identifier type */
- u8 signer_len; /* Length of signer's name */
- u8 key_id_len; /* Length of key identifier */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
-
-/*
* Digest the module contents.
*/
static struct public_key_signature *mod_make_digest(enum pkey_hash_algo hash,
@@ -183,10 +163,10 @@ static struct key *request_asymmetric_key(const char *signer, size_t signer_len,
/*
* Verify the signature on a module.
*/
-int mod_verify_sig(const void *mod, unsigned long *_modlen)
+int mod_verify_sig(const void *mod,
+ const struct module_signature *ms, unsigned long *_modlen)
{
struct public_key_signature *pks;
- struct module_signature ms;
struct key *key;
const void *sig;
size_t modlen = *_modlen, sig_len;
@@ -194,44 +174,38 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
pr_devel("==>%s(,%lu)\n", __func__, modlen);
- 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);
+ 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;
+ modlen -= (size_t)ms->signer_len + ms.key_id_len;
*_modlen = modlen;
sig = mod + modlen;
/* For the moment, only support RSA and X.509 identifiers */
- if (ms.algo != PKEY_ALGO_RSA ||
- ms.id_type != PKEY_ID_X509)
+ if (ms->algo != PKEY_ALGO_RSA ||
+ ms->id_type != PKEY_ID_X509)
return -ENOPKG;
- if (ms.hash >= PKEY_HASH__LAST ||
+ if (ms->hash >= PKEY_HASH__LAST ||
!pkey_hash_algo[ms.hash])
return -ENOPKG;
- key = request_asymmetric_key(sig, ms.signer_len,
- sig + ms.signer_len, ms.key_id_len);
+ key = request_asymmetric_key(sig, ms->signer_len,
+ sig + ms->signer_len, ms->key_id_len);
if (IS_ERR(key))
return PTR_ERR(key);
- pks = mod_make_digest(ms.hash, mod, modlen);
+ pks = mod_make_digest(ms->hash, mod, modlen);
if (IS_ERR(pks)) {
ret = PTR_ERR(pks);
goto error_put_key;
}
- ret = mod_extract_mpi_array(pks, sig + ms.signer_len + ms.key_id_len,
+ ret = mod_extract_mpi_array(pks, sig + ms->signer_len + ms->key_id_len,
sig_len);
if (ret < 0)
goto error_free_pks;
Rusty Russell <[email protected]> wrote:
> Meh, I really wanted to separate the module signature locating (my
> problem) from the decoding and checking (your problem).
You could split mod_verify_sig() at the:
/* For the moment, only support RSA and X.509 identifiers */
comment. At that point we have verified the size fields of the
module_signature struct and we have determined mod, modlen and sig.
Assuming a detached signature is going to look the same as an attached
signature, just without the magic number[1], then you can duplicate the first
part of mod_verify_sig() with the size checks.
[1] The signature may need to be kept small to make sure it will fit within an
xattr, just in case the host fs has limits.
> If we're going to require such a header, we can simplify further (untested!).
Do we want to require that a detached signature be exactly the same as an
appended signature, structure, magic string and all?
I'm also slightly against copying the magic string to the stack if we can avoid
it as the compiler can't recover the space before calling mod_verify_sig().
Besides, there's no particular need to copy the magic string.
> BTW, I'm not convinced your use of bitfields here is portable; it may be
> portable enough for Linux though.
I consulted a gcc engineer and he suggested that it's arch dependent, so I
should probably use u8. Since the enums are basically int-sized types, the
compiler might reorder them depending on endianness and might insert a byte of
padding.
It's just that I prefer the enums for documentary sake.
David
David Howells <[email protected]> writes:
> Rusty Russell <[email protected]> wrote:
>
>> Meh, I really wanted to separate the module signature locating (my
>> problem) from the decoding and checking (your problem).
>
> You could split mod_verify_sig() at the:
>
> /* For the moment, only support RSA and X.509 identifiers */
>
> comment. At that point we have verified the size fields of the
> module_signature struct and we have determined mod, modlen and sig.
>
> Assuming a detached signature is going to look the same as an attached
> signature, just without the magic number[1], then you can duplicate the first
> part of mod_verify_sig() with the size checks.
>
> [1] The signature may need to be kept small to make sure it will fit within an
> xattr, just in case the host fs has limits.
>
>> If we're going to require such a header, we can simplify further (untested!).
>
> Do we want to require that a detached signature be exactly the same as an
> appended signature, structure, magic string and all?
AFAICT the IMA patches already have an xattr format they use for every
other file, they'll want the same thing here.
> I'm also slightly against copying the magic string to the stack if we can avoid
> it as the compiler can't recover the space before calling mod_verify_sig().
> Besides, there's no particular need to copy the magic string.
You're the one who made this new format up, I'm just trying to clean it.
If the module has to end in 'struct sig' then 'string', the put that in
the damn struct definition. structs are a great, readable way for
programmers to know how something is laid out. Use them.
I'll hand this one, it's just a cleanup.
>> BTW, I'm not convinced your use of bitfields here is portable; it may be
>> portable enough for Linux though.
>
> I consulted a gcc engineer and he suggested that it's arch dependent, so I
> should probably use u8. Since the enums are basically int-sized types, the
> compiler might reorder them depending on endianness and might insert a byte of
> padding.
>
> It's just that I prefer the enums for documentary sake.
I prefer enums too, but it'd suck to fix this later.
Patch appreciated,
Rusty.