[v2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature()

From: Roberto Sassu <roberto.sassu@huawei.com>

Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
mapping") checks that both the signature and the digest reside in the
linear mapping area.

However, more recently commit ba14a194a434c ("fork: Add generic vmalloced
stack support"), made it possible to move the stack in the vmalloc area,
which is not contiguous, and thus not suitable for sg_set_buf() which needs
adjacent pages.

Always make a copy of the signature and digest in the same buffer used to
store the key and its parameters, and pass them to sg_set_buf(). Prefer it
to conditionally doing the copy if necessary, to keep the code simple. The
buffer allocated with kmalloc() is in the linear mapping area.

Cc: stable@vger.kernel.org # 4.9.x
Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
Suggested-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 crypto/asymmetric_keys/public_key.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

On Fri, Dec 09, 2022 at 04:06:33PM +0100, Roberto Sassu wrote:
> +	/* key is used to store the sig and digest too. */
> +	key = kmalloc(key_max_len, GFP_KERNEL);
>  	if (!key)
>  		goto error_free_req;

Maybe just call this 'buf', as the key is just one of the purposes the buffer is
used for now.

> +	/* Cannot use one scatterlist. The first needs to be s->s_size long. */
> +	sg_set_buf(&src_sg[0], key, sig->s_size);
> +	sg_set_buf(&src_sg[1], key + sig->s_size, sig->digest_size);
>  	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
>  				   sig->digest_size);

AFAIK, none of the crypto APIs that operate on 'scatterlist' are supposed to
care how the data is divided up into scatterlist elements.  So it sounds like
there is another bug that needs to be fixed.  It should be fixed, not worked
around.

- Eric

On Fri, 2022-12-09 at 11:04 -0800, Eric Biggers wrote:
> On Fri, Dec 09, 2022 at 04:06:33PM +0100, Roberto Sassu wrote:
> > +	/* key is used to store the sig and digest too. */
> > +	key = kmalloc(key_max_len, GFP_KERNEL);
> >  	if (!key)
> >  		goto error_free_req;
> 
> Maybe just call this 'buf', as the key is just one of the purposes the buffer is
> used for now.

Yes, better.

> > +	/* Cannot use one scatterlist. The first needs to be s->s_size long. */
> > +	sg_set_buf(&src_sg[0], key, sig->s_size);
> > +	sg_set_buf(&src_sg[1], key + sig->s_size, sig->digest_size);
> >  	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
> >  				   sig->digest_size);
> 
> AFAIK, none of the crypto APIs that operate on 'scatterlist' are supposed to
> care how the data is divided up into scatterlist elements.  So it sounds like
> there is another bug that needs to be fixed.  It should be fixed, not worked
> around.

The problem is a misalignment between req->src_len (set to sig->s_size
by akcipher_request_set_crypt()) and the length of the scatterlist (if
we set the latter to sig->s_size + sig->digest_size).

When rsa_enc() calls mpi_read_raw_from_sgl(), it passes req->src_len as
argument, and the latter allocates the MPI according to that. However,
it does parsing depending on the length of the scatterlist.

If there are two scatterlists, it is not a problem, there is no
misalignment. mpi_read_raw_from_sgl() picks the first. If there is just
one, mpi_read_raw_from_sgl() parses all data there.

Roberto

On Wed, Dec 21, 2022 at 02:53:58PM +0800, Herbert Xu wrote:
> On Tue, Dec 20, 2022 at 08:30:16PM +0000, Eric Biggers wrote:
> >
> > > Tried, could not boot the UML kernel.
> > > 
> > > After looking, it seems we have to call sg_miter_stop(). Or alternatively,
> > > we could let sg_miter_next() be called but not writing anything inside the
> > > loop.
> > > 
> > > With either of those fixes, the tests pass (using one scatterlist).
> 
> Thanks for the quick feedback Roberto!
> 
> > I think it should look like:
> > 
> > 	while (nbytes) {
> > 		sg_miter_next(&miter);
> > 		...
> > 	}
> > 	sg_miter_stop(&miter);
> 
> You're right Eric.  However, we could also do it by simply not
> checking nbytes since we already set nents according to nbytes
> at the top of the function.
> 
> ---8<---
> The helper mpi_read_raw_from_sgl sets the number of entries in
> the SG list according to nbytes.  However, if the last entry
> in the SG list contains more data than nbytes, then it may overrun
> the buffer because it only allocates enough memory for nbytes.
> 
> Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
> Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
> index 39c4c6731094..157ef532a6a2 100644
> --- a/lib/mpi/mpicoder.c
> +++ b/lib/mpi/mpicoder.c
> @@ -504,7 +501,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
>  
>  	while (sg_miter_next(&miter)) {
>  		buff = miter.addr;
> -		len = miter.length;
> +		len = min_t(unsigned, miter.length, nbytes);
> +		nbytes -= len;
>  
>  		for (x = 0; x < len; x++) {
>  			a <<= 8;

That's fine, I guess.  One quirk of the above approach is that if the last
needed element of the scatterlist has a lot of extra pages, this will iterate
through all those extra pages, processing 0 bytes from each.  It could just stop
when done.  I suppose it's not worth worrying about that case, though.

- Eric

On Wed, Dec 21, 2022 at 12:53:29PM -0800, Eric Biggers wrote:
>
> That's fine, I guess.  One quirk of the above approach is that if the last
> needed element of the scatterlist has a lot of extra pages, this will iterate
> through all those extra pages, processing 0 bytes from each.  It could just stop
> when done.  I suppose it's not worth worrying about that case, though.

Ideally this should be handled in the sg_miter interface, IOW,
it should allow us to cap the SG list at a certain number of bytes
as opposed to a certain number of entries.

Cheers,