Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200812092232.364991-1-omosnace@redhat.com> <20200812123311.GA21384@gondor.apana.org.au>
In-Reply-To: <20200812123311.GA21384@gondor.apana.org.au>
From:   Ondrej Mosnacek <omosnace@redhat.com>
Date:   Wed, 12 Aug 2020 14:47:00 +0200
Message-ID: <CAFqZXNsCDb+C4Kr5i+vRkS1bcughXFuorJDJRgXkzznPigSZfg@mail.gmail.com>
Subject: Re: [PATCH] crypto: af_alg - fix uninitialized ctx->init
To:     Herbert Xu <herbert@gondor.apana.org.au>
Cc:     linux-crypto@vger.kernel.org, Stephan Mueller <smueller@chronox.de>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 12, 2020 at 2:33 PM Herbert Xu <herbert@gondor.apana.org.au> 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 <smueller@chronox.de>
> > Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when ctx->more is zero")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  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.