libkmod/libkmod-signature: rework struct kmod_signature_info

Currently, we use a stack allocated instance which bolts of misc private
data via a void *, while also having an optional free callback.

In kmod we opt to pre-calculate the total size, do a one-off allocation,
copy the data as needed, adjusting the pointers.

Doing the same here, gives us a mixed bag of benefits:
 - shorter and simpler code
 - smaller binary - ~100 bytes off
 - fewer instructions - ~40 per module
 - few allocations - ~2 per module
 - extra bytes are allocated - ~180 per module

The updated code seems far more natural and consistent with the
code-base. Although, if we really want to squeeze more cycles we could
use a reasonably large stack buffer and fallback to heap.

v2:
 - don't leak on d2i_PKCS7_bio failure
 - use +1 (instead of sizeof('\0')) for the null terminator

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Link: https://github.com/kmod-project/kmod/pull/427
Signed-off-by: Lucas De Marchi <demarchi@kernel.org>

diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index d1177102e72675301bd75d53e32ddde7a5f22ed2..cc681e2219c33b5da6a0230038e394418dbe3a13 100644 (file)
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -173,11 +173,8 @@ struct kmod_signature_info {
        const char *hash_algo, *id_type;
        const char *sig;
        size_t sig_len;
-       void (*free)(void *);
-       void *private;
 };
-_must_check_ _nonnull_all_ bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info);
-_nonnull_all_ void kmod_module_signature_info_free(struct kmod_signature_info *sig_info);
+_must_check_ _nonnull_all_ bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info **sig_info);
 
 /* libkmod-builtin.c */
 _nonnull_all_ ssize_t kmod_builtin_get_modinfo(struct kmod_ctx *ctx, const char *modname, char ***modinfo);

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 72beec14f7bde9f234ead941e929d72fe879df58..e849dd9f1282bac3e0031dec0cbbfe54e25edfcd 100644 (file)
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -1869,7 +1869,7 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod,
 {
        char **strings;
        int i, count, ret = -ENOMEM;
-       struct kmod_signature_info sig_info = {};
+       struct kmod_signature_info *sig_info = NULL;
 
        if (mod == NULL || list == NULL)
                return -ENOENT;
@@ -1918,36 +1918,36 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod,
                struct kmod_list *n;
 
                n = kmod_module_info_append(list, "sig_id", strlen("sig_id"),
-                                           sig_info.id_type, strlen(sig_info.id_type));
+                                           sig_info->id_type, strlen(sig_info->id_type));
                if (n == NULL)
                        goto list_error;
                count++;
 
                n = kmod_module_info_append(list, "signer", strlen("signer"),
-                                           sig_info.signer, sig_info.signer_len);
+                                           sig_info->signer, sig_info->signer_len);
                if (n == NULL)
                        goto list_error;
                count++;
 
                n = kmod_module_info_append_hex(list, "sig_key", strlen("sig_key"),
-                                               sig_info.key_id, sig_info.key_id_len);
+                                               sig_info->key_id, sig_info->key_id_len);
                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));
+                                           sig_info->hash_algo,
+                                           strlen(sig_info->hash_algo));
                if (n == NULL)
                        goto list_error;
                count++;
 
                /*
-                * Omit sig_info.algo for now, as these
+                * Omit sig_info->algo for now, as these
                 * are currently constant.
                 */
                n = kmod_module_info_append_hex(list, "signature", strlen("signature"),
-                                               sig_info.sig, sig_info.sig_len);
+                                               sig_info->sig, sig_info->sig_len);
 
                if (n == NULL)
                        goto list_error;
@@ -1957,7 +1957,7 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod,
 
 list_error:
        /* aux structures freed in normal case also */
