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: <3f1c74f320a288b6581241fc3039103cbcee7b27.camel@huaweicloud.com>
Date:   Fri, 09 Dec 2022 15:15:06 +0100
From:   Roberto Sassu <roberto.sassu@...weicloud.com>
To:     Eric Biggers <ebiggers@...nel.org>
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, 2022-12-08 at 15:17 -0800, Eric Biggers wrote:
> 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.

Took some time to figure out why Redzone was overwritten.

There must be two separate scatterlists. If you set the first only with
the sum of the key length and digest length, mpi_read_raw_from_sgl()
called by rsa_enc() is going to write before the d pointer in MPI.

		for (x = 0; x < len; x++) {
			a <<= 8;
			a |= *buff++;
			if (((z + x + 1) % BYTES_PER_MPI_LIMB) == 0) {
				val->d[j--] = a;
				a = 0;
			}
		}

Roberto

> 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