On Fri, 2022-12-09 at 15:15 +0100, Roberto Sassu wrote:
> 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()
Of signature length and digest length.
Roberto
> 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
