diff mbox series

crypto: af_alg - Disallow multiple in-flight AIO requests

Message ID ZWWkDZRR33ypncn7@gondor.apana.org.au
State Accepted
Commit 67b164a871af1d736f131fd6fe78a610909f06f3
Headers show
Series crypto: af_alg - Disallow multiple in-flight AIO requests | expand

Commit Message

Herbert Xu Nov. 28, 2023, 8:25 a.m. UTC
Having multiple in-flight AIO requests results in unpredictable
output because they all share the same IV.  Fix this by only allowing
one request at a time.

Fixes: 83094e5e9e49 ("crypto: af_alg - add async support to algif_aead")
Fixes: a596999b7ddf ("crypto: algif - change algif_skcipher to be asynchronous")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/af_alg.c         | 14 +++++++++++++-
 include/crypto/if_alg.h |  3 +++
 2 files changed, 16 insertions(+), 1 deletion(-)

Comments

Ralph Siemsen March 15, 2024, 5:55 p.m. UTC | #1
Hi Herbert,

I have found a regression in userspace behaviour after this patch was 
merged into the 4.19.y kernel. The fix seems to involve backporting a 
few more changes. Could you review details below and confirm if this is 
the right approach?

On Tue, Nov 28, 2023 at 04:25:49PM +0800, Herbert Xu wrote:
>Having multiple in-flight AIO requests results in unpredictable
>output because they all share the same IV.  Fix this by only allowing
>one request at a time.
>
>Fixes: 83094e5e9e49 ("crypto: af_alg - add async support to algif_aead")
>Fixes: a596999b7ddf ("crypto: algif - change algif_skcipher to be asynchronous")
>Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>---
> crypto/af_alg.c         | 14 +++++++++++++-
> include/crypto/if_alg.h |  3 +++
> 2 files changed, 16 insertions(+), 1 deletion(-)

This change got backported on the 4.19 kernel in January:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.19.y&id=19af0310c8767c993f2a5d5261e4df3f9f465ce1

Since then, I am seeing a regression in a simple openssl encoding test:

openssl enc -k mysecret -aes-256-cbc -in plain.txt -out cipher.txt -engine afalg

It fails intermittently with the message "error writing to file", but 
this error is a bit misleading, the actual problem is that the kernel 
returns -16 (EBUSY) on the encoding operation.

This happens only in 4.19, and not under 5.10. The patch seems correct, 
however it seems we are missing a couple of other patches on 4.19:

f3c802a1f3001 crypto: algif_aead - Only wake up when ctx->more is zero
21dfbcd1f5cbf crypto: algif_aead - fix uninitialized ctx->init

I was able to cherry-pick those into 4.19.y, with just a minor conflict 
in one case. With those applied, the openssl command no longer fails.

I suspect similar changes would be needed also in 5.4 kernel, however I 
neither checked that, nor have I run any tests on that version.

Regards,
-Ralph
Linux regression tracking (Thorsten Leemhuis) March 20, 2024, 2:54 p.m. UTC | #2
[CCing the stable team, as it looks like two prerequisite changes for a
patch already applied are missing in at least 4.19.y]

On 15.03.24 18:55, Ralph Siemsen wrote:
> 
> I have found a regression in userspace behaviour after this patch was
> merged into the 4.19.y kernel. The fix seems to involve backporting a
> few more changes. Could you review details below and confirm if this is
> the right approach?

FWIW, developers are totally free to not care about stable and longterm
kernels series. Not sure if Herbert is among those developers, but it
might explain why there is no reply yet. That's why I CCed the stable
maintainers, strictly speaking they are responsible.

> On Tue, Nov 28, 2023 at 04:25:49PM +0800, Herbert Xu wrote:
>> Having multiple in-flight AIO requests results in unpredictable
>> output because they all share the same IV.  Fix this by only allowing
>> one request at a time.
> [...]
> This change got backported on the 4.19 kernel in January:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.19.y&id=19af0310c8767c993f2a5d5261e4df3f9f465ce1
> 
> Since then, I am seeCiao, ing a regression in a simple openssl encoding test:
> 
> openssl enc -k mysecret -aes-256-cbc -in plain.txt -out cipher.txt
> -engine afalg
> 
> It fails intermittently with the message "error writing to file", but
> this error is a bit misleading, the actual problem is that the kernel
> returns -16 (EBUSY) on the encoding operation.
> 
> This happens only in 4.19, and not under 5.10. The patch seems correct,
> however it seems we are missing a couple of other patches on 4.19:
> 
> f3c802a1f3001 crypto: algif_aead - Only wake up when ctx->more is zero
> 21dfbcd1f5cbf crypto: algif_aead - fix uninitialized ctx->init
> 
> I was able to cherry-pick those into 4.19.y, with just a minor conflict
> in one case. With those applied, the openssl command no longer fails.

Some feedback here from Herbert would of course be splendid, but maybe
your tests are all the stable team needs to pick those up for a future
4.19.y release.

> I suspect similar changes would be needed also in 5.4 kernel, however I
> neither checked that, nor have I run any tests on that version.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
diff mbox series

Patch

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index ea6fb8e89d06..68cc9290cabe 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1116,9 +1116,13 @@  EXPORT_SYMBOL_GPL(af_alg_sendmsg);
 void af_alg_free_resources(struct af_alg_async_req *areq)
 {
 	struct sock *sk = areq->sk;
+	struct af_alg_ctx *ctx;
 
 	af_alg_free_areq_sgls(areq);
 	sock_kfree_s(sk, areq, areq->areqlen);
+
+	ctx = alg_sk(sk)->private;
+	ctx->inflight = false;
 }
 EXPORT_SYMBOL_GPL(af_alg_free_resources);
 
@@ -1188,11 +1192,19 @@  EXPORT_SYMBOL_GPL(af_alg_poll);
 struct af_alg_async_req *af_alg_alloc_areq(struct sock *sk,
 					   unsigned int areqlen)
 {
-	struct af_alg_async_req *areq = sock_kmalloc(sk, areqlen, GFP_KERNEL);
+	struct af_alg_ctx *ctx = alg_sk(sk)->private;
+	struct af_alg_async_req *areq;
 
+	/* Only one AIO request can be in flight. */
+	if (ctx->inflight)
+		return ERR_PTR(-EBUSY);
+
+	areq = sock_kmalloc(sk, areqlen, GFP_KERNEL);
 	if (unlikely(!areq))
 		return ERR_PTR(-ENOMEM);
 
+	ctx->inflight = true;
+
 	areq->areqlen = areqlen;
 	areq->sk = sk;
 	areq->first_rsgl.sgl.sgt.sgl = areq->first_rsgl.sgl.sgl;
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index ef8ce86b1f78..08b803a4fcde 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -136,6 +136,7 @@  struct af_alg_async_req {
  *			recvmsg is invoked.
  * @init:		True if metadata has been sent.
  * @len:		Length of memory allocated for this data structure.
+ * @inflight:		Non-zero when AIO requests are in flight.
  */
 struct af_alg_ctx {
 	struct list_head tsgl_list;
@@ -154,6 +155,8 @@  struct af_alg_ctx {
 	bool init;
 
 	unsigned int len;
+
+	unsigned int inflight;
 };
 
 int af_alg_register_type(const struct af_alg_type *type);