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

2017-07-13 Thread David Miller 
From: Al Viro 
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

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

2017-07-13 Thread 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

2017-07-11 Thread David Miller 
From: Al Viro 
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 

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.

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

2017-07-08 Thread 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 
---
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);
 }