2020-08-12 09:26:26

by Ondrej Mosnacek

Subject: [PATCH] crypto: af_alg - fix uninitialized ctx->init

This new member of struct af_alg_ctx was not being initialized before
use, leading to random errors. Found via libkcapi testsuite.

Cc: Stephan Mueller <[email protected]>
Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when ctx->more is zero")
Signed-off-by: Ondrej Mosnacek <[email protected]>
---
crypto/algif_aead.c | 1 +
crypto/algif_skcipher.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index d48d2156e6210..9b5bd0ff3c47d 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -563,6 +563,7 @@ static int aead_accept_parent_nokey(void *private, struct sock *sk)
ctx->more = 0;
ctx->merge = 0;
ctx->enc = 0;
+ ctx->init = 0;
ctx->aead_assoclen = 0;
crypto_init_wait(&ctx->wait);

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a51ba22fef58f..0de035b991943 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -350,6 +350,7 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
ctx->more = 0;
ctx->merge = 0;
ctx->enc = 0;
+ ctx->init = 0;
crypto_init_wait(&ctx->wait);

ask->private = ctx;
--
2.26.2

2020-08-12 12:36:38

by Herbert Xu

Subject: Re: [PATCH] crypto: af_alg - fix uninitialized ctx->init

On Wed, Aug 12, 2020 at 11:22:32AM +0200, Ondrej Mosnacek wrote:
> This new member of struct af_alg_ctx was not being initialized before
> use, leading to random errors. Found via libkcapi testsuite.
>
> Cc: Stephan Mueller <[email protected]>
> Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when ctx->more is zero")
> Signed-off-by: Ondrej Mosnacek <[email protected]>
> ---
> crypto/algif_aead.c | 1 +
> crypto/algif_skcipher.c | 1 +
> 2 files changed, 2 insertions(+)

Thanks for the patch.

> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index d48d2156e6210..9b5bd0ff3c47d 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -563,6 +563,7 @@ static int aead_accept_parent_nokey(void *private, struct sock *sk)
> ctx->more = 0;
> ctx->merge = 0;
> ctx->enc = 0;
> + ctx->init = 0;
> ctx->aead_assoclen = 0;
> crypto_init_wait(&ctx->wait);

This isn't necessary because there is a memset on ctx already.

> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index a51ba22fef58f..0de035b991943 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -350,6 +350,7 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
> ctx->more = 0;
> ctx->merge = 0;
> ctx->enc = 0;
> + ctx->init = 0;
> crypto_init_wait(&ctx->wait);

We should add a memset here for skcipher and get rid of these
zero assignments.

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

2020-08-12 12:47:59

by Ondrej Mosnacek

Subject: Re: [PATCH] crypto: af_alg - fix uninitialized ctx->init

On Wed, Aug 12, 2020 at 2:33 PM Herbert Xu <[email protected]> wrote:
> On Wed, Aug 12, 2020 at 11:22:32AM +0200, Ondrej Mosnacek wrote:
> > This new member of struct af_alg_ctx was not being initialized before
> > use, leading to random errors. Found via libkcapi testsuite.
> >
> > Cc: Stephan Mueller <[email protected]>
> > Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when ctx->more is zero")
> > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > ---
> > crypto/algif_aead.c | 1 +
> > crypto/algif_skcipher.c | 1 +
> > 2 files changed, 2 insertions(+)
>
> Thanks for the patch.
>
> > diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> > index d48d2156e6210..9b5bd0ff3c47d 100644
> > --- a/crypto/algif_aead.c
> > +++ b/crypto/algif_aead.c
> > @@ -563,6 +563,7 @@ static int aead_accept_parent_nokey(void *private, struct sock *sk)
> > ctx->more = 0;
> > ctx->merge = 0;
> > ctx->enc = 0;
> > + ctx->init = 0;
> > ctx->aead_assoclen = 0;
> > crypto_init_wait(&ctx->wait);
>
> This isn't necessary because there is a memset on ctx already.
>
> > diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> > index a51ba22fef58f..0de035b991943 100644
> > --- a/crypto/algif_skcipher.c
> > +++ b/crypto/algif_skcipher.c
> > @@ -350,6 +350,7 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
> > ctx->more = 0;
> > ctx->merge = 0;
> > ctx->enc = 0;
> > + ctx->init = 0;
> > crypto_init_wait(&ctx->wait);
>
> We should add a memset here for skcipher and get rid of these
> zero assignments.

Makes sense, will do as you suggest in v2.

Thanks,

--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.