[RFC] get_compat_msghdr(): get rid of field-by-field copyin

[RFC] get_compat_msghdr(): get rid of field-by-field copyin

[Al Viro]

There are 3 commits in vfs.git#misc.compat I hadn't pushed to Linus yet;
they touch net/* and I'd like to see at least "no objections" from networking
folks before asking to pull that; all of those are about getting rid of
field-by-field copyin.  Please, review and comment.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/net/compat.c b/net/compat.c
index aba929e5250f..dba5e222a0e5 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -37,21 +37,16 @@ int get_compat_msghdr(struct msghdr *kmsg,
 		      struct sockaddr __user **save_addr,
 		      struct iovec **iov)
 {
-	compat_uptr_t uaddr, uiov, tmp3;
-	compat_size_t nr_segs;
+	struct compat_msghdr msg;
 	ssize_t err;
 
-	if (!access_ok(VERIFY_READ, umsg, sizeof(*umsg)) ||
-	    __get_user(uaddr, &umsg->msg_name) ||
-	    __get_user(kmsg->msg_namelen, &umsg->msg_namelen) ||
-	    __get_user(uiov, &umsg->msg_iov) ||
-	    __get_user(nr_segs, &umsg->msg_iovlen) ||
-	    __get_user(tmp3, &umsg->msg_control) ||
-	    __get_user(kmsg->msg_controllen, &umsg->msg_controllen) ||
-	    __get_user(kmsg->msg_flags, &umsg->msg_flags))
+	if (copy_from_user(&msg, umsg, sizeof(*umsg)))
 		return -EFAULT;
 
-	if (!uaddr)
+	kmsg->msg_flags = msg.msg_flags;
+	kmsg->msg_namelen = msg.msg_namelen;
+
+	if (!msg.msg_name)
 		kmsg->msg_namelen = 0;
 
 	if (kmsg->msg_namelen < 0)
@@ -59,14 +54,16 @@ int get_compat_msghdr(struct msghdr *kmsg,
 
 	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
 		kmsg->msg_namelen = sizeof(struct sockaddr_storage);
-	kmsg->msg_control = compat_ptr(tmp3);
+
+	kmsg->msg_control = compat_ptr(msg.msg_control);
+	kmsg->msg_controllen = msg.msg_controllen;
 
 	if (save_addr)
-		*save_addr = compat_ptr(uaddr);
+		*save_addr = compat_ptr(msg.msg_name);
 
-	if (uaddr && kmsg->msg_namelen) {
+	if (msg.msg_name && kmsg->msg_namelen) {
 		if (!save_addr) {
-			err = move_addr_to_kernel(compat_ptr(uaddr),
+			err = move_addr_to_kernel(compat_ptr(msg.msg_name),
 						  kmsg->msg_namelen,
 						  kmsg->msg_name);
 			if (err < 0)
@@ -77,13 +74,13 @@ int get_compat_msghdr(struct msghdr *kmsg,
 		kmsg->msg_namelen = 0;
 	}
 
-	if (nr_segs > UIO_MAXIOV)
+	if (msg.msg_iovlen > UIO_MAXIOV)
 		return -EMSGSIZE;
 
 	kmsg->msg_iocb = NULL;
 
 	return compat_import_iovec(save_addr ? READ : WRITE,
-				   compat_ptr(uiov), nr_segs,
+				   compat_ptr(msg.msg_iov), msg.msg_iovlen,
 				   UIO_FASTIOV, iov, &kmsg->msg_iter);
 }
 

[RFC] copy_msghdr_from_user(): get rid of field-by-field copyin

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/net/socket.c b/net/socket.c
index c2564eb25c6b..af33d929135a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1870,22 +1870,18 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
 				 struct sockaddr __user **save_addr,
 				 struct iovec **iov)
 {
-	struct sockaddr __user *uaddr;
-	struct iovec __user *uiov;
-	size_t nr_segs;
+	struct user_msghdr msg;
 	ssize_t err;
 
-	if (!access_ok(VERIFY_READ, umsg, sizeof(*umsg)) ||
-	    __get_user(uaddr, &umsg->msg_name) ||
-	    __get_user(kmsg->msg_namelen, &umsg->msg_namelen) ||
-	    __get_user(uiov, &umsg->msg_iov) ||
-	    __get_user(nr_segs, &umsg->msg_iovlen) ||
-	    __get_user(kmsg->msg_control, &umsg->msg_control) ||
-	    __get_user(kmsg->msg_controllen, &umsg->msg_controllen) ||
-	    __get_user(kmsg->msg_flags, &umsg->msg_flags))
+	if (copy_from_user(&msg, umsg, sizeof(*umsg)))
 		return -EFAULT;
 
-	if (!uaddr)
+	kmsg->msg_control = msg.msg_control;
+	kmsg->msg_controllen = msg.msg_controllen;
+	kmsg->msg_flags = msg.msg_flags;
+
+	kmsg->msg_namelen = msg.msg_namelen;
+	if (!msg.msg_name)
 		kmsg->msg_namelen = 0;
 
 	if (kmsg->msg_namelen < 0)
@@ -1895,11 +1891,11 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
 		kmsg->msg_namelen = sizeof(struct sockaddr_storage);
 
 	if (save_addr)
-		*save_addr = uaddr;
+		*save_addr = msg.msg_name;
 
-	if (uaddr && kmsg->msg_namelen) {
+	if (msg.msg_name && kmsg->msg_namelen) {
 		if (!save_addr) {
-			err = move_addr_to_kernel(uaddr, kmsg->msg_namelen,
+			err = move_addr_to_kernel(msg.msg_name, kmsg->msg_namelen,
 						  kmsg->msg_name);
 			if (err < 0)
 				return err;
@@ -1909,12 +1905,13 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
 		kmsg->msg_namelen = 0;
 	}
 
-	if (nr_segs > UIO_MAXIOV)
+	if (msg.msg_iovlen > UIO_MAXIOV)
 		return -EMSGSIZE;
 
 	kmsg->msg_iocb = NULL;
 
-	return import_iovec(save_addr ? READ : WRITE, uiov, nr_segs,
+	return import_iovec(save_addr ? READ : WRITE,
+			    msg.msg_iov, msg.msg_iovlen,
 			    UIO_FASTIOV, iov, &kmsg->msg_iter);
 }
 

[RFC] get_compat_bpf_fprog(): don't copyin field-by-field

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/net/compat.c b/net/compat.c
index dba5e222a0e5..6ded6c821d7a 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -313,15 +313,15 @@ struct sock_fprog __user *get_compat_bpf_fprog(char __user *optval)
 {
 	struct compat_sock_fprog __user *fprog32 = (struct compat_sock_fprog __user *)optval;
 	struct sock_fprog __user *kfprog = compat_alloc_user_space(sizeof(struct sock_fprog));
-	compat_uptr_t ptr;
-	u16 len;
-
-	if (!access_ok(VERIFY_READ, fprog32, sizeof(*fprog32)) ||
-	    !access_ok(VERIFY_WRITE, kfprog, sizeof(struct sock_fprog)) ||
-	    __get_user(len, &fprog32->len) ||
-	    __get_user(ptr, &fprog32->filter) ||
-	    __put_user(len, &kfprog->len) ||
-	    __put_user(compat_ptr(ptr), &kfprog->filter))
+	struct compat_sock_fprog f32;
+	struct sock_fprog f;
+
+	if (copy_from_user(&f32, fprog32, sizeof(*fprog32)))
+		return NULL;
+	memset(&f, 0, sizeof(f));
+	f.len = f32.len;
+	f.filter = compat_ptr(f32.filter);
+	if (copy_to_user(kfprog, &f, sizeof(struct sock_fprog)))
 		return NULL;
 
 	return kfprog;

Re: [RFC] get_compat_bpf_fprog(): don't copyin field-by-field

[Daniel Borkmann]

On 07/08/2017 08:22 PM, Al Viro wrote:
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

(Looks like ppp_sock_fprog_ioctl_trans() is another such candidate.)

Re: [RFC] get_compat_msghdr(): get rid of field-by-field copyin

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sat, 8 Jul 2017 19:21:00 +0100

> There are 3 commits in vfs.git#misc.compat I hadn't pushed to Linus yet;
> they touch net/* and I'd like to see at least "no objections" from networking
> folks before asking to pull that; all of those are about getting rid of
> field-by-field copyin.  Please, review and comment.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

That weird sparc64 regression concerns me.

The commit referenced in that discussion:

d9e968cb9f849770288f5fde3d8d3a5f7e339052 ("getrlimit()/setrlimit(): move compat to native")

looks harmless, or if there is a bug in there I can't see it.

But whatever it is, that same problem could be hiding in some of these
other transformations as well.

I think the bug might be that we are corrupting the user's stack
somehow.  But the two user copies in that commit look perfectly fine
to my eyes.

There shouldn't be any padding in that compat_rlimit structure, so
it's not like we're copying extra bytes.  Well, we'd be exposing
kernel stack memory if that were the case.

Color me stumped, but cautious about ACK'ing these networking
compat changes.

Re: [RFC] get_compat_msghdr(): get rid of field-by-field copyin

[Al Viro]

On Tue, Jul 11, 2017 at 08:25:14PM -0700, David Miller wrote:

> looks harmless, or if there is a bug in there I can't see it.
> 
> But whatever it is, that same problem could be hiding in some of these
> other transformations as well.
> 
> I think the bug might be that we are corrupting the user's stack
> somehow.  But the two user copies in that commit look perfectly fine
> to my eyes.
> 
> There shouldn't be any padding in that compat_rlimit structure, so
> it's not like we're copying extra bytes.  Well, we'd be exposing
> kernel stack memory if that were the case.

There isn't any padding in compat_rlimit; unfortunately, it was
mistakenly declared as struct rlimit instead.  Which, of course,
has different member sizes - otherwise we wouldn't have needed
a compat syscall there in the first place.

It was harder to spot since I combined move and a transformation
into one commit.  Shouldn't have done so...  Had those been two
separate commits, the bug would've stood out immediately.  Shouldn't
be the case here...

> Color me stumped, but cautious about ACK'ing these networking
> compat changes.

Re: [RFC] get_compat_msghdr(): get rid of field-by-field copyin

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Fri, 14 Jul 2017 02:37:50 +0100

> On Tue, Jul 11, 2017 at 08:25:14PM -0700, David Miller wrote:
> 
>> looks harmless, or if there is a bug in there I can't see it.
>> 
>> But whatever it is, that same problem could be hiding in some of these
>> other transformations as well.
>> 
>> I think the bug might be that we are corrupting the user's stack
>> somehow.  But the two user copies in that commit look perfectly fine
>> to my eyes.
>> 
>> There shouldn't be any padding in that compat_rlimit structure, so
>> it's not like we're copying extra bytes.  Well, we'd be exposing
>> kernel stack memory if that were the case.
> 
> There isn't any padding in compat_rlimit; unfortunately, it was
> mistakenly declared as struct rlimit instead.  Which, of course,
> has different member sizes - otherwise we wouldn't have needed
> a compat syscall there in the first place.
> 
> It was harder to spot since I combined move and a transformation
> into one commit.  Shouldn't have done so...  Had those been two
> separate commits, the bug would've stood out immediately.  Shouldn't
> be the case here...

Ok, I'll ack these patches then:

Acked-by: David S. Miller <davem@davemloft.net>

[git pull] vfs.git network field-by-field copyin patches

[Al Viro]

[My apologies for late pull requests - I hoped to be back to normal
connectivity by yesterday evening.  Spent the night sitting in Logan
instead...]

This part of misc.compat queue was held back for review from networking
folks and since davem has jus ACKed those...

The following changes since commit 0d0606060baefdb13d3d80dba1b4c816b0676e16:

  mqueue: move compat syscalls to native ones (2017-07-04 13:13:49 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git misc.compat

for you to fetch changes up to f8f8a727eab1c5b78c3703a461565b042979cc79:

  get_compat_bpf_fprog(): don't copyin field-by-field (2017-07-04 13:14:34 -0400)

----------------------------------------------------------------
Al Viro (3):
      copy_msghdr_from_user(): get rid of field-by-field copyin
      get_compat_msghdr(): get rid of field-by-field copyin
      get_compat_bpf_fprog(): don't copyin field-by-field

 net/compat.c | 49 +++++++++++++++++++++++--------------------------
 net/socket.c | 31 ++++++++++++++-----------------
 2 files changed, 37 insertions(+), 43 deletions(-)