get_compat_msghdr(): get rid of field-by-field copyin
Subsystems (3)
RFC 1/1 get_compat_msghdr(): get rid of field-by-field copyin
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.
---
net/compat.c +14 -17
@@ -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 1/1 copy_msghdr_from_user(): get rid of field-by-field copyin
---
net/socket.c +14 -17
@@ -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 1/1 get_compat_bpf_fprog(): don't copyin field-by-field
In reply to Al Viro---
net/compat.c +9 -9
@@ -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
Quoted 1 line · Al Viro
Signed-off-by Al Viro <[email protected]>
(Looks like ppp_sock_fprog_ioctl_trans() is another such candidate.)
David Miller
maintainerFrom: Al Viro <[email protected]>
Date: Sat, 8 Jul 2017 19:21:00 +0100
Quoted 6 lines
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 <[email protected]>
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.
Quoted 5 lines · David Miller
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.
Quoted 7 lines
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_rlimitstructure, 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...
Quoted 2 lines
Color me stumped, but cautious about ACK'ing these networking compat changes.
From: Al Viro <[email protected]>
Date: Fri, 14 Jul 2017 02:37:50 +0100
Quoted 20 lines · David Miller
Quoted 6 lines
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.
Quoted 7 lines
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_rlimitstructure, 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.
Quoted 4 lines
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:
[git pull] vfs.git network field-by-field copyin patches
[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(-)