[v2] crypto: af_alg - Work around empty control messages without MSG_MORE
diff mbox series

Message ID 20200827071436.GA30281@gondor.apana.org.au
State Accepted
Commit c195d66a8a75c60515819b101975f38b7ec6577f
Headers show
Series
  • [v2] crypto: af_alg - Work around empty control messages without MSG_MORE
Related show

Commit Message

Herbert Xu Aug. 27, 2020, 7:14 a.m. UTC
On Thu, Aug 27, 2020 at 08:40:01AM +0200, Ard Biesheuvel wrote:
>
> It is part of iwd - just build that and run 'make check'
> 
> With your patch applied, the occurrence of sendmsg() in
> operate_cipher() triggers the warn_once(), but if I add MSG_MORE
> there, the test hangs.

I see.  This is a different issue.  The original kernel change
was a bit too strict here and it is barfing at the fact that two
successive sendmsg's of the same request both contain a control
message.

Here's an updated patch to allow this.

---8<---
The iwd daemon uses libell which sets up the skcipher operation with
two separate control messages.  As the first control message is sent
without MSG_MORE, it is interpreted as an empty request.

While libell should be fixed to use MSG_MORE where appropriate, this
patch works around the bug in the kernel so that existing binaries
continue to work.

We will print a warning however.

A separate issue is that the new kernel code no longer allows the
control message to be sent twice within the same request.  This
restriction is obviously incompatible with what iwd was doing (first
setting an IV and then sending the real control message).  This
patch changes the kernel so that this is explicitly allowed.

Reported-by: Caleb Jorden <caljorden@hotmail.com>
Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when...")
Cc: <stable@vger.kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Ard Biesheuvel Aug. 27, 2020, 7:40 a.m. UTC | #1
On Thu, 27 Aug 2020 at 09:15, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Aug 27, 2020 at 08:40:01AM +0200, Ard Biesheuvel wrote:
> >
> > It is part of iwd - just build that and run 'make check'
> >
> > With your patch applied, the occurrence of sendmsg() in
> > operate_cipher() triggers the warn_once(), but if I add MSG_MORE
> > there, the test hangs.
>
> I see.  This is a different issue.  The original kernel change
> was a bit too strict here and it is barfing at the fact that two
> successive sendmsg's of the same request both contain a control
> message.
>
> Here's an updated patch to allow this.
>
> ---8<---
> The iwd daemon uses libell which sets up the skcipher operation with
> two separate control messages.  As the first control message is sent
> without MSG_MORE, it is interpreted as an empty request.
>
> While libell should be fixed to use MSG_MORE where appropriate, this
> patch works around the bug in the kernel so that existing binaries
> continue to work.
>
> We will print a warning however.
>
> A separate issue is that the new kernel code no longer allows the
> control message to be sent twice within the same request.  This
> restriction is obviously incompatible with what iwd was doing (first
> setting an IV and then sending the real control message).  This
> patch changes the kernel so that this is explicitly allowed.
>
> Reported-by: Caleb Jorden <caljorden@hotmail.com>
> Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when...")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index a6f581ab200c..8be8bec07cdd 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -16,6 +16,7 @@
>  #include <linux/module.h>
>  #include <linux/net.h>
>  #include <linux/rwsem.h>
> +#include <linux/sched.h>
>  #include <linux/sched/signal.h>
>  #include <linux/security.h>
>
> @@ -845,9 +846,15 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
>         }
>
>         lock_sock(sk);
> -       if (ctx->init && (init || !ctx->more)) {
> -               err = -EINVAL;
> -               goto unlock;
> +       if (ctx->init && !ctx->more) {
> +               if (ctx->used) {
> +                       err = -EINVAL;
> +                       goto unlock;
> +               }
> +
> +               pr_info_once(
> +                       "%s sent an empty control message without MSG_MORE.\n",
> +                       current->comm);
>         }
>         ctx->init = true;
>

Yep, that works.

Patch
diff mbox series

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index a6f581ab200c..8be8bec07cdd 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -16,6 +16,7 @@ 
 #include <linux/module.h>
 #include <linux/net.h>
 #include <linux/rwsem.h>
+#include <linux/sched.h>
 #include <linux/sched/signal.h>
 #include <linux/security.h>
 
@@ -845,9 +846,15 @@  int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
 	}
 
 	lock_sock(sk);
-	if (ctx->init && (init || !ctx->more)) {
-		err = -EINVAL;
-		goto unlock;
+	if (ctx->init && !ctx->more) {
+		if (ctx->used) {
+			err = -EINVAL;
+			goto unlock;
+		}
+
+		pr_info_once(
+			"%s sent an empty control message without MSG_MORE.\n",
+			current->comm);
 	}
 	ctx->init = true;