* kernel BUG at net/phonet/socket.c:LINE! @ 2020-04-10 7:16 syzbot 2020-04-11 0:43 ` Vito Caputo 2021-12-19 14:58 ` [syzbot] " syzbot 0 siblings, 2 replies; 8+ messages in thread From: syzbot @ 2020-04-10 7:16 UTC (permalink / raw) To: courmisch, davem, kuba, linux-kernel, netdev, syzkaller-bugs Hello, syzbot found the following crash on: HEAD commit: b93cfb9c net: icmp6: do not select saddr from iif when rou.. git tree: net console output: https://syzkaller.appspot.com/x/log.txt?x=13501d2be00000 kernel config: https://syzkaller.appspot.com/x/.config?x=94a7f1dec460ee83 dashboard link: https://syzkaller.appspot.com/bug?extid=2dc91e7fc3dea88b1e8a compiler: gcc (GCC) 9.0.0 20181231 (experimental) Unfortunately, I don't have any reproducer for this crash yet. IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+2dc91e7fc3dea88b1e8a@syzkaller.appspotmail.com ------------[ cut here ]------------ kernel BUG at net/phonet/socket.c:213! invalid opcode: 0000 [#1] PREEMPT SMP KASAN CPU: 0 PID: 8394 Comm: syz-executor.4 Not tainted 5.6.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] RIP: 0010:pn_socket_autobind+0x13c/0x160 net/phonet/socket.c:202 Code: 44 05 00 00 00 00 00 44 89 e0 48 8b 4c 24 58 65 48 33 0c 25 28 00 00 00 75 23 48 83 c4 60 5b 5d 41 5c 41 5d c3 e8 b4 ad 41 fa <0f> 0b e8 9d 56 7f fa eb 9f e8 b6 56 7f fa e9 6d ff ff ff e8 0c dd RSP: 0018:ffffc900034cfda8 EFLAGS: 00010212 RAX: 0000000000040000 RBX: 0000000000000000 RCX: ffffc90011d66000 RDX: 0000000000000081 RSI: ffffffff873183dc RDI: 0000000000000003 RBP: 1ffff92000699fb5 R08: ffff8880620f61c0 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: dffffc0000000000 R14: 0000000000000000 R15: 0000000000000000 FS: 00007f84223c9700(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000004d8730 CR3: 000000005cce0000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: pn_socket_listen+0x40/0x200 net/phonet/socket.c:399 __sys_listen+0x17d/0x250 net/socket.c:1696 __do_sys_listen net/socket.c:1705 [inline] __se_sys_listen net/socket.c:1703 [inline] __x64_sys_listen+0x50/0x70 net/socket.c:1703 do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 entry_SYSCALL_64_after_hwframe+0x49/0xb3 RIP: 0033:0x45c889 Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f84223c8c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000032 RAX: ffffffffffffffda RBX: 00007f84223c96d4 RCX: 000000000045c889 RDX: 0000000000000000 RSI: 000000000000007d RDI: 0000000000000003 RBP: 000000000076bf00 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff R13: 0000000000000712 R14: 00000000004c9e2d R15: 000000000076bf0c Modules linked in: ---[ end trace 65d6f1331216c544 ]--- RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] RIP: 0010:pn_socket_autobind+0x13c/0x160 net/phonet/socket.c:202 Code: 44 05 00 00 00 00 00 44 89 e0 48 8b 4c 24 58 65 48 33 0c 25 28 00 00 00 75 23 48 83 c4 60 5b 5d 41 5c 41 5d c3 e8 b4 ad 41 fa <0f> 0b e8 9d 56 7f fa eb 9f e8 b6 56 7f fa e9 6d ff ff ff e8 0c dd RSP: 0018:ffffc900034cfda8 EFLAGS: 00010212 RAX: 0000000000040000 RBX: 0000000000000000 RCX: ffffc90011d66000 RDX: 0000000000000081 RSI: ffffffff873183dc RDI: 0000000000000003 RBP: 1ffff92000699fb5 R08: ffff8880620f61c0 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: dffffc0000000000 R14: 0000000000000000 R15: 0000000000000000 FS: 00007f84223c9700(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000000076c000 CR3: 000000005cce0000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kernel BUG at net/phonet/socket.c:LINE! 2020-04-10 7:16 kernel BUG at net/phonet/socket.c:LINE! syzbot @ 2020-04-11 0:43 ` Vito Caputo 2020-04-11 7:36 ` Rémi Denis-Courmont 2021-12-19 14:58 ` [syzbot] " syzbot 1 sibling, 1 reply; 8+ messages in thread From: Vito Caputo @ 2020-04-11 0:43 UTC (permalink / raw) To: netdev; +Cc: courmisch, davem, kuba, linux-kernel Hello peeps, I stared a bit at the code surrounding this report, and maybe someone more familiar with the network stack can clear something up for me real quick: > RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] net/phonet/socket.c: 202 static int pn_socket_autobind(struct socket *sock) 203 { 204 struct sockaddr_pn sa; 205 int err; 206 207 memset(&sa, 0, sizeof(sa)); 208 sa.spn_family = AF_PHONET; 209 err = pn_socket_bind(sock, (struct sockaddr *)&sa, 210 sizeof(struct sockaddr_pn)); 211 if (err != -EINVAL) 212 return err; 213 BUG_ON(!pn_port(pn_sk(sock->sk)->sobject)); 214 return 0; /* socket was already bound */ 215 } line 213 is the BUG_ON for !sock->sk->sobject, a phonet-specific member: include/net/phonet/phonet.h: 23 struct pn_sock { 24 struct sock sk; 25 u16 sobject; 26 u16 dobject; 27 u8 resource; 28 }; pn_socket_autobind() expects that to be non-null whenever pn_socket_bind() returns -EINVAL, which seems odd, but the comment claims it's already bound, let's look at pn_socket_bind(): net/phonet/socket.c: 156 static int pn_socket_bind(struct socket *sock, struct sockaddr *addr, int len) 157 { 158 struct sock *sk = sock->sk; 159 struct pn_sock *pn = pn_sk(sk); 160 struct sockaddr_pn *spn = (struct sockaddr_pn *)addr; 161 int err; 162 u16 handle; 163 u8 saddr; 164 165 if (sk->sk_prot->bind) 166 return sk->sk_prot->bind(sk, addr, len); 167 168 if (len < sizeof(struct sockaddr_pn)) 169 return -EINVAL; 170 if (spn->spn_family != AF_PHONET) 171 return -EAFNOSUPPORT; 172 173 handle = pn_sockaddr_get_object((struct sockaddr_pn *)addr); 174 saddr = pn_addr(handle); 175 if (saddr && phonet_address_lookup(sock_net(sk), saddr)) 176 return -EADDRNOTAVAIL; 177 178 lock_sock(sk); 179 if (sk->sk_state != TCP_CLOSE || pn_port(pn->sobject)) { 180 err = -EINVAL; /* attempt to rebind */ 181 goto out; 182 } 183 WARN_ON(sk_hashed(sk)); 184 mutex_lock(&port_mutex); 185 err = sk->sk_prot->get_port(sk, pn_port(handle)); 186 if (err) 187 goto out_port; 188 189 /* get_port() sets the port, bind() sets the address if applicable */ 190 pn->sobject = pn_object(saddr, pn_port(pn->sobject)); 191 pn->resource = spn->spn_resource; 192 193 /* Enable RX on the socket */ 194 err = sk->sk_prot->hash(sk); 195 out_port: 196 mutex_unlock(&port_mutex); 197 out: 198 release_sock(sk); 199 return err; 200 } The first return branch in there simply hands off the bind to and indirection sk->sk_prot->bind() if present. This smells ripe for breaking the assumptions of that BUG_ON(). I'm assuming there's no point for such an indirection if not to enable a potentially non-phonet-ops hook, otherwise we'd just be do the bind. If not, isn't this plain recursive? Color me confused. Will other bind() implementations return -EINVAL when already bound with sobject set? no. Furthermore, -EINVAL is also returned when len < sizeof(struct sockaddr_pn), maybe the rebind case should return -EADDRINUSE instead of -EINVAL? I must be missing some things. Regards, Vito Caputo On Fri, Apr 10, 2020 at 12:16:17AM -0700, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: b93cfb9c net: icmp6: do not select saddr from iif when rou.. > git tree: net > console output: https://syzkaller.appspot.com/x/log.txt?x=13501d2be00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=94a7f1dec460ee83 > dashboard link: https://syzkaller.appspot.com/bug?extid=2dc91e7fc3dea88b1e8a > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+2dc91e7fc3dea88b1e8a@syzkaller.appspotmail.com > > ------------[ cut here ]------------ > kernel BUG at net/phonet/socket.c:213! > invalid opcode: 0000 [#1] PREEMPT SMP KASAN > CPU: 0 PID: 8394 Comm: syz-executor.4 Not tainted 5.6.0-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] > RIP: 0010:pn_socket_autobind+0x13c/0x160 net/phonet/socket.c:202 > Code: 44 05 00 00 00 00 00 44 89 e0 48 8b 4c 24 58 65 48 33 0c 25 28 00 00 00 75 23 48 83 c4 60 5b 5d 41 5c 41 5d c3 e8 b4 ad 41 fa <0f> 0b e8 9d 56 7f fa eb 9f e8 b6 56 7f fa e9 6d ff ff ff e8 0c dd > RSP: 0018:ffffc900034cfda8 EFLAGS: 00010212 > RAX: 0000000000040000 RBX: 0000000000000000 RCX: ffffc90011d66000 > RDX: 0000000000000081 RSI: ffffffff873183dc RDI: 0000000000000003 > RBP: 1ffff92000699fb5 R08: ffff8880620f61c0 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > R13: dffffc0000000000 R14: 0000000000000000 R15: 0000000000000000 > FS: 00007f84223c9700(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000004d8730 CR3: 000000005cce0000 CR4: 00000000001406f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > pn_socket_listen+0x40/0x200 net/phonet/socket.c:399 > __sys_listen+0x17d/0x250 net/socket.c:1696 > __do_sys_listen net/socket.c:1705 [inline] > __se_sys_listen net/socket.c:1703 [inline] > __x64_sys_listen+0x50/0x70 net/socket.c:1703 > do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 > entry_SYSCALL_64_after_hwframe+0x49/0xb3 > RIP: 0033:0x45c889 > Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:00007f84223c8c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000032 > RAX: ffffffffffffffda RBX: 00007f84223c96d4 RCX: 000000000045c889 > RDX: 0000000000000000 RSI: 000000000000007d RDI: 0000000000000003 > RBP: 000000000076bf00 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff > R13: 0000000000000712 R14: 00000000004c9e2d R15: 000000000076bf0c > Modules linked in: > ---[ end trace 65d6f1331216c544 ]--- > RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] > RIP: 0010:pn_socket_autobind+0x13c/0x160 net/phonet/socket.c:202 > Code: 44 05 00 00 00 00 00 44 89 e0 48 8b 4c 24 58 65 48 33 0c 25 28 00 00 00 75 23 48 83 c4 60 5b 5d 41 5c 41 5d c3 e8 b4 ad 41 fa <0f> 0b e8 9d 56 7f fa eb 9f e8 b6 56 7f fa e9 6d ff ff ff e8 0c dd > RSP: 0018:ffffc900034cfda8 EFLAGS: 00010212 > RAX: 0000000000040000 RBX: 0000000000000000 RCX: ffffc90011d66000 > RDX: 0000000000000081 RSI: ffffffff873183dc RDI: 0000000000000003 > RBP: 1ffff92000699fb5 R08: ffff8880620f61c0 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > R13: dffffc0000000000 R14: 0000000000000000 R15: 0000000000000000 > FS: 00007f84223c9700(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 000000000076c000 CR3: 000000005cce0000 CR4: 00000000001406f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > > --- > This bug is generated by a bot. It may contain errors. > See https://goo.gl/tpsmEJ for more information about syzbot. > syzbot engineers can be reached at syzkaller@googlegroups.com. > > syzbot will keep track of this bug report. See: > https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kernel BUG at net/phonet/socket.c:LINE! 2020-04-11 0:43 ` Vito Caputo @ 2020-04-11 7:36 ` Rémi Denis-Courmont 2020-04-13 5:49 ` Vito Caputo 0 siblings, 1 reply; 8+ messages in thread From: Rémi Denis-Courmont @ 2020-04-11 7:36 UTC (permalink / raw) To: Vito Caputo; +Cc: netdev, courmisch, davem, kuba, linux-kernel Hi, Le lauantaina 11. huhtikuuta 2020, 3.43.20 EEST Vito Caputo a écrit : > I stared a bit at the code surrounding this report, and maybe someone more > familiar with the network stack can clear something up for me real quick: > > RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] > > net/phonet/socket.c: > 202 static int pn_socket_autobind(struct socket *sock) > 203 { > 204 struct sockaddr_pn sa; > 205 int err; > 206 > 207 memset(&sa, 0, sizeof(sa)); > 208 sa.spn_family = AF_PHONET; > 209 err = pn_socket_bind(sock, (struct sockaddr *)&sa, > 210 sizeof(struct sockaddr_pn)); > 211 if (err != -EINVAL) > 212 return err; > 213 BUG_ON(!pn_port(pn_sk(sock->sk)->sobject)); > 214 return 0; /* socket was already bound */ > 215 } > > line 213 is the BUG_ON for !sock->sk->sobject, a phonet-specific member: > > include/net/phonet/phonet.h: > 23 struct pn_sock { > 24 struct sock sk; > 25 u16 sobject; > 26 u16 dobject; > 27 u8 resource; > 28 }; > > pn_socket_autobind() expects that to be non-null whenever pn_socket_bind() > returns -EINVAL, which seems odd, but the comment claims it's already bound, > let's look at pn_socket_bind(): > > net/phonet/socket.c: > 156 static int pn_socket_bind(struct socket *sock, struct sockaddr *addr, > int len) 157 { > 158 struct sock *sk = sock->sk; > 159 struct pn_sock *pn = pn_sk(sk); > 160 struct sockaddr_pn *spn = (struct sockaddr_pn *)addr; > 161 int err; > 162 u16 handle; > 163 u8 saddr; > 164 > 165 if (sk->sk_prot->bind) > 166 return sk->sk_prot->bind(sk, addr, len); > 167 > 168 if (len < sizeof(struct sockaddr_pn)) > 169 return -EINVAL; > 170 if (spn->spn_family != AF_PHONET) > 171 return -EAFNOSUPPORT; > 172 > 173 handle = pn_sockaddr_get_object((struct sockaddr_pn *)addr); > 174 saddr = pn_addr(handle); > 175 if (saddr && phonet_address_lookup(sock_net(sk), saddr)) > 176 return -EADDRNOTAVAIL; > 177 > 178 lock_sock(sk); > 179 if (sk->sk_state != TCP_CLOSE || pn_port(pn->sobject)) { > 180 err = -EINVAL; /* attempt to rebind */ > 181 goto out; > 182 } > 183 WARN_ON(sk_hashed(sk)); > 184 mutex_lock(&port_mutex); > 185 err = sk->sk_prot->get_port(sk, pn_port(handle)); > 186 if (err) > 187 goto out_port; > 188 > 189 /* get_port() sets the port, bind() sets the address if > applicable */ 190 pn->sobject = pn_object(saddr, > pn_port(pn->sobject)); > 191 pn->resource = spn->spn_resource; > 192 > 193 /* Enable RX on the socket */ > 194 err = sk->sk_prot->hash(sk); > 195 out_port: > 196 mutex_unlock(&port_mutex); > 197 out: > 198 release_sock(sk); > 199 return err; > 200 } > > > The first return branch in there simply hands off the bind to and > indirection sk->sk_prot->bind() if present. This smells ripe for breaking > the assumptions of that BUG_ON(). I believe that this is in line with the design of the socket stack within the Linux kernel. 'struct proto_ops' carries the protocol family operations, then 'struct proto' carries the protocol operations. Admittedly, Phonet only had one datagram and one stream protocol ever written, as the hardware development ceased. So in practice, there is a 1:1 mapping between the two, and sk_prot.bind is always NULL. > I'm assuming there's no point for such an indirection if not to enable a > potentially non-phonet-ops hook, otherwise we'd just be do the bind. In my understanding, that's *not* what sk_prot is for, no. It's rather meant to specialize the socket calls on a per-protocol basis. For instance, UDP and UDP-Lite share their address family 'struct proto_ops' (either inet_dgram_ops or inet6_dgram_ops) but they have different 'struct proto'. > If not, isn't this plain recursive? Color me confused. Will other bind() > implementations return -EINVAL when already bound with sobject set? no. As far as I can find, there are no cases where the bind pointer would not be NULL in upstream kernel. This can only happen if an out-of-tree module defines its own protocol within the Phonet family. > Furthermore, -EINVAL is also returned when len < sizeof(struct sockaddr_pn), > maybe the rebind case should return -EADDRINUSE instead of -EINVAL? bind() semantics require returning EINVAL if the socket address size is, well, invalid. If we are to distinguish the two error scenarii, then it's the rebind case that needs a different error, but EINVAL is consistent with INET. > I must be missing some things. -- Rémi Denis-Courmont Tapiolan uusi kaupunki, Uudenmaan tasavalta ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kernel BUG at net/phonet/socket.c:LINE! 2020-04-11 7:36 ` Rémi Denis-Courmont @ 2020-04-13 5:49 ` Vito Caputo 2020-04-13 6:01 ` Rémi Denis-Courmont 0 siblings, 1 reply; 8+ messages in thread From: Vito Caputo @ 2020-04-13 5:49 UTC (permalink / raw) To: Rémi Denis-Courmont; +Cc: netdev, courmisch, davem, kuba, linux-kernel On Sat, Apr 11, 2020 at 10:36:59AM +0300, Rémi Denis-Courmont wrote: > Hi, > > Le lauantaina 11. huhtikuuta 2020, 3.43.20 EEST Vito Caputo a écrit : > > I stared a bit at the code surrounding this report, and maybe someone more > > familiar with the network stack can clear something up for me real quick: > > > RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] > > > > net/phonet/socket.c: > > 202 static int pn_socket_autobind(struct socket *sock) > > 203 { > > 204 struct sockaddr_pn sa; > > 205 int err; > > 206 > > 207 memset(&sa, 0, sizeof(sa)); > > 208 sa.spn_family = AF_PHONET; > > 209 err = pn_socket_bind(sock, (struct sockaddr *)&sa, > > 210 sizeof(struct sockaddr_pn)); > > 211 if (err != -EINVAL) > > 212 return err; > > 213 BUG_ON(!pn_port(pn_sk(sock->sk)->sobject)); > > 214 return 0; /* socket was already bound */ > > 215 } > > > > line 213 is the BUG_ON for !sock->sk->sobject, a phonet-specific member: > > > > include/net/phonet/phonet.h: > > 23 struct pn_sock { > > 24 struct sock sk; > > 25 u16 sobject; > > 26 u16 dobject; > > 27 u8 resource; > > 28 }; > > > > pn_socket_autobind() expects that to be non-null whenever pn_socket_bind() > > returns -EINVAL, which seems odd, but the comment claims it's already bound, > > let's look at pn_socket_bind(): > > > > net/phonet/socket.c: > > 156 static int pn_socket_bind(struct socket *sock, struct sockaddr *addr, > > int len) 157 { > > 158 struct sock *sk = sock->sk; > > 159 struct pn_sock *pn = pn_sk(sk); > > 160 struct sockaddr_pn *spn = (struct sockaddr_pn *)addr; > > 161 int err; > > 162 u16 handle; > > 163 u8 saddr; > > 164 > > 165 if (sk->sk_prot->bind) > > 166 return sk->sk_prot->bind(sk, addr, len); > > 167 > > 168 if (len < sizeof(struct sockaddr_pn)) > > 169 return -EINVAL; > > 170 if (spn->spn_family != AF_PHONET) > > 171 return -EAFNOSUPPORT; > > 172 > > 173 handle = pn_sockaddr_get_object((struct sockaddr_pn *)addr); > > 174 saddr = pn_addr(handle); > > 175 if (saddr && phonet_address_lookup(sock_net(sk), saddr)) > > 176 return -EADDRNOTAVAIL; > > 177 > > 178 lock_sock(sk); > > 179 if (sk->sk_state != TCP_CLOSE || pn_port(pn->sobject)) { > > 180 err = -EINVAL; /* attempt to rebind */ > > 181 goto out; > > 182 } > > 183 WARN_ON(sk_hashed(sk)); > > 184 mutex_lock(&port_mutex); > > 185 err = sk->sk_prot->get_port(sk, pn_port(handle)); > > 186 if (err) > > 187 goto out_port; > > 188 > > 189 /* get_port() sets the port, bind() sets the address if > > applicable */ 190 pn->sobject = pn_object(saddr, > > pn_port(pn->sobject)); > > 191 pn->resource = spn->spn_resource; > > 192 > > 193 /* Enable RX on the socket */ > > 194 err = sk->sk_prot->hash(sk); > > 195 out_port: > > 196 mutex_unlock(&port_mutex); > > 197 out: > > 198 release_sock(sk); > > 199 return err; > > 200 } > > > > > > The first return branch in there simply hands off the bind to and > > indirection sk->sk_prot->bind() if present. This smells ripe for breaking > > the assumptions of that BUG_ON(). > > I believe that this is in line with the design of the socket stack within the > Linux kernel. 'struct proto_ops' carries the protocol family operations, then > 'struct proto' carries the protocol operations. > > Admittedly, Phonet only had one datagram and one stream protocol ever written, > as the hardware development ceased. So in practice, there is a 1:1 mapping > between the two, and sk_prot.bind is always NULL. > > > I'm assuming there's no point for such an indirection if not to enable a > > potentially non-phonet-ops hook, otherwise we'd just be do the bind. > > In my understanding, that's *not* what sk_prot is for, no. It's rather meant > to specialize the socket calls on a per-protocol basis. > > For instance, UDP and UDP-Lite share their address family 'struct proto_ops' > (either inet_dgram_ops or inet6_dgram_ops) but they have different > 'struct proto'. > > > If not, isn't this plain recursive? Color me confused. Will other bind() > > implementations return -EINVAL when already bound with sobject set? no. > > As far as I can find, there are no cases where the bind pointer would not be > NULL in upstream kernel. This can only happen if an out-of-tree module defines > its own protocol within the Phonet family. > Ok, so we can disregard that bit as benign apparently. > > Furthermore, -EINVAL is also returned when len < sizeof(struct sockaddr_pn), > > maybe the rebind case should return -EADDRINUSE instead of -EINVAL? > > bind() semantics require returning EINVAL if the socket address size is, well, > invalid. > > If we are to distinguish the two error scenarii, then it's the rebind case > that needs a different error, but EINVAL is consistent with INET. > Isn't the existing code is bugged if treating -EINVAL as valid and a rebind? The invalid size will return a NULL sobject but -EINVAL, triggering the BUG_ON. Thanks, Vito Caputo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kernel BUG at net/phonet/socket.c:LINE! 2020-04-13 5:49 ` Vito Caputo @ 2020-04-13 6:01 ` Rémi Denis-Courmont 2020-04-13 6:05 ` Vito Caputo 0 siblings, 1 reply; 8+ messages in thread From: Rémi Denis-Courmont @ 2020-04-13 6:01 UTC (permalink / raw) To: Vito Caputo; +Cc: netdev, courmisch, davem, kuba, linux-kernel Le maanantaina 13. huhtikuuta 2020, 8.49.14 EEST Vito Caputo a écrit : > > If we are to distinguish the two error scenarii, then it's the rebind > > case > > that needs a different error, but EINVAL is consistent with INET. > > Isn't the existing code is bugged if treating -EINVAL as valid and a rebind? > > The invalid size will return a NULL sobject but -EINVAL, triggering the > BUG_ON. How do you pass an invalid size? It's a constant `sizeof(struct sockaddr_pn)` in that code path. -- Rémi Denis-Courmont Tapiolan uusi kaupunki, Uudenmaan tasavalta ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kernel BUG at net/phonet/socket.c:LINE! 2020-04-13 6:01 ` Rémi Denis-Courmont @ 2020-04-13 6:05 ` Vito Caputo 0 siblings, 0 replies; 8+ messages in thread From: Vito Caputo @ 2020-04-13 6:05 UTC (permalink / raw) To: Rémi Denis-Courmont; +Cc: netdev, courmisch, davem, kuba, linux-kernel On Mon, Apr 13, 2020 at 09:01:58AM +0300, Rémi Denis-Courmont wrote: > Le maanantaina 13. huhtikuuta 2020, 8.49.14 EEST Vito Caputo a écrit : > > > If we are to distinguish the two error scenarii, then it's the rebind > > > case > > > that needs a different error, but EINVAL is consistent with INET. > > > > Isn't the existing code is bugged if treating -EINVAL as valid and a rebind? > > > > The invalid size will return a NULL sobject but -EINVAL, triggering the > > BUG_ON. > > How do you pass an invalid size? It's a constant `sizeof(struct sockaddr_pn)` > in that code path. > duh, sorry for the noise, I should have re-read the code before replying. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] kernel BUG at net/phonet/socket.c:LINE! 2020-04-10 7:16 kernel BUG at net/phonet/socket.c:LINE! syzbot 2020-04-11 0:43 ` Vito Caputo @ 2021-12-19 14:58 ` syzbot 2021-12-19 21:09 ` Pavel Skripkin 1 sibling, 1 reply; 8+ messages in thread From: syzbot @ 2021-12-19 14:58 UTC (permalink / raw) To: courmisch, davem, kuba, linux-kernel, netdev, syzkaller-bugs syzbot has found a reproducer for the following issue on: HEAD commit: 60ec7fcfe768 qlcnic: potential dereference null pointer of.. git tree: net console output: https://syzkaller.appspot.com/x/log.txt?x=11b3505db00000 kernel config: https://syzkaller.appspot.com/x/.config?x=fa556098924b78f0 dashboard link: https://syzkaller.appspot.com/bug?extid=2dc91e7fc3dea88b1e8a compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 syz repro: https://syzkaller.appspot.com/x/repro.syz?x=168791cdb00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14a0cbcdb00000 IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+2dc91e7fc3dea88b1e8a@syzkaller.appspotmail.com netlink: 'syz-executor185': attribute type 2 has an invalid length. ------------[ cut here ]------------ kernel BUG at net/phonet/socket.c:213! invalid opcode: 0000 [#1] PREEMPT SMP KASAN CPU: 0 PID: 3620 Comm: syz-executor185 Not tainted 5.16.0-rc5-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] net/phonet/socket.c:202 RIP: 0010:pn_socket_autobind+0x13c/0x160 net/phonet/socket.c:202 net/phonet/socket.c:202 Code: 44 05 00 00 00 00 00 48 8b 44 24 58 65 48 2b 04 25 28 00 00 00 75 26 48 83 c4 60 44 89 e0 5b 5d 41 5c 41 5d c3 e8 64 60 1e f9 <0f> 0b e8 2d 25 65 f9 eb 9f e8 46 25 65 f9 e9 6d ff ff ff e8 8c 2b RSP: 0018:ffffc90001bafc40 EFLAGS: 00010293 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 RDX: ffff88801c748000 RSI: ffffffff8859516c RDI: 0000000000000003 RBP: 1ffff92000375f88 R08: 00000000ffffffea R09: 0000000000000000 R10: ffffffff8859512c R11: 0000000000000000 R12: 0000000000000000 R13: dffffc0000000000 R14: 0000000000000000 R15: ffff88801c748000 FS: 0000555556878300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000740 CR3: 0000000018a78000 CR4: 00000000003506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> pn_socket_connect+0xfc/0x970 net/phonet/socket.c:227 net/phonet/socket.c:227 __sys_connect_file+0x155/0x1a0 net/socket.c:1896 net/socket.c:1896 __sys_connect+0x161/0x190 net/socket.c:1913 net/socket.c:1913 __do_sys_connect net/socket.c:1923 [inline] __se_sys_connect net/socket.c:1920 [inline] __do_sys_connect net/socket.c:1923 [inline] net/socket.c:1920 __se_sys_connect net/socket.c:1920 [inline] net/socket.c:1920 __x64_sys_connect+0x6f/0xb0 net/socket.c:1920 net/socket.c:1920 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_x64 arch/x86/entry/common.c:50 [inline] arch/x86/entry/common.c:80 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7f9bf1080159 Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 41 15 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007fff1adca428 EFLAGS: 00000246 ORIG_RAX: 000000000000002a RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f9bf1080159 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000005 RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000013355 R13: 00007fff1adca490 R14: 00007fff1adca480 R15: 00007fff1adca44c </TASK> Modules linked in: ---[ end trace 16a4e3e11e1ba5b9 ]--- RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline] net/phonet/socket.c:202 RIP: 0010:pn_socket_autobind+0x13c/0x160 net/phonet/socket.c:202 net/phonet/socket.c:202 Code: 44 05 00 00 00 00 00 48 8b 44 24 58 65 48 2b 04 25 28 00 00 00 75 26 48 83 c4 60 44 89 e0 5b 5d 41 5c 41 5d c3 e8 64 60 1e f9 <0f> 0b e8 2d 25 65 f9 eb 9f e8 46 25 65 f9 e9 6d ff ff ff e8 8c 2b RSP: 0018:ffffc90001bafc40 EFLAGS: 00010293 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 RDX: ffff88801c748000 RSI: ffffffff8859516c RDI: 0000000000000003 RBP: 1ffff92000375f88 R08: 00000000ffffffea R09: 0000000000000000 R10: ffffffff8859512c R11: 0000000000000000 R12: 0000000000000000 R13: dffffc0000000000 R14: 0000000000000000 R15: ffff88801c748000 FS: 0000555556878300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000740 CR3: 0000000018a78000 CR4: 00000000003506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] kernel BUG at net/phonet/socket.c:LINE! 2021-12-19 14:58 ` [syzbot] " syzbot @ 2021-12-19 21:09 ` Pavel Skripkin 0 siblings, 0 replies; 8+ messages in thread From: Pavel Skripkin @ 2021-12-19 21:09 UTC (permalink / raw) To: syzbot, courmisch, davem, kuba, linux-kernel, netdev, syzkaller-bugs On 12/19/21 17:58, syzbot wrote: > syzbot has found a reproducer for the following issue on: > > HEAD commit: 60ec7fcfe768 qlcnic: potential dereference null pointer of.. > git tree: net > console output: https://syzkaller.appspot.com/x/log.txt?x=11b3505db00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=fa556098924b78f0 > dashboard link: https://syzkaller.appspot.com/bug?extid=2dc91e7fc3dea88b1e8a > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=168791cdb00000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14a0cbcdb00000 > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+2dc91e7fc3dea88b1e8a@syzkaller.appspotmail.com > This bug can be triggered via simple sk = socket(AF_PHONET) ioctl(sk, SIOCPNENABLEPIPE, 0) connect(sk); ioctl() sets sk->sk_state to TCP_SYN_SENT in pep_sock_enable() and then there is following check in pn_socket_bind(): if (sk->sk_state != TCP_CLOSE || pn_port(pn->sobject)) { err = -EINVAL; /* attempt to rebind */ goto out; } Looks like "sk->sk_state != TCP_CLOSE" check is redundant and pn_port(pn->sobject) is unique flag, that socket is already binded. With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-12-19 21:10 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-10 7:16 kernel BUG at net/phonet/socket.c:LINE! syzbot 2020-04-11 0:43 ` Vito Caputo 2020-04-11 7:36 ` Rémi Denis-Courmont 2020-04-13 5:49 ` Vito Caputo 2020-04-13 6:01 ` Rémi Denis-Courmont 2020-04-13 6:05 ` Vito Caputo 2021-12-19 14:58 ` [syzbot] " syzbot 2021-12-19 21:09 ` Pavel Skripkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).