Messages in this thread | | | Subject | Re: [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control | From | Eric Dumazet <> | Date | Wed, 13 May 2020 09:18:36 -0700 |
| |
On 5/13/20 9:09 AM, Christoph Hellwig wrote: > On Wed, May 13, 2020 at 08:41:57AM -0700, Eric Dumazet wrote: >>> + * recv* side when msg_control_is_user is set, msg_control is the kernel >>> + * buffer used for all other cases. >>> + */ >>> + union { >>> + void *msg_control; >>> + void __user *msg_control_user; >>> + }; >>> + bool msg_control_is_user : 1; >> >> Adding a field in this structure seems dangerous. >> >> Some users of 'struct msghdr ' define their own struct on the stack, >> and are unaware of this new mandatory field. >> >> This bit contains garbage, crashes are likely to happen ? >> >> Look at IPV6_2292PKTOPTIONS for example. > > I though of that, an that is why the field is structured as-is. The idea > is that the field only matters if: > > (1) we are in the recvmsg and friends path, and > (2) msg_control is non-zero > > I went through the places that initialize msg_control to find any spot > that would need an annotation. The IPV6_2292PKTOPTIONS sockopt doesn't > need one as it is using the msghdr in sendmsg-like context. > > That being said while I did the audit I'd appreciate another look from > people that know the networking code better than me of course. >
Please try the following syzbot repro, since it crashes after your patch.
// autogenerated by syzkaller (https://github.com/google/syzkaller)
#define _GNU_SOURCE
#include <endian.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/syscall.h> #include <sys/types.h> #include <unistd.h>
uint64_t r[1] = {0xffffffffffffffff};
int main(void) { syscall(__NR_mmap, 0x1ffff000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul); syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 7ul, 0x32ul, -1, 0ul); syscall(__NR_mmap, 0x21000000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul); intptr_t res = 0;
// socket(AF_INET6, SOCK_STREAM, IPPROTO_IP) = 3 res = syscall(__NR_socket, 0xaul, 1ul, 0); if (res != -1) r[0] = res;
*(uint32_t*)0x20000080 = 7; // setsockopt(3, SOL_IPV6, IPV6_2292HOPLIMIT, [7], 4) = 0 syscall(__NR_setsockopt, r[0], 0x29, 8, 0x20000080ul, 4ul);
*(uint32_t*)0x20000040 = 0x18ff8; // getsockopt(3, SOL_IPV6, IPV6_2292PKTOPTIONS, "\24\0\0\0\0\0\0\0)\0\0\0\10\0\0\0\1\0\0\0\0\0\0\0", [102392->24]) = 0 syscall(__NR_getsockopt, r[0], 0x29, 6, 0x20004040ul, 0x20000040ul);
return 0; }
| |