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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y5JwpdGF50oFKw0z@sol.localdomain>
Date:   Thu, 8 Dec 2022 15:17:57 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Roberto Sassu <roberto.sassu@...weicloud.com>
Cc:     dhowells@...hat.com, herbert@...dor.apana.org.au,
        davem@...emloft.net, zohar@...ux.ibm.com,
        dmitry.kasatkin@...il.com, paul@...l-moore.com, jmorris@...ei.org,
        serge@...lyn.com, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        Roberto Sassu <roberto.sassu@...wei.com>,
        stable@...r.kernel.org
Subject: Re: [PATCH] KEYS: asymmetric: Make a copy of sig and digest in
 vmalloced stack

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