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: [email protected]
------------[ 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 [email protected].
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
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: [email protected]
>
> ------------[ 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 [email protected].
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
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
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
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
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.
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: [email protected]
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
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: [email protected]
>
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