Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t
On Tue, Dec 19, 2017 at 10:31:22AM +, Jonathan Cameron wrote:
> This variable was increased and decreased without any protection.
> Result was an occasional misscount and negative wrap around resulting
> in false resource allocation failures.
>
> Signed-off-by: Jonathan Cameron
> Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
Patch applied. Thanks.
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t
On Fri, Dec 22, 2017 at 08:50:01AM +0100, Stephan Mueller wrote:
> Am Freitag, 22. Dezember 2017, 08:48:03 CET schrieb Herbert Xu:
>
> Hi Herbert,
>
> > On Tue, Dec 19, 2017 at 10:31:22AM +, Jonathan Cameron wrote:
> > > This variable was increased and decreased without any protection.
> > > Result was an occasional misscount and negative wrap around resulting
> > > in false resource allocation failures.
> > >
> > > Signed-off-by: Jonathan Cameron
> > > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> >
> > Actually I think it used to be fine because we held the socket
> > lock in the async callback. It only got broken when we removed
> > the socket lock from the callback.
>
> But we cannot hold the lock in the async callback since it may be called in
> interrupt context.
I know. I'm just saying that the bug was introduced in the commit
that removed the socket lock rather than the commit that introduced
this originally.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t
Am Freitag, 22. Dezember 2017, 08:48:03 CET schrieb Herbert Xu:
Hi Herbert,
> On Tue, Dec 19, 2017 at 10:31:22AM +, Jonathan Cameron wrote:
> > This variable was increased and decreased without any protection.
> > Result was an occasional misscount and negative wrap around resulting
> > in false resource allocation failures.
> >
> > Signed-off-by: Jonathan Cameron
> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
>
> Actually I think it used to be fine because we held the socket
> lock in the async callback. It only got broken when we removed
> the socket lock from the callback.
But we cannot hold the lock in the async callback since it may be called in
interrupt context.
Ciao
Stephan
Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t
On Tue, Dec 19, 2017 at 10:31:22AM +, Jonathan Cameron wrote:
> This variable was increased and decreased without any protection.
> Result was an occasional misscount and negative wrap around resulting
> in false resource allocation failures.
>
> Signed-off-by: Jonathan Cameron
> Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
Actually I think it used to be fine because we held the socket
lock in the async callback. It only got broken when we removed
the socket lock from the callback.
commit 7d2c3f54e6f646887d019faa45f35d6fe9fe82ce
Author: Stephan Mueller
Date: Fri Nov 10 13:20:55 2017 +0100
crypto: af_alg - remove locking in async callback
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t
Am Dienstag, 19. Dezember 2017, 12:26:42 CET schrieb Jonathan Cameron: Hi Jonathan, > > > + atomic_set(&ctx->rcvused, 0); > > > > I think ATOMIC_INIT(0) is more suitable here. > > It's ugly to use it to assign a dynamic element like this. > > ctx->rcvused = (atomic_t)ATOMIC_INIT(0); You are right, it is dynamic and not static assignment. Please disregard my comment. Ciao Stephan
Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t
On Tue, 19 Dec 2017 12:11:19 +0100
Stephan Mueller wrote:
> Am Dienstag, 19. Dezember 2017, 11:31:22 CET schrieb Jonathan Cameron:
>
> Hi Jonathan,
>
> > This variable was increased and decreased without any protection.
> > Result was an occasional misscount and negative wrap around resulting
> > in false resource allocation failures.
> >
> > Signed-off-by: Jonathan Cameron
> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> > ---
> > Resending due to incompetence on my part - sorry all!
> >
> > The fixes tag is probably not ideal as I'm not 100% sure when this actually
> > became a bug. The code in question was introduced in
> >
> > Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management")
> > and related.
> > rcvused moved in
> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> > So I have gone with the latter option as that is where it will cleanly
> > apply. However, it probably doesn't matter as both are present only in the
> > 4.14 final kernel.
> >
> > crypto/af_alg.c | 4 ++--
> > crypto/algif_aead.c | 2 +-
> > crypto/algif_skcipher.c | 2 +-
> > include/crypto/if_alg.h | 5 +++--
> > 4 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> > index 8612aa36a3ef..759cfa678c04 100644
> > --- a/crypto/af_alg.c
> > +++ b/crypto/af_alg.c
> > @@ -664,7 +664,7 @@ void af_alg_free_areq_sgls(struct af_alg_async_req
> > *areq) unsigned int i;
> >
> > list_for_each_entry_safe(rsgl, tmp, &areq->rsgl_list, list) {
> > - ctx->rcvused -= rsgl->sg_num_bytes;
> > + atomic_sub(rsgl->sg_num_bytes, &ctx->rcvused);
> > af_alg_free_sg(&rsgl->sgl);
> > list_del(&rsgl->list);
> > if (rsgl != &areq->first_rsgl)
> > @@ -1173,7 +1173,7 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr
> > *msg, int flags,
> >
> > areq->last_rsgl = rsgl;
> > len += err;
> > - ctx->rcvused += err;
> > + atomic_add(err, &ctx->rcvused);
> > rsgl->sg_num_bytes = err;
> > iov_iter_advance(&msg->msg_iter, err);
> > }
> > diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> > index db9378a45296..d3da3b0869f5 100644
> > --- a/crypto/algif_aead.c
> > +++ b/crypto/algif_aead.c
> > @@ -550,7 +550,7 @@ static int aead_accept_parent_nokey(void *private,
> > struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list);
> > ctx->len = len;
> > ctx->used = 0;
> > - ctx->rcvused = 0;
> > + atomic_set(&ctx->rcvused, 0);
>
> I think ATOMIC_INIT(0) is more suitable here.
It's ugly to use it to assign a dynamic element like this.
ctx->rcvused = (atomic_t)ATOMIC_INIT(0);
Need the cast to avoid getting
error: expected expression before '{' token
#define ATOMIC_INIT(i) { (i) }
There are only two drivers in the kernel doing this vs
a lot doing initialization using the atomic_set option.
I'm happy to change it though if you would prefer.
>
> > ctx->more = 0;
> > ctx->merge = 0;
> > ctx->enc = 0;
> > diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> > index c7c75ef41952..a5b4898f625a 100644
> > --- a/crypto/algif_skcipher.c
> > +++ b/crypto/algif_skcipher.c
> > @@ -388,7 +388,7 @@ static int skcipher_accept_parent_nokey(void *private,
> > struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list);
> > ctx->len = len;
> > ctx->used = 0;
> > - ctx->rcvused = 0;
> > + atomic_set(&ctx->rcvused, 0);
>
> dto.
>
> > ctx->more = 0;
> > ctx->merge = 0;
> > ctx->enc = 0;
> > diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> > index 38d9c5861ed8..f38227a78eae 100644
> > --- a/include/crypto/if_alg.h
> > +++ b/include/crypto/if_alg.h
> > @@ -18,6 +18,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include
> > @@ -150,7 +151,7 @@ struct af_alg_ctx {
> > struct crypto_wait wait;
> >
> > size_t used;
> > - size_t rcvused;
> > + atomic_t rcvused;
> >
> > bool more;
> > bool merge;
> > @@ -215,7 +216,7 @@ static inline int af_alg_rcvbuf(struct sock *sk)
> > struct af_alg_ctx *ctx = ask->private;
> >
> > return max_t(int, max_t(int, sk->sk_rcvbuf & PAGE_MASK, PAGE_SIZE) -
> > - ctx->rcvused, 0);
> > +atomic_read(&ctx->rcvused), 0);
> > }
> >
> > /**
>
> Other than the comments above:
>
> Reviewed-by: Stephan Mueller
>
>
> Ciao
> Stephan
Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t
Am Dienstag, 19. Dezember 2017, 11:31:22 CET schrieb Jonathan Cameron:
Hi Jonathan,
> This variable was increased and decreased without any protection.
> Result was an occasional misscount and negative wrap around resulting
> in false resource allocation failures.
>
> Signed-off-by: Jonathan Cameron
> Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> ---
> Resending due to incompetence on my part - sorry all!
>
> The fixes tag is probably not ideal as I'm not 100% sure when this actually
> became a bug. The code in question was introduced in
>
> Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management")
> and related.
> rcvused moved in
> Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> So I have gone with the latter option as that is where it will cleanly
> apply. However, it probably doesn't matter as both are present only in the
> 4.14 final kernel.
>
> crypto/af_alg.c | 4 ++--
> crypto/algif_aead.c | 2 +-
> crypto/algif_skcipher.c | 2 +-
> include/crypto/if_alg.h | 5 +++--
> 4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 8612aa36a3ef..759cfa678c04 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -664,7 +664,7 @@ void af_alg_free_areq_sgls(struct af_alg_async_req
> *areq) unsigned int i;
>
> list_for_each_entry_safe(rsgl, tmp, &areq->rsgl_list, list) {
> - ctx->rcvused -= rsgl->sg_num_bytes;
> + atomic_sub(rsgl->sg_num_bytes, &ctx->rcvused);
> af_alg_free_sg(&rsgl->sgl);
> list_del(&rsgl->list);
> if (rsgl != &areq->first_rsgl)
> @@ -1173,7 +1173,7 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr
> *msg, int flags,
>
> areq->last_rsgl = rsgl;
> len += err;
> - ctx->rcvused += err;
> + atomic_add(err, &ctx->rcvused);
> rsgl->sg_num_bytes = err;
> iov_iter_advance(&msg->msg_iter, err);
> }
> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index db9378a45296..d3da3b0869f5 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -550,7 +550,7 @@ static int aead_accept_parent_nokey(void *private,
> struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list);
> ctx->len = len;
> ctx->used = 0;
> - ctx->rcvused = 0;
> + atomic_set(&ctx->rcvused, 0);
I think ATOMIC_INIT(0) is more suitable here.
> ctx->more = 0;
> ctx->merge = 0;
> ctx->enc = 0;
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index c7c75ef41952..a5b4898f625a 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -388,7 +388,7 @@ static int skcipher_accept_parent_nokey(void *private,
> struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list);
> ctx->len = len;
> ctx->used = 0;
> - ctx->rcvused = 0;
> + atomic_set(&ctx->rcvused, 0);
dto.
> ctx->more = 0;
> ctx->merge = 0;
> ctx->enc = 0;
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index 38d9c5861ed8..f38227a78eae 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -150,7 +151,7 @@ struct af_alg_ctx {
> struct crypto_wait wait;
>
> size_t used;
> - size_t rcvused;
> + atomic_t rcvused;
>
> bool more;
> bool merge;
> @@ -215,7 +216,7 @@ static inline int af_alg_rcvbuf(struct sock *sk)
> struct af_alg_ctx *ctx = ask->private;
>
> return max_t(int, max_t(int, sk->sk_rcvbuf & PAGE_MASK, PAGE_SIZE) -
> - ctx->rcvused, 0);
> + atomic_read(&ctx->rcvused), 0);
> }
>
> /**
Other than the comments above:
Reviewed-by: Stephan Mueller
Ciao
Stephan
