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: #427
Signed-off-by: Lucas De Marchi <demarchi@kernel.org>

Author: evelikov

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);

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);

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,26 +150,21 @@ 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;
 
 	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 @@ static bool fill_pkcs7(const char *mem, off_t size, size_t sig_len,
 #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;
 }