| Message ID | 20220622150220.1091182-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1228b34c8d0ecf6de18c4c95d22f60cc8607c50a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: clear msg_get_inq in __sys_recvfrom() and __copy_msghdr_from_user() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 6 this patch: 6 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | warning | WARNING: Possible repeated word: 'Google' |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 6/22/22 9:02 AM, Eric Dumazet wrote: > syzbot reported uninit-value in tcp_recvmsg() [1] > > Issue here is that msg->msg_get_inq should have been cleared, > otherwise tcp_recvmsg() might read garbage and perform > more work than needed, or have undefined behavior. > > Given CONFIG_INIT_STACK_ALL_ZERO=y is probably going to be > the default soon, I chose to change __sys_recvfrom() to clear > all fields but msghdr.addr which might be not NULL. > > For __copy_msghdr_from_user(), I added an explicit clear > of kmsg->msg_get_inq. > > [1] > BUG: KMSAN: uninit-value in tcp_recvmsg+0x6cf/0xb60 net/ipv4/tcp.c:2557 > tcp_recvmsg+0x6cf/0xb60 net/ipv4/tcp.c:2557 > inet_recvmsg+0x13a/0x5a0 net/ipv4/af_inet.c:850 > sock_recvmsg_nosec net/socket.c:995 [inline] > sock_recvmsg net/socket.c:1013 [inline] > __sys_recvfrom+0x696/0x900 net/socket.c:2176 > __do_sys_recvfrom net/socket.c:2194 [inline] > __se_sys_recvfrom net/socket.c:2190 [inline] > __x64_sys_recvfrom+0x122/0x1c0 net/socket.c:2190 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > Local variable msg created at: > __sys_recvfrom+0x81/0x900 net/socket.c:2154 > __do_sys_recvfrom net/socket.c:2194 [inline] > __se_sys_recvfrom net/socket.c:2190 [inline] > __x64_sys_recvfrom+0x122/0x1c0 net/socket.c:2190 > > CPU: 0 PID: 3493 Comm: syz-executor170 Not tainted 5.19.0-rc3-syzkaller-30868-g4b28366af7d9 #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Thanks Eric, looks good to me: Reviewed-by: Jens Axboe <axboe@kernel.dk>
On Wed, Jun 22, 2022 at 5:24 PM Jens Axboe <axboe@kernel.dk> wrote: > > On 6/22/22 9:02 AM, Eric Dumazet wrote: > > syzbot reported uninit-value in tcp_recvmsg() [1] > > > > Issue here is that msg->msg_get_inq should have been cleared, > > otherwise tcp_recvmsg() might read garbage and perform > > more work than needed, or have undefined behavior. > > > > Given CONFIG_INIT_STACK_ALL_ZERO=y is probably going to be > > the default soon, I chose to change __sys_recvfrom() to clear > > all fields but msghdr.addr which might be not NULL. > > > > For __copy_msghdr_from_user(), I added an explicit clear > > of kmsg->msg_get_inq. > > > > [1] > > BUG: KMSAN: uninit-value in tcp_recvmsg+0x6cf/0xb60 net/ipv4/tcp.c:2557 > > tcp_recvmsg+0x6cf/0xb60 net/ipv4/tcp.c:2557 > > inet_recvmsg+0x13a/0x5a0 net/ipv4/af_inet.c:850 > > sock_recvmsg_nosec net/socket.c:995 [inline] > > sock_recvmsg net/socket.c:1013 [inline] > > __sys_recvfrom+0x696/0x900 net/socket.c:2176 > > __do_sys_recvfrom net/socket.c:2194 [inline] > > __se_sys_recvfrom net/socket.c:2190 [inline] > > __x64_sys_recvfrom+0x122/0x1c0 net/socket.c:2190 > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80 > > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > > > Local variable msg created at: > > __sys_recvfrom+0x81/0x900 net/socket.c:2154 > > __do_sys_recvfrom net/socket.c:2194 [inline] > > __se_sys_recvfrom net/socket.c:2190 [inline] > > __x64_sys_recvfrom+0x122/0x1c0 net/socket.c:2190 > > > > CPU: 0 PID: 3493 Comm: syz-executor170 Not tainted 5.19.0-rc3-syzkaller-30868-g4b28366af7d9 #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > > Thanks Eric, looks good to me: > > Reviewed-by: Jens Axboe <axboe@kernel.dk> > > -- > Jens Axboe > Alexander tested the patch as well: Tested-by: Alexander Potapenko<glider@google.com> Thanks !
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 22 Jun 2022 15:02:20 +0000 you wrote: > syzbot reported uninit-value in tcp_recvmsg() [1] > > Issue here is that msg->msg_get_inq should have been cleared, > otherwise tcp_recvmsg() might read garbage and perform > more work than needed, or have undefined behavior. > > Given CONFIG_INIT_STACK_ALL_ZERO=y is probably going to be > the default soon, I chose to change __sys_recvfrom() to clear > all fields but msghdr.addr which might be not NULL. > > [...] Here is the summary with links: - [net] net: clear msg_get_inq in __sys_recvfrom() and __copy_msghdr_from_user() https://git.kernel.org/netdev/net/c/1228b34c8d0e You are awesome, thank you!
diff --git a/net/socket.c b/net/socket.c index 2bc8773d9dc5e60595b14c2289e48d4ff0241da0..96300cdc06251fd654bce6277c54677f686079f8 100644 --- a/net/socket.c +++ b/net/socket.c @@ -2149,10 +2149,13 @@ SYSCALL_DEFINE4(send, int, fd, void __user *, buff, size_t, len, int __sys_recvfrom(int fd, void __user *ubuf, size_t size, unsigned int flags, struct sockaddr __user *addr, int __user *addr_len) { + struct sockaddr_storage address; + struct msghdr msg = { + /* Save some cycles and don't copy the address if not needed */ + .msg_name = addr ? (struct sockaddr *)&address : NULL, + }; struct socket *sock; struct iovec iov; - struct msghdr msg; - struct sockaddr_storage address; int err, err2; int fput_needed; @@ -2163,14 +2166,6 @@ int __sys_recvfrom(int fd, void __user *ubuf, size_t size, unsigned int flags, if (!sock) goto out; - msg.msg_control = NULL; - msg.msg_controllen = 0; - /* Save some cycles and don't copy the address if not needed */ - msg.msg_name = addr ? (struct sockaddr *)&address : NULL; - /* We assume all kernel code knows the size of sockaddr_storage */ - msg.msg_namelen = 0; - msg.msg_iocb = NULL; - msg.msg_flags = 0; if (sock->file->f_flags & O_NONBLOCK) flags |= MSG_DONTWAIT; err = sock_recvmsg(sock, &msg, flags); @@ -2375,6 +2370,7 @@ int __copy_msghdr_from_user(struct msghdr *kmsg, return -EFAULT; kmsg->msg_control_is_user = true; + kmsg->msg_get_inq = 0; kmsg->msg_control_user = msg.msg_control; kmsg->msg_controllen = msg.msg_controllen; kmsg->msg_flags = msg.msg_flags;
syzbot reported uninit-value in tcp_recvmsg() [1] Issue here is that msg->msg_get_inq should have been cleared, otherwise tcp_recvmsg() might read garbage and perform more work than needed, or have undefined behavior. Given CONFIG_INIT_STACK_ALL_ZERO=y is probably going to be the default soon, I chose to change __sys_recvfrom() to clear all fields but msghdr.addr which might be not NULL. For __copy_msghdr_from_user(), I added an explicit clear of kmsg->msg_get_inq. [1] BUG: KMSAN: uninit-value in tcp_recvmsg+0x6cf/0xb60 net/ipv4/tcp.c:2557 tcp_recvmsg+0x6cf/0xb60 net/ipv4/tcp.c:2557 inet_recvmsg+0x13a/0x5a0 net/ipv4/af_inet.c:850 sock_recvmsg_nosec net/socket.c:995 [inline] sock_recvmsg net/socket.c:1013 [inline] __sys_recvfrom+0x696/0x900 net/socket.c:2176 __do_sys_recvfrom net/socket.c:2194 [inline] __se_sys_recvfrom net/socket.c:2190 [inline] __x64_sys_recvfrom+0x122/0x1c0 net/socket.c:2190 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Local variable msg created at: __sys_recvfrom+0x81/0x900 net/socket.c:2154 __do_sys_recvfrom net/socket.c:2194 [inline] __se_sys_recvfrom net/socket.c:2190 [inline] __x64_sys_recvfrom+0x122/0x1c0 net/socket.c:2190 CPU: 0 PID: 3493 Comm: syz-executor170 Not tainted 5.19.0-rc3-syzkaller-30868-g4b28366af7d9 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Fixes: f94fd25cb0aa ("tcp: pass back data left in socket after receive") Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Jens Axboe <axboe@kernel.dk> --- net/socket.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)