Re: [PATCH] KEYS: asymmetric: Make a copy of sig and digest in vmalloced stack

[Eric Biggers]

On Thu, Dec 08, 2022 at 05:46:10PM +0100, Roberto Sassu wrote:
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index 2f8352e88860..307799ffbc3e 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -363,7 +363,8 @@ int public_key_verify_signature(const struct public_key *pkey,
>  	struct scatterlist src_sg[2];
>  	char alg_name[CRYPTO_MAX_ALG_NAME];
>  	char *key, *ptr;
> -	int ret;
> +	char *sig_s, *digest;
> +	int ret, verif_bundle_len;
>  
>  	pr_devel("==>%s()\n", __func__);
>  
> @@ -400,8 +401,21 @@ int public_key_verify_signature(const struct public_key *pkey,
>  	if (!req)
>  		goto error_free_tfm;
>  
> -	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> -		      GFP_KERNEL);
> +	verif_bundle_len = pkey->keylen + sizeof(u32) * 2 + pkey->paramlen;
> +
> +	sig_s = sig->s;
> +	digest = sig->digest;
> +
> +	if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> +		if (!virt_addr_valid(sig_s))
> +			verif_bundle_len += sig->s_size;
> +
> +		if (!virt_addr_valid(digest))
> +			verif_bundle_len += sig->digest_size;
> +	}
> +
> +	/* key points to a buffer which could contain the sig and digest too. */
> +	key = kmalloc(verif_bundle_len, GFP_KERNEL);
>  	if (!key)
>  		goto error_free_req;
>  
> @@ -424,9 +438,24 @@ int public_key_verify_signature(const struct public_key *pkey,
>  			goto error_free_key;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> +		ptr += pkey->paramlen;
> +
> +		if (!virt_addr_valid(sig_s)) {
> +			sig_s = ptr;
> +			memcpy(sig_s, sig->s, sig->s_size);
> +			ptr += sig->s_size;
> +		}
> +
> +		if (!virt_addr_valid(digest)) {
> +			digest = ptr;
> +			memcpy(digest, sig->digest, sig->digest_size);
> +		}
> +	}
> +
>  	sg_init_table(src_sg, 2);
> -	sg_set_buf(&src_sg[0], sig->s, sig->s_size);
> -	sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);
> +	sg_set_buf(&src_sg[0], sig_s, sig->s_size);
> +	sg_set_buf(&src_sg[1], digest, sig->digest_size);
>  	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
>  				   sig->digest_size);
>  	crypto_init_wait(&cwait);

We should try to avoid adding error-prone special cases.  How about just doing
the copy of the signature and digest unconditionally?  That would be much
simpler.  It would even mean that the scatterlist would only need one element.

Also, the size of buffer needed is only

	max(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
	    sig->s_size + sig->digest_size)

... since the signature and digest aren't needed until the key was already used.

- Eric