crypto/simd: correctly take reqsize of wrapped skcipher into account

The simd wrapper's skcipher request context structure consists
of a single subrequest whose size is taken from the subordinate
skcipher. However, in simd_skcipher_init(), the reqsize that is
retrieved is not from the subordinate skcipher but from the
cryptd request structure, whose size is completely unrelated to
the actual wrapped skcipher.

Reported-by: Qian Cai <cai@gmx.us>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 crypto/simd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.19.1

On 8 November 2018 at 23:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The simd wrapper's skcipher request context structure consists

> of a single subrequest whose size is taken from the subordinate

> skcipher. However, in simd_skcipher_init(), the reqsize that is

> retrieved is not from the subordinate skcipher but from the

> cryptd request structure, whose size is completely unrelated to

> the actual wrapped skcipher.

>

> Reported-by: Qian Cai <cai@gmx.us>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  crypto/simd.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/crypto/simd.c b/crypto/simd.c

> index ea7240be3001..2f3d6e897afc 100644

> --- a/crypto/simd.c

> +++ b/crypto/simd.c

> @@ -125,7 +125,7 @@ static int simd_skcipher_init(struct crypto_skcipher *tfm)

>         ctx->cryptd_tfm = cryptd_tfm;

>

>         reqsize = sizeof(struct skcipher_request);

> -       reqsize += crypto_skcipher_reqsize(&cryptd_tfm->base);

> +       reqsize += crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm));

>


This should be

reqsize += max(crypto_skcipher_reqsize(&cryptd_tfm->base);
       crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm)));

since the cryptd path in simd still needs some space in the subreq for
the completion.

> On Nov 8, 2018, at 6:33 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> 

> On 8 November 2018 at 23:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> The simd wrapper's skcipher request context structure consists

>> of a single subrequest whose size is taken from the subordinate

>> skcipher. However, in simd_skcipher_init(), the reqsize that is

>> retrieved is not from the subordinate skcipher but from the

>> cryptd request structure, whose size is completely unrelated to

>> the actual wrapped skcipher.

>> 

>> Reported-by: Qian Cai <cai@gmx.us>

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>> crypto/simd.c | 2 +-

>> 1 file changed, 1 insertion(+), 1 deletion(-)

>> 

>> diff --git a/crypto/simd.c b/crypto/simd.c

>> index ea7240be3001..2f3d6e897afc 100644

>> --- a/crypto/simd.c

>> +++ b/crypto/simd.c

>> @@ -125,7 +125,7 @@ static int simd_skcipher_init(struct crypto_skcipher *tfm)

>>        ctx->cryptd_tfm = cryptd_tfm;

>> 

>>        reqsize = sizeof(struct skcipher_request);

>> -       reqsize += crypto_skcipher_reqsize(&cryptd_tfm->base);

>> +       reqsize += crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm));

>> 

> 

> This should be

> 

> reqsize += max(crypto_skcipher_reqsize(&cryptd_tfm->base);

>       crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm)));

> 

> since the cryptd path in simd still needs some space in the subreq for

> the completion.

Tested-by: Qian Cai <cai@gmx.us>

On Fri, Nov 09, 2018 at 12:33:23AM +0100, Ard Biesheuvel wrote:
>

> This should be

> 

> reqsize += max(crypto_skcipher_reqsize(&cryptd_tfm->base);

>        crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm)));

> 

> since the cryptd path in simd still needs some space in the subreq for

> the completion.


OK this is what I applied to the cryptodev tree, please double-check
to see if I did anything silly:

diff --git a/crypto/simd.c b/crypto/simd.c
index ea7240be3001..78e8d037ae2b 100644
--- a/crypto/simd.c
+++ b/crypto/simd.c
@@ -124,8 +124,9 @@ static int simd_skcipher_init(struct crypto_skcipher *tfm)
 
 	ctx->cryptd_tfm = cryptd_tfm;
 
-	reqsize = sizeof(struct skcipher_request);
-	reqsize += crypto_skcipher_reqsize(&cryptd_tfm->base);
+	reqsize = crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm));
+	reqsize = max(reqsize, crypto_skcipher_reqsize(&cryptd_tfm->base));
+	reqsize += sizeof(struct skcipher_request);
 
 	crypto_skcipher_set_reqsize(tfm, reqsize);
 
Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Fri, Nov 09, 2018 at 05:44:47PM +0800, Herbert Xu wrote:
> On Fri, Nov 09, 2018 at 12:33:23AM +0100, Ard Biesheuvel wrote:

> >

> > This should be

> > 

> > reqsize += max(crypto_skcipher_reqsize(&cryptd_tfm->base);

> >        crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm)));

> > 

> > since the cryptd path in simd still needs some space in the subreq for

> > the completion.

> 

> OK this is what I applied to the cryptodev tree, please double-check

> to see if I did anything silly:


I meant the crypto tree rather than cryptodev.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On 9 November 2018 at 10:45, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Nov 09, 2018 at 05:44:47PM +0800, Herbert Xu wrote:

>> On Fri, Nov 09, 2018 at 12:33:23AM +0100, Ard Biesheuvel wrote:

>> >

>> > This should be

>> >

>> > reqsize += max(crypto_skcipher_reqsize(&cryptd_tfm->base);

>> >        crypto_skcipher_reqsize(cryptd_skcipher_child(cryptd_tfm)));

>> >

>> > since the cryptd path in simd still needs some space in the subreq for

>> > the completion.

>>

>> OK this is what I applied to the cryptodev tree, please double-check

>> to see if I did anything silly:

>

> I meant the crypto tree rather than cryptodev.

>


That looks fine. Thanks Herbert.