-       kmod_module_signature_info_free(&sig_info);
+       free(sig_info);
 
        if (ret < 0) {
                kmod_module_info_free_list(*list);

diff --git a/libkmod/libkmod-signature.c b/libkmod/libkmod-signature.c
index ff4c57a272087283ad55243a7a1ec1c06b18d803..2af16ded7b095fb1485c3f4150dc7155bf0c3ef4 100644 (file)
--- a/libkmod/libkmod-signature.c
+++ b/libkmod/libkmod-signature.c
@@ -83,8 +83,12 @@ struct module_signature {
 
 static bool fill_default(const char *mem, off_t size,
                         const struct module_signature *modsig, size_t sig_len,
-                        struct kmod_signature_info *sig_info)
+                        struct kmod_signature_info **out_sig_info)
 {
+       struct kmod_signature_info *sig_info = calloc(1, sizeof(*sig_info));
+       if (sig_info == NULL)
+               return false;
+
        size -= sig_len;
        sig_info->sig = mem + size;
        sig_info->sig_len = sig_len;
@@ -99,31 +103,13 @@ static bool fill_default(const char *mem, off_t size,
 
        sig_info->hash_algo = pkey_hash_algo[modsig->hash];
 
+       *out_sig_info = sig_info;
+
        return true;
 }
 
 #if ENABLE_OPENSSL
 
-struct pkcs7_private {
-       PKCS7 *pkcs7;
-       char *key_id;
-       BIGNUM *sno;
-       char *hash_algo;
-};
-
-static void pkcs7_free(void *s)
-{
-       struct kmod_signature_info *si = s;
-       struct pkcs7_private *pvt = si->private;
-
-       PKCS7_free(pvt->pkcs7);
-       BN_free(pvt->sno);
-       free(pvt->key_id);
-       free(pvt->hash_algo);
-       free(pvt);
-       si->private = NULL;
-}
-
 static const char *x509_name_to_str(X509_NAME *name)
 {
        int i;
@@ -150,8 +136,10 @@ static const char *x509_name_to_str(X509_NAME *name)
 }
 
 static bool fill_pkcs7(const char *mem, off_t size, size_t sig_len,
-                      struct kmod_signature_info *sig_info)
+                      struct kmod_signature_info **out_sig_info)
 {
+       struct kmod_signature_info *sig_info;
+       char *p;
        const char *pkcs7_raw;
        PKCS7 *pkcs7;
        STACK_OF(PKCS7_SIGNER_INFO) * sis;
@@ -162,12 +150,9 @@ static bool fill_pkcs7(const char *mem, off_t size, size_t sig_len,
        X509_ALGOR *dig_alg;
        const ASN1_OBJECT *o;
        BIO *in;
-       int len;
-       char *key_id_str;
-       struct pkcs7_private *pvt;
        const char *issuer_str;
-       char *hash_algo;
        int hash_algo_len;
+       size_t total_len;
 
        size -= sig_len;
        pkcs7_raw = mem + size;
@@ -175,13 +160,11 @@ static bool fill_pkcs7(const char *mem, off_t size, size_t sig_len,
        in = BIO_new_mem_buf(pkcs7_raw, sig_len);
 
        pkcs7 = d2i_PKCS7_bio(in, NULL);
-       if (pkcs7 == NULL) {
-               BIO_free(in);
-               return false;
-       }
-
        BIO_free(in);
 
+       if (pkcs7 == NULL)
+               goto err;
+
        sis = PKCS7_get_signer_info(pkcs7);
        if (sis == NULL)
                goto err;
@@ -191,66 +174,86 @@ static bool fill_pkcs7(const char *mem, off_t size, size_t sig_len,
                goto err;
 
        is = si->issuer_and_serial;
-       sig = si->enc_digest;
 
-       PKCS7_SIGNER_INFO_get0_algs(si, NULL, &dig_alg, NULL);
+       /* Calculate the total length */
+       total_len = sizeof(struct kmod_signature_info);
 
-       sig_info->sig = (const char *)ASN1_STRING_get0_data(sig);
-       sig_info->sig_len = ASN1_STRING_length(sig);
+       /* signer */
+       issuer_str = x509_name_to_str(is->issuer);
+       if (issuer_str != NULL)
+               total_len += strlen(issuer_str);
 
+       /* key_id */
        sno_bn = ASN1_INTEGER_to_BN(is->serial, NULL);
        if (sno_bn == NULL)
                goto err;
 
-       len = BN_num_bytes(sno_bn);
-       key_id_str = malloc(len);
-       if (key_id_str == NULL)
-               goto err2;
-       BN_bn2bin(sno_bn, (unsigned char *)key_id_str);
+       total_len += BN_num_bytes(sno_bn);
 
-       sig_info->key_id = key_id_str;
-       sig_info->key_id_len = len;
+       /* hash_algo */
+       PKCS7_SIGNER_INFO_get0_algs(si, NULL, &dig_alg, NULL);
+       X509_ALGOR_get0(&o, NULL, NULL, dig_alg);
+       hash_algo_len = OBJ_obj2txt(NULL, 0, o, 0);
+       if (hash_algo_len < 0)
+               goto err1;
 
-       issuer_str = x509_name_to_str(is->issuer);
+       total_len += hash_algo_len + 1;
+
+       /* sig */
+       sig = si->enc_digest;
+       total_len += ASN1_STRING_length(sig);
+
+       sig_info = calloc(1, total_len);
+       if (sig_info == NULL)
+               goto err1;
+
+       p = (char *)sig_info;
+
+       p += sizeof(struct kmod_signature_info);
+
+       /* signer */
        if (issuer_str != NULL) {
-               sig_info->signer = issuer_str;
+               sig_info->signer = p;
                sig_info->signer_len = strlen(issuer_str);
+
+               memcpy(p, issuer_str, sig_info->signer_len);
+               p += sig_info->signer_len;
        }
 
-       X509_ALGOR_get0(&o, NULL, NULL, dig_alg);
+       /* key_id */
+       sig_info->key_id = p;
+       sig_info->key_id_len = BN_num_bytes(sno_bn);
 
-       // Use OBJ_obj2txt to calculate string length
-       hash_algo_len = OBJ_obj2txt(NULL, 0, o, 0);
-       if (hash_algo_len < 0)
-               goto err3;
-       hash_algo = malloc(hash_algo_len + 1);
-       if (hash_algo == NULL)
-               goto err3;
-       hash_algo_len = OBJ_obj2txt(hash_algo, hash_algo_len + 1, o, 0);
+       BN_bn2bin(sno_bn, (unsigned char *)p);
+       p += sig_info->key_id_len;
+
+       /* hash_algo */
+       sig_info->hash_algo = p;
+
+       hash_algo_len = OBJ_obj2txt(p, hash_algo_len + 1, o, 0);
        if (hash_algo_len < 0)
-               goto err4;
+               goto err2;
+       p += hash_algo_len;
 
-       // Assign libcrypto hash algo string or number
-       sig_info->hash_algo = hash_algo;
+       *p = '\0';
+       p++;
 
-       pvt = malloc(sizeof(*pvt));
-       if (pvt == NULL)
-               goto err4;
+       /* sig */
+       sig_info->sig = p;
+       sig_info->sig_len = ASN1_STRING_length(sig);
+
+       memcpy(p, ASN1_STRING_get0_data(sig), sig_info->sig_len);
 
-       pvt->pkcs7 = pkcs7;
-       pvt->key_id = key_id_str;
-       pvt->sno = sno_bn;
-       pvt->hash_algo = hash_algo;
-       sig_info->private = pvt;
+       BN_free(sno_bn);
+       PKCS7_free(pkcs7);
 
-       sig_info->free = pkcs7_free;
+       *out_sig_info = sig_info;
 
        return true;
-err4:
-       free(hash_algo);
-err3:
-       free(key_id_str);
+
 err2:
+       free(sig_info);
+err1:
        BN_free(sno_bn);
 err:
        PKCS7_free(pkcs7);
@@ -260,9 +263,14 @@ err:
 #else
 
 static bool fill_pkcs7(const char *mem, off_t size, size_t sig_len,
-                      struct kmod_signature_info *sig_info)
+                      struct kmod_signature_info **out_sig_info)
 {
+       struct kmod_signature_info *sig_info = calloc(1, sizeof(*sig_info));
+       if (sig_info == NULL)
+               return false;
+
        sig_info->hash_algo = "unknown";
+       *out_sig_info = sig_info;
        return true;
 }
 
@@ -282,14 +290,14 @@ static bool fill_pkcs7(const char *mem, off_t size, size_t sig_len,
  */
 
 bool kmod_module_signature_info(const struct kmod_file *file,
-                               struct kmod_signature_info *sig_info)
+                               struct kmod_signature_info **sig_info)
 {
        const char *mem;
        const void *contents;
        off_t size;
        struct module_signature modsig;
        size_t sig_len;
-       int ret;
+       bool ret;
 
        ret = kmod_file_get_contents(file, &contents, &size);
        if (ret)
@@ -318,18 +326,17 @@ bool kmod_module_signature_info(const struct kmod_file *file,
            size < (int64_t)sig_len + modsig.signer_len + modsig.key_id_len)
                return false;
 
-       sig_info->id_type = pkey_id_type[modsig.id_type];
-
        switch (modsig.id_type) {
        case PKEY_ID_PKCS7:
-               return fill_pkcs7(mem, size, sig_len, sig_info);
+               ret = fill_pkcs7(mem, size, sig_len, sig_info);
+               break;
        default:
-               return fill_default(mem, size, &modsig, sig_len, sig_info);
+               ret = fill_default(mem, size, &modsig, sig_len, sig_info);
+               break;
        }
-}
 
-void kmod_module_signature_info_free(struct kmod_signature_info *sig_info)
-{
-       if (sig_info->free)
-               sig_info->free(sig_info);
+       if (ret)
+               (*sig_info)->id_type = pkey_id_type[modsig.id_type];
+
+       return ret;
 }