Re: [RFC PATCH 12/20] PKCS#7: Make the signature a pointer rather than embedding it [ver #2]

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

Hi David,

On Tue, 2016-01-19 at 11:31 +0000, David Howells wrote:
> Point to the public_key_signature struct from the pkcs7_signed_info struct
> rather than embedding it.  This makes it easier to have it take an
> arbitrary number of MPIs in future.

Just a reminder ...

Reviewing patches isn't easy no matter how well written and documented,
especially large patch sets.   For this reason, patch sets should be
limited to the patches that are required to accomplish the patch set
goal.  In this case, that goal is to change  "how certificates/keys are
determined to be trusted."

Although this patch is straight forward, the patch description should
include a reason for including this patch in this patch set.   Is having
an arbitrary number of MPIs included in this patch set?  Could this
patch be deferred?

> We also save a copy of the digest in the signature without sharing the
> memory with the crypto layer metadata.  This means we can use
> public_key_free() to get rid of the signature record.

Again, is this needed to accomplish the patch set goals?

> Signed-off-by: David Howells <dhowells@redhat.com>

Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

> ---
> 
>  crypto/asymmetric_keys/pkcs7_parser.c |   38 ++++++++++++++----------
>  crypto/asymmetric_keys/pkcs7_parser.h |   10 ++----
>  crypto/asymmetric_keys/pkcs7_trust.c  |    4 +-
>  crypto/asymmetric_keys/pkcs7_verify.c |   53 +++++++++++++++++----------------
>  4 files changed, 56 insertions(+), 49 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
> index 7b69783cff99..8454ae5b5aa8 100644
> --- a/crypto/asymmetric_keys/pkcs7_parser.c
> +++ b/crypto/asymmetric_keys/pkcs7_parser.c
> @@ -44,9 +44,7 @@ struct pkcs7_parse_context {
>  static void pkcs7_free_signed_info(struct pkcs7_signed_info *sinfo)
>  {
>  	if (sinfo) {
> -		mpi_free(sinfo->sig.mpi[0]);
> -		kfree(sinfo->sig.digest);
> -		kfree(sinfo->signing_cert_id);
> +		public_key_free(NULL, sinfo->sig);
>  		kfree(sinfo);
>  	}
>  }
> @@ -125,6 +123,10 @@ struct pkcs7_message *pkcs7_parse_message(const void *data, size_t datalen)
>  	ctx->sinfo = kzalloc(sizeof(struct pkcs7_signed_info), GFP_KERNEL);
>  	if (!ctx->sinfo)
>  		goto out_no_sinfo;
> +	ctx->sinfo->sig = kzalloc(sizeof(struct public_key_signature),
> +				  GFP_KERNEL);
> +	if (!ctx->sinfo->sig)
> +		goto out_no_sig;
> 
>  	ctx->data = (unsigned long)data;
>  	ctx->ppcerts = &ctx->certs;
> @@ -150,6 +152,7 @@ out:
>  		ctx->certs = cert->next;
>  		x509_free_certificate(cert);
>  	}
> +out_no_sig:
>  	pkcs7_free_signed_info(ctx->sinfo);
>  out_no_sinfo:
>  	pkcs7_free_message(ctx->msg);
> @@ -219,25 +222,25 @@ int pkcs7_sig_note_digest_algo(void *context, size_t hdrlen,
> 
>  	switch (ctx->last_oid) {
>  	case OID_md4:
> -		ctx->sinfo->sig.pkey_hash_algo = HASH_ALGO_MD4;
> +		ctx->sinfo->sig->pkey_hash_algo = HASH_ALGO_MD4;
>  		break;
>  	case OID_md5:
> -		ctx->sinfo->sig.pkey_hash_algo = HASH_ALGO_MD5;
> +		ctx->sinfo->sig->pkey_hash_algo = HASH_ALGO_MD5;
>  		break;
>  	case OID_sha1:
> -		ctx->sinfo->sig.pkey_hash_algo = HASH_ALGO_SHA1;
> +		ctx->sinfo->sig->pkey_hash_algo = HASH_ALGO_SHA1;
>  		break;
>  	case OID_sha256:
> -		ctx->sinfo->sig.pkey_hash_algo = HASH_ALGO_SHA256;
> +		ctx->sinfo->sig->pkey_hash_algo = HASH_ALGO_SHA256;
>  		break;
>  	case OID_sha384:
> -		ctx->sinfo->sig.pkey_hash_algo = HASH_ALGO_SHA384;
> +		ctx->sinfo->sig->pkey_hash_algo = HASH_ALGO_SHA384;
>  		break;
>  	case OID_sha512:
> -		ctx->sinfo->sig.pkey_hash_algo = HASH_ALGO_SHA512;
> +		ctx->sinfo->sig->pkey_hash_algo = HASH_ALGO_SHA512;
>  		break;
>  	case OID_sha224:
> -		ctx->sinfo->sig.pkey_hash_algo = HASH_ALGO_SHA224;
> +		ctx->sinfo->sig->pkey_hash_algo = HASH_ALGO_SHA224;
>  	default:
>  		printk("Unsupported digest algo: %u\n", ctx->last_oid);
>  		return -ENOPKG;
> @@ -256,7 +259,7 @@ int pkcs7_sig_note_pkey_algo(void *context, size_t hdrlen,
> 
>  	switch (ctx->last_oid) {
>  	case OID_rsaEncryption:
> -		ctx->sinfo->sig.pkey_algo = PKEY_ALGO_RSA;
> +		ctx->sinfo->sig->pkey_algo = PKEY_ALGO_RSA;
>  		break;
>  	default:
>  		printk("Unsupported pkey algo: %u\n", ctx->last_oid);
> @@ -617,16 +620,17 @@ int pkcs7_sig_note_signature(void *context, size_t hdrlen,
>  			     const void *value, size_t vlen)
>  {
>  	struct pkcs7_parse_context *ctx = context;
> +	struct public_key_signature *sig = ctx->sinfo->sig;
>  	MPI mpi;
> 
> -	BUG_ON(ctx->sinfo->sig.pkey_algo != PKEY_ALGO_RSA);
> +	BUG_ON(sig->pkey_algo != PKEY_ALGO_RSA);
> 
>  	mpi = mpi_read_raw_data(value, vlen);
>  	if (!mpi)
>  		return -ENOMEM;
> 
> -	ctx->sinfo->sig.mpi[0] = mpi;
> -	ctx->sinfo->sig.nr_mpi = 1;
> +	sig->mpi[0] = mpi;
> +	sig->nr_mpi = 1;
>  	return 0;
>  }
> 
> @@ -662,12 +666,16 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen,
> 
>  	pr_devel("SINFO KID: %u [%*phN]\n", kid->len, kid->len, kid->data);
> 
> -	sinfo->signing_cert_id = kid;
> +	sinfo->sig->auth_ids[0] = kid;
>  	sinfo->index = ++ctx->sinfo_index;
>  	*ctx->ppsinfo = sinfo;
>  	ctx->ppsinfo = &sinfo->next;
>  	ctx->sinfo = kzalloc(sizeof(struct pkcs7_signed_info), GFP_KERNEL);
>  	if (!ctx->sinfo)
>  		return -ENOMEM;
> +	ctx->sinfo->sig = kzalloc(sizeof(struct public_key_signature),
> +				  GFP_KERNEL);
> +	if (!ctx->sinfo->sig)
> +		return -ENOMEM;
>  	return 0;
>  }
> diff --git a/crypto/asymmetric_keys/pkcs7_parser.h b/crypto/asymmetric_keys/pkcs7_parser.h
> index c8159983ed8f..f4e81074f5e0 100644
> --- a/crypto/asymmetric_keys/pkcs7_parser.h
> +++ b/crypto/asymmetric_keys/pkcs7_parser.h
> @@ -40,19 +40,17 @@ struct pkcs7_signed_info {
>  #define	sinfo_has_ms_statement_type	5
>  	time64_t	signing_time;
> 
> -	/* Issuing cert serial number and issuer's name [PKCS#7 or CMS ver 1]
> -	 * or issuing cert's SKID [CMS ver 3].
> -	 */
> -	struct asymmetric_key_id *signing_cert_id;
> -
>  	/* Message signature.
>  	 *
>  	 * This contains the generated digest of _either_ the Content Data or
>  	 * the Authenticated Attributes [RFC2315 9.3].  If the latter, one of
>  	 * the attributes contains the digest of the the Content Data within
>  	 * it.
> +	 *
> +	 * THis also contains the issuing cert serial number and issuer's name
> +	 * [PKCS#7 or CMS ver 1] or issuing cert's SKID [CMS ver 3].
>  	 */
> -	struct public_key_signature sig;
> +	struct public_key_signature *sig;
>  };
> 
>  struct pkcs7_message {
> diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
> index 7bb9389fd644..400ef359448a 100644
> --- a/crypto/asymmetric_keys/pkcs7_trust.c
> +++ b/crypto/asymmetric_keys/pkcs7_trust.c
> @@ -27,7 +27,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
>  				    struct pkcs7_signed_info *sinfo,
>  				    struct key *trust_keyring)
>  {
> -	struct public_key_signature *sig = &sinfo->sig;
> +	struct public_key_signature *sig = sinfo->sig;
>  	struct x509_certificate *x509, *last = NULL, *p;
>  	struct key *key;
>  	int ret;
> @@ -102,7 +102,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
>  	 * the signed info directly.
>  	 */
>  	key = x509_request_asymmetric_key(trust_keyring,
> -					  sinfo->signing_cert_id,
> +					  sinfo->sig->auth_ids[0],
>  					  NULL,
>  					  false);
>  	if (!IS_ERR(key)) {
> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
> index 0d1173081b5c..3b8124c2cd91 100644
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -25,36 +25,38 @@
>  static int pkcs7_digest(struct pkcs7_message *pkcs7,
>  			struct pkcs7_signed_info *sinfo)
>  {
> +	struct public_key_signature *sig = sinfo->sig;
>  	struct crypto_shash *tfm;
>  	struct shash_desc *desc;
> -	size_t digest_size, desc_size;
> -	void *digest;
> +	size_t desc_size;
>  	int ret;
> 
> -	kenter(",%u,%u", sinfo->index, sinfo->sig.pkey_hash_algo);
> +	kenter(",%u,%u", sinfo->index, sig->pkey_hash_algo);
> 
> -	if (sinfo->sig.pkey_hash_algo >= PKEY_HASH__LAST ||
> -	    !hash_algo_name[sinfo->sig.pkey_hash_algo])
> +	if (sig->pkey_hash_algo >= PKEY_HASH__LAST ||
> +	    !hash_algo_name[sig->pkey_hash_algo])
>  		return -ENOPKG;
> 
>  	/* Allocate the hashing algorithm we're going to need and find out how
>  	 * big the hash operational data will be.
>  	 */
> -	tfm = crypto_alloc_shash(hash_algo_name[sinfo->sig.pkey_hash_algo],
> +	tfm = crypto_alloc_shash(hash_algo_name[sinfo->sig->pkey_hash_algo],
>  				 0, 0);
>  	if (IS_ERR(tfm))
>  		return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> 
>  	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> -	sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
> +	sig->digest_size = crypto_shash_digestsize(tfm);
> 
>  	ret = -ENOMEM;
> -	digest = kzalloc(ALIGN(digest_size, __alignof__(*desc)) + desc_size,
> -			 GFP_KERNEL);
> -	if (!digest)
> +	sig->digest = kmalloc(sig->digest_size, GFP_KERNEL);
> +	if (!sig->digest)
> +		goto error_no_desc;
> +
> +	desc = kzalloc(desc_size, GFP_KERNEL);
> +	if (!desc)
>  		goto error_no_desc;
> 
> -	desc = PTR_ALIGN(digest + digest_size, __alignof__(*desc));
>  	desc->tfm   = tfm;
>  	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> 
> @@ -62,10 +64,11 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
>  	ret = crypto_shash_init(desc);
>  	if (ret < 0)
>  		goto error;
> -	ret = crypto_shash_finup(desc, pkcs7->data, pkcs7->data_len, digest);
> +	ret = crypto_shash_finup(desc, pkcs7->data, pkcs7->data_len,
> +				 sig->digest);
>  	if (ret < 0)
>  		goto error;
> -	pr_devel("MsgDigest = [%*ph]\n", 8, digest);
> +	pr_devel("MsgDigest = [%*ph]\n", 8, sig->digest);
> 
>  	/* However, if there are authenticated attributes, there must be a
>  	 * message digest attribute amongst them which corresponds to the
> @@ -80,14 +83,15 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
>  			goto error;
>  		}
> 
> -		if (sinfo->msgdigest_len != sinfo->sig.digest_size) {
> +		if (sinfo->msgdigest_len != sig->digest_size) {
>  			pr_debug("Sig %u: Invalid digest size (%u)\n",
>  				 sinfo->index, sinfo->msgdigest_len);
>  			ret = -EBADMSG;
>  			goto error;
>  		}
> 
> -		if (memcmp(digest, sinfo->msgdigest, sinfo->msgdigest_len) != 0) {
> +		if (memcmp(sig->digest, sinfo->msgdigest,
> +			   sinfo->msgdigest_len) != 0) {
>  			pr_debug("Sig %u: Message digest doesn't match\n",
>  				 sinfo->index);
>  			ret = -EKEYREJECTED;
> @@ -99,7 +103,7 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
>  		 * convert the attributes from a CONT.0 into a SET before we
>  		 * hash it.
>  		 */
> -		memset(digest, 0, sinfo->sig.digest_size);
> +		memset(sig->digest, 0, sig->digest_size);
> 
>  		ret = crypto_shash_init(desc);
>  		if (ret < 0)
> @@ -109,17 +113,14 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
>  		if (ret < 0)
>  			goto error;
>  		ret = crypto_shash_finup(desc, sinfo->authattrs,
> -					 sinfo->authattrs_len, digest);
> +					 sinfo->authattrs_len, sig->digest);
>  		if (ret < 0)
>  			goto error;
> -		pr_devel("AADigest = [%*ph]\n", 8, digest);
> +		pr_devel("AADigest = [%*ph]\n", 8, sig->digest);
>  	}
> 
> -	sinfo->sig.digest = digest;
> -	digest = NULL;
> -
>  error:
> -	kfree(digest);
> +	kfree(desc);
>  error_no_desc:
>  	crypto_free_shash(tfm);
>  	kleave(" = %d", ret);
> @@ -146,12 +147,12 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,
>  		 * PKCS#7 message - but I can't be 100% sure of that.  It's
>  		 * possible this will need element-by-element comparison.
>  		 */
> -		if (!asymmetric_key_id_same(x509->id, sinfo->signing_cert_id))
> +		if (!asymmetric_key_id_same(x509->id, sinfo->sig->auth_ids[0]))
>  			continue;
>  		pr_devel("Sig %u: Found cert serial match X.509[%u]\n",
>  			 sinfo->index, certix);
> 
> -		if (x509->pub->pkey_algo != sinfo->sig.pkey_algo) {
> +		if (x509->pub->pkey_algo != sinfo->sig->pkey_algo) {
>  			pr_warn("Sig %u: X.509 algo and PKCS#7 sig algo don't match\n",
>  				sinfo->index);
>  			continue;
> @@ -166,7 +167,7 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,
>  	 */
>  	pr_debug("Sig %u: Issuing X.509 cert not found (#%*phN)\n",
>  		 sinfo->index,
> -		 sinfo->signing_cert_id->len, sinfo->signing_cert_id->data);
> +		 sinfo->sig->auth_ids[0]->len, sinfo->sig->auth_ids[0]->data);
>  	return 0;
>  }
> 
> @@ -325,7 +326,7 @@ static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
>  	}
> 
>  	/* Verify the PKCS#7 binary against the key */
> -	ret = public_key_verify_signature(sinfo->signer->pub, &sinfo->sig);
> +	ret = public_key_verify_signature(sinfo->signer->pub, sinfo->sig);
>  	if (ret < 0)
>  		return ret;
> 
>