Re: [Plinth PATCH]{topost} crypto: Fix race around ctx->rcvused by making it atomic_t
Sorry idiot moment. Sent that out with our internal markings.
Resending shortly.
On Tue, 19 Dec 2017 10:27:24 +
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")
> ---
> 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);
> 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);
> 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);
> }
>
> /**
[Plinth PATCH]{topost} crypto: Fix race around ctx->rcvused by making it atomic_t
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")
---
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);
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);
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);
}
/**
--
2.11.0
