kernel BUG at net/phonet/socket.c:LINE!

kernel BUG at net/phonet/socket.c:LINE!

[syzbot]

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.

Re: kernel BUG at net/phonet/socket.c:LINE!

[Vito Caputo]

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.

Re: kernel BUG at net/phonet/socket.c:LINE!

[Rémi Denis-Courmont]

	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

Re: kernel BUG at net/phonet/socket.c:LINE!

[Vito Caputo]

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

Re: kernel BUG at net/phonet/socket.c:LINE!

[Rémi Denis-Courmont]

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

Re: kernel BUG at net/phonet/socket.c:LINE!

[Vito Caputo]

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.

Re: [syzbot] kernel BUG at net/phonet/socket.c:LINE!

[syzbot]

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

Re: [syzbot] kernel BUG at net/phonet/socket.c:LINE!

[Pavel Skripkin]

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