{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 <[email protected]>

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(-)

crypto/af_alg.c +2 -2

index 8612aa36a3ef..759cfa678c04 100644
@@ -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);
 	}

crypto/algif_aead.c +1 -1

index db9378a45296..d3da3b0869f5 100644
@@ -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;

crypto/algif_skcipher.c +1 -1

index c7c75ef41952..a5b4898f625a 100644
@@ -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;

include/crypto/if_alg.h +3 -2

index 38d9c5861ed8..f38227a78eae 100644
@@ -18,6 +18,7 @@
 #include <linux/if_alg.h>
 #include <linux/scatterlist.h>
 #include <linux/types.h>
+#include <linux/atomic.h>
 #include <net/sock.h>
 
 #include <crypto/aead.h>

@@ -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);
 }
 
 /**

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 <[email protected]>

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(-)

crypto/af_alg.c +2 -2

index 8612aa36a3ef..759cfa678c04 100644
@@ -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);

crypto/af_alg.c +0 -0

 	}

crypto/algif_aead.c +0 -0

index db9378a45296..d3da3b0869f5 100644

crypto/algif_skcipher.c +0 -0

 	ctx->more = 0;
 	ctx->merge = 0;
 	ctx->enc = 0;

include/crypto/if_alg.h +2 -1

index 38d9c5861ed8..f38227a78eae 100644
@@ -18,6 +18,7 @@
 #include <linux/if_alg.h>
 #include <linux/scatterlist.h>
 #include <linux/types.h>
+#include <linux/atomic.h>
 #include <net/sock.h>
 
 #include <crypto/aead.h>

@@ -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;

Am Dienstag, 19. Dezember 2017, 11:31:22 CET schrieb Jonathan Cameron:

Hi Jonathan,

Quoted 53 lines

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 <[email protected]>

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(-)

crypto/af_alg.c +2 -2

index 8612aa36a3ef..759cfa678c04 100644
@@ -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);

crypto/af_alg.c +0 -0

 	}

crypto/algif_aead.c +0 -0

index db9378a45296..d3da3b0869f5 100644

crypto/algif_aead.c static int aead_accept_parent_nokey(void *private, · line 553

Quoted 6 lines

crypto/algif_aead.c +1 -1

@@ -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.

Quoted 9 lines

crypto/algif_aead.c +0 -0

 	ctx->more = 0;
 	ctx->merge = 0;
 	ctx->enc = 0;

crypto/algif_skcipher.c +0 -0

index c7c75ef41952..a5b4898f625a 100644

crypto/algif_skcipher.c static int skcipher_accept_parent_nokey(void *private, · line 391

Quoted 5 lines

crypto/algif_skcipher.c +1 -1

@@ -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.

Quoted 25 lines

crypto/algif_skcipher.c +0 -0

 	ctx->more = 0;
 	ctx->merge = 0;
 	ctx->enc = 0;

include/crypto/if_alg.h +2 -1

index 38d9c5861ed8..f38227a78eae 100644
@@ -18,6 +18,7 @@
 #include <linux/if_alg.h>
 #include <linux/scatterlist.h>
 #include <linux/types.h>
+#include <linux/atomic.h>
 #include <net/sock.h>
 
 #include <crypto/aead.h>

@@ -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;

include/crypto/if_alg.h static inline int af_alg_rcvbuf(struct sock *sk) · line 222

Quoted 8 lines

include/crypto/if_alg.h +1 -1

@@ -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 <[email protected]>

Ciao
Stephan

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -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);

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 <[email protected]>

Fixes 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")

Quoted 6 lines

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 <[email protected]>

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.

Am Freitag, 22. Dezember 2017, 08:48:03 CET schrieb Herbert Xu:

Hi Herbert,

Quoted 8 lines · Jonathan Cameron

Quoted 6 lines

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 <[email protected]>

Fixes 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")

Quoted 3 lines

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.

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 <[email protected]>

Fixes 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")