| Message ID | 20170708182100.GR10672@ZenIV.linux.org.uk |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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.
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>
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); }
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> ---