Hi Michal,
On Wed, Jan 16, 2013 at 7:18 AM, Michal Marek <mmarek@xxxxxxx> wrote:
> If the module is built with CONFIG_MODULE_SIG, add the the signer's
> name, hexadecimal key id and hash algorithm to the list returned in
> kmod_module_get_info(). The modinfo output then looks like this:
>
> filename:
> /home/mmarek/kmod/testsuite/rootfs-pristine/test-modinfo/ext4-x86_64-sha256.ko
> license: GPL
> description: Fourth Extended Filesystem
> author: Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others
> alias: ext3
> alias: ext2
> depends: mbcache,jbd2
> intree: Y
> vermagic: 3.7.0 SMP mod_unload
> signer: Magrathea: Glacier signing key
> sig_key: E3:C8:FC:A7:3F:B3:1D:DE:84:81:EF:38:E3:4C:DE:4B:0C:FD:1B:F9
> sig_hashalgo: sha256
>
> The signature algorithm (RSA) and key identifier type (X509) are not
> displayed, because they are constant information for every signed
> module. But it would be trivial to add this. Note: No attempt is made at
> verifying the signature, I don't think that modinfo is the right tool
> for this.
Ack. In general the patch looks good, but I have a few comments.
> ---
> Makefile.am | 3 +-
> libkmod/libkmod-module.c | 40 ++++++++++++
> libkmod/libkmod-private.h | 9 +++
> libkmod/libkmod-signature.c | 141 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 192 insertions(+), 1 deletion(-)
> create mode 100644 libkmod/libkmod-signature.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 995f2de..9feaf96 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -62,7 +62,8 @@ libkmod_libkmod_la_SOURCES =\
> libkmod/libkmod-index.h \
> libkmod/libkmod-module.c \
> libkmod/libkmod-file.c \
> - libkmod/libkmod-elf.c
> + libkmod/libkmod-elf.c \
> + libkmod/libkmod-signature.c
>
> EXTRA_DIST += libkmod/libkmod.sym
> EXTRA_DIST += libkmod/README libkmod/COPYING testsuite/COPYING COPYING
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index ae0d9c8..cf7b45f 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -2134,6 +2134,7 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
You forgot to update the docs. The doc says it gets the info from
.modinfo section. This is not true for the signature, which is in the
end of the module.
> struct kmod_elf *elf;
> char **strings;
> int i, count, ret = -ENOMEM;
> + struct kmod_signature_info sig_info;
>
> if (mod == NULL || list == NULL)
> return -ENOENT;
> @@ -2168,6 +2169,45 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
> if (n == NULL)
> goto list_error;
> }
missing blank line here.
> + if (kmod_module_signature_info(mod->file, &sig_info)) {
> + struct kmod_list *n;
> + char *key_hex;
> +
> + n = kmod_module_info_append(list, "signer", strlen("signer"),
> + sig_info.signer, sig_info.signer_len);
> + if (n == NULL)
> + goto list_error;
and here. Also I didn't notice in the first patch that you are freeing
and setting *list to NULL on ENOMEM. Please add this after label
list_error.
> + count++;
missing blank line here, too... so we group each key-val.
> + /* Display the key id as 01:12:DE:AD:BE:EF:... */
> + key_hex = malloc(sig_info.key_id_len * 3);
> + if (key_hex == NULL) {
> + kmod_module_info_free_list(*list);
> + *list = NULL;
> + goto list_error;
As said, these lines should be after list_error
> + }
> + for (i = 0; i < (int)sig_info.key_id_len; i++) {
> + sprintf(key_hex + i * 3, "%02X",
> + (unsigned char)sig_info.key_id[i]);
no need for this cast?
> + if (i < (int)sig_info.key_id_len - 1)
> + key_hex[i * 3 + 2] = ':';
> + }
> + n = kmod_module_info_append(list, "sig_key", strlen("sig_key"),
> + key_hex, sig_info.key_id_len * 3 - 1);
> + free(key_hex);
> + if (n == NULL)
> + goto list_error;
> + count++;
> + n = kmod_module_info_append(list,
> + "sig_hashalgo", strlen("sig_hashalgo"),
> + sig_info.hash_algo, strlen(sig_info.hash_algo));
> + if (n == NULL)
> + goto list_error;
> + count++;
> + /*
> + * Omit sig_info.id_type and sig_info.algo for now, as these
> + * are currently constant.
> + */
> + }
> ret = count;
>
> list_error:
> diff --git a/libkmod/libkmod-private.h b/libkmod/libkmod-private.h
> index 4760733..c2f80e7 100644
> --- a/libkmod/libkmod-private.h
> +++ b/libkmod/libkmod-private.h
> @@ -168,5 +168,14 @@ int kmod_elf_strip_vermagic(struct kmod_elf *elf) _must_check_ __attribute__((no
> */
> int kmod_elf_get_section(const struct kmod_elf *elf, const char *section, const void **buf, uint64_t *buf_size) _must_check_ __attribute__((nonnull(1,2,3,4)));
>
> +/* libkmod-signature.c */
> +struct kmod_signature_info {
> + const char *signer;
> + size_t signer_len;
> + const char *key_id;
> + size_t key_id_len;
> + const char *algo, *hash_algo, *id_type;
> +};
> +int kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info) _must_check_ __attribute__((nonnull(1, 2)));
> /* util functions */
> #include "libkmod-util.h"
> diff --git a/libkmod/libkmod-signature.c b/libkmod/libkmod-signature.c
> new file mode 100644
> index 0000000..1ffe4e4
> --- /dev/null
> +++ b/libkmod/libkmod-signature.c
> @@ -0,0 +1,141 @@
> +/*
> + * libkmod - module signature display
> + *
> + * Copyright (C) 2013 Michal Marek, SUSE
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <endian.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include "libkmod-private.h"
> +
> +/* These types and tables were copied from the 3.7 kernel sources.
> + * As this is just description of the signature format, it should not be
> + * considered derived work (so libkmod can use the LGPL license).
> + */
> +enum pkey_algo {
> + PKEY_ALGO_DSA,
> + PKEY_ALGO_RSA,
> + PKEY_ALGO__LAST
> +};
> +
> +static const char *const pkey_algo[PKEY_ALGO__LAST] = {
> + [PKEY_ALGO_DSA] = "DSA",
> + [PKEY_ALGO_RSA] = "RSA",
> +};
> +
> +enum pkey_hash_algo {
> + PKEY_HASH_MD4,
> + PKEY_HASH_MD5,
> + PKEY_HASH_SHA1,
> + PKEY_HASH_RIPE_MD_160,
> + PKEY_HASH_SHA256,
> + PKEY_HASH_SHA384,
> + PKEY_HASH_SHA512,
> + PKEY_HASH_SHA224,
> + PKEY_HASH__LAST
> +};
> +
> +const char *const pkey_hash_algo[PKEY_HASH__LAST] = {
> + [PKEY_HASH_MD4] = "md4",
> + [PKEY_HASH_MD5] = "md5",
> + [PKEY_HASH_SHA1] = "sha1",
> + [PKEY_HASH_RIPE_MD_160] = "rmd160",
> + [PKEY_HASH_SHA256] = "sha256",
> + [PKEY_HASH_SHA384] = "sha384",
> + [PKEY_HASH_SHA512] = "sha512",
> + [PKEY_HASH_SHA224] = "sha224",
> +};
> +
> +enum pkey_id_type {
> + PKEY_ID_PGP, /* OpenPGP generated key ID */
> + PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
> + PKEY_ID_TYPE__LAST
> +};
> +
> +const char *const pkey_id_type[PKEY_ID_TYPE__LAST] = {
> + [PKEY_ID_PGP] = "PGP",
> + [PKEY_ID_X509] = "X509",
> +};
> +
> +/*
> + * 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 {
> + uint8_t algo; /* Public-key crypto algorithm [enum pkey_algo] */
> + uint8_t hash; /* Digest algorithm [enum pkey_hash_algo] */
> + uint8_t id_type; /* Key identifier type [enum pkey_id_type] */
> + uint8_t signer_len; /* Length of signer's name */
> + uint8_t key_id_len; /* Length of key identifier */
> + uint8_t __pad[3];
> + uint32_t sig_len; /* Length of signature data (big endian) */
> +};
> +
> +#define SIG_MAGIC "~Module signature appended~\n"
> +
> +int kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info)
please make this bool since we only return true or false.
> +{
> + const char *mem;
> + off_t size;
> + const struct module_signature *modsig;
> + size_t sig_len;
> +
> +
> + size = kmod_file_get_size(file);
> + mem = kmod_file_get_contents(file);
> + if (size < (off_t)strlen(SIG_MAGIC))
> + return 0;
> + size -= strlen(SIG_MAGIC);
> + if (memcmp(SIG_MAGIC, mem + size, strlen(SIG_MAGIC)) != 0)
> + return 0;
> +
> + if (size < (off_t)sizeof(struct module_signature))
> + return 0;
> + size -= sizeof(struct module_signature);
> + modsig = (struct module_signature *)(mem + size);
> + if (modsig->algo >= PKEY_ALGO__LAST ||
> + modsig->hash >= PKEY_HASH__LAST ||
> + modsig->id_type >= PKEY_ID_TYPE__LAST)
> + return 0;
> + sig_len = be32toh(modsig->sig_len);
> + if (size < (off_t)(modsig->signer_len + modsig->key_id_len + sig_len))
> + return 0;
> +
> + size -= modsig->key_id_len + sig_len;
> + sig_info->key_id = mem + size;
> + sig_info->key_id_len = modsig->key_id_len;
> +
> + size -= modsig->signer_len;
> + sig_info->signer = mem + size;
> + sig_info->signer_len = modsig->signer_len;
> +
> + sig_info->algo = pkey_algo[modsig->algo];
> + sig_info->hash_algo = pkey_hash_algo[modsig->hash];
> + sig_info->id_type = pkey_id_type[modsig->id_type];
> +
> + return 1;
> +}
> --
> 1.7.10.4
Otherwise it looks good.
cheers
Lucas de Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
