[PATCH] Re: PROBLEM: cryptsetup fails to unlock drive in 5.8-rc6 (regression)

[PATCH] Re: PROBLEM: cryptsetup fails to unlock drive in 5.8-rc6 (regression)

[Al Viro]

On Mon, Jul 27, 2020 at 05:05:54PM +0100, Al Viro wrote:
> On Thu, Jul 23, 2020 at 11:51:01AM -0400, Nick Bowler wrote:
> > Hi,
> > 
> > After installing Linux 5.8-rc6, it seems cryptsetup can no longer
> > open LUKS volumes.  Regardless of the entered passphrase (correct
> > or otherwise), the result is a very unhelpful "Keyslot open failed."
> > message.
> > 
> > On the kernels which fail, I also noticed that the cryptsetup
> > benchmark command appears to not be able to determine that any
> > ciphers are available (output at end of message), possibly for
> > the same reason.
> > 
> > Bisected to the following commit, which suggests a problem specific
> > to compat userspace (this is amd64 kernel).  I tested both ia32 and
> > x32 userspace to confirm the problem.  Reverting this commit on top
> > of 5.8-rc6 resolves the issue.
> > 
> > Looking at strace output the failing syscall appears to be:
> > 
> >   sendmsg(8, {msg_name=NULL, msg_namelen=0, 
> > 	     msg_iov=[{iov_base=..., iov_len=512}], msg_iovlen=1,
> > 	     msg_control=[{cmsg_len=16, cmsg_level=SOL_ALG,
> > 	     cmsg_type=0x3}, {cmsg_len=32, cmsg_level=SOL_ALG,
> > 	     cmsg_type=0x2}], msg_controllen=48, msg_flags=0}, 0)
> > 	     = -1 EINVAL (Invalid argument)
> 
> Huh?  Just in case - could you verify that on the kernel with that
> commit reverted the same sendmsg() succeeds?

Oh, fuck...  Please see if the following fixes your reproducer; the braino
is, of course, that instead of fetching ucmsg->cmsg_len into ucmlen we read
the entire thing into cmsg.  Other uses of ucmlen had been replaced with
cmsg.cmsg_len; this one was missed.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/net/compat.c b/net/compat.c
index 5e3041a2c37d..434838bef5f8 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -202,7 +202,7 @@ int cmsghdr_from_user_compat_to_kern(struct msghdr *kmsg, struct sock *sk,
 
 		/* Advance. */
 		kcmsg = (struct cmsghdr *)((char *)kcmsg + tmp);
-		ucmsg = cmsg_compat_nxthdr(kmsg, ucmsg, ucmlen);
+		ucmsg = cmsg_compat_nxthdr(kmsg, ucmsg, cmsg.cmsg_len);
 	}
 
 	/*

Re: [PATCH] Re: PROBLEM: cryptsetup fails to unlock drive in 5.8-rc6 (regression)

[Nick Bowler]

On 2020-07-27, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jul 27, 2020 at 05:05:54PM +0100, Al Viro wrote:
>> On Thu, Jul 23, 2020 at 11:51:01AM -0400, Nick Bowler wrote:
>> > After installing Linux 5.8-rc6, it seems cryptsetup can no longer
>> > open LUKS volumes.  Regardless of the entered passphrase (correct
>> > or otherwise), the result is a very unhelpful "Keyslot open failed."
>> > message.
[...]
> Oh, fuck...  Please see if the following fixes your reproducer; the braino
> is, of course, that instead of fetching ucmsg->cmsg_len into ucmlen we read
> the entire thing into cmsg.  Other uses of ucmlen had been replaced with
> cmsg.cmsg_len; this one was missed.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/net/compat.c b/net/compat.c
> index 5e3041a2c37d..434838bef5f8 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -202,7 +202,7 @@ int cmsghdr_from_user_compat_to_kern(struct msghdr
> *kmsg, struct sock *sk,
>
>  		/* Advance. */
>  		kcmsg = (struct cmsghdr *)((char *)kcmsg + tmp);
> -		ucmsg = cmsg_compat_nxthdr(kmsg, ucmsg, ucmlen);
> +		ucmsg = cmsg_compat_nxthdr(kmsg, ucmsg, cmsg.cmsg_len);
>  	}
>
>  	/*

This patch appears to resolve the problem when applied on top of 5.8-rc7.

Thanks,
  Nick

[PATCH net] fix a braino in cmsghdr_from_user_compat_to_kern()

[Al Viro]

	commit 547ce4cfb34c ("switch cmsghdr_from_user_compat_to_kern() to
copy_from_user()") missed one of the places where ucmlen should've been
replaced with cmsg.cmsg_len, now that we are fetching the entire struct
rather than doing it field-by-field.

	As the result, compat sendmsg() with several different-sized cmsg
attached started to fail with EINVAL.  Trivial to fix, fortunately.

Reported-by: Nick Bowler <nbowler@draconx.ca>
Tested-by: Nick Bowler <nbowler@draconx.ca>
Fixes: 547ce4cfb34c ("switch cmsghdr_from_user_compat_to_kern() to copy_from_user()")

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/net/compat.c b/net/compat.c
index 5e3041a2c37d..434838bef5f8 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -202,7 +202,7 @@ int cmsghdr_from_user_compat_to_kern(struct msghdr *kmsg, struct sock *sk,
 
 		/* Advance. */
 		kcmsg = (struct cmsghdr *)((char *)kcmsg + tmp);
-		ucmsg = cmsg_compat_nxthdr(kmsg, ucmsg, ucmlen);
+		ucmsg = cmsg_compat_nxthdr(kmsg, ucmsg, cmsg.cmsg_len);
 	}
 
 	/*

Re: [PATCH net] fix a braino in cmsghdr_from_user_compat_to_kern()

[David Miller]

From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 27 Jul 2020 19:22:20 +0100

> 	commit 547ce4cfb34c ("switch cmsghdr_from_user_compat_to_kern() to
> copy_from_user()") missed one of the places where ucmlen should've been
> replaced with cmsg.cmsg_len, now that we are fetching the entire struct
> rather than doing it field-by-field.
> 
> 	As the result, compat sendmsg() with several different-sized cmsg
> attached started to fail with EINVAL.  Trivial to fix, fortunately.
> 
> Reported-by: Nick Bowler <nbowler@draconx.ca>
> Tested-by: Nick Bowler <nbowler@draconx.ca>
> Fixes: 547ce4cfb34c ("switch cmsghdr_from_user_compat_to_kern() to copy_from_user()")
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Applied, thanks Al.