| Message ID | 20181108225516.9967-1-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto/simd: correctly take reqsize of wrapped skcipher into account | expand |
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,
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,
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.
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)); crypto_skcipher_set_reqsize(tfm, reqsize);
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(-)