[v1] net: phonet: do not BUG_ON() in pn_socket_autobind() on failed bind

[PATCH] net: phonet: do not BUG_ON() in pn_socket_autobind() on failed bind

Posted by Morduan Zang 1 month, 3 weeks ago

syzbot reported a kernel BUG triggered from pn_socket_sendmsg() via
pn_socket_autobind():

  kernel BUG at net/phonet/socket.c:213!
  RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline]
  RIP: 0010:pn_socket_sendmsg+0x240/0x250 net/phonet/socket.c:421
  Call Trace:
   sock_sendmsg_nosec+0x112/0x150 net/socket.c:797
   __sock_sendmsg net/socket.c:812 [inline]
   __sys_sendto+0x402/0x590 net/socket.c:2280
   ...

pn_socket_autobind() calls pn_socket_bind() with port 0 and, on
-EINVAL, assumes the socket was already bound and asserts that the
port is non-zero:

  err = pn_socket_bind(sock, ..., sizeof(struct sockaddr_pn));
  if (err != -EINVAL)
          return err;
  BUG_ON(!pn_port(pn_sk(sock->sk)->sobject));
  return 0; /* socket was already bound */

However pn_socket_bind() also returns -EINVAL when sk->sk_state is not
TCP_CLOSE, even when the socket has never been bound and pn_port() is
still 0.  In that case the BUG_ON() fires and panics the kernel from a
user-triggerable path.

Treat the "bind returned -EINVAL but pn_port() is still 0" case as a
regular error and propagate -EINVAL to the caller instead of crashing.
Existing callers already translate a non-zero return from
pn_socket_autobind() into -ENOBUFS/-EAGAIN, so returning -EINVAL here
only changes behaviour from panic to a normal errno.

Fixes: ba113a94b750 ("Phonet: common socket glue")
Reported-by: syzbot+706f5eb79044e686c794@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=706f5eb79044e686c794
Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
Signed-off-by: zhanjun <zhanjun@uniontech.com>
---
 net/phonet/socket.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 4423d483c630..de9108adfe1c 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -210,7 +210,15 @@ static int pn_socket_autobind(struct socket *sock)
 			     sizeof(struct sockaddr_pn));
 	if (err != -EINVAL)
 		return err;
-	BUG_ON(!pn_port(pn_sk(sock->sk)->sobject));
+	/*
+	 * pn_socket_bind() can return -EINVAL both when the socket is
+	 * already bound (pn_port() != 0) and when sk_state != TCP_CLOSE
+	 * without a prior bind.  Only the former is an "already bound"
+	 * success for autobind; otherwise propagate -EINVAL instead of
+	 * crashing the kernel.
+	 */
+	if (!pn_port(pn_sk(sock->sk)->sobject))
+		return -EINVAL;
 	return 0; /* socket was already bound */
 }
 
-- 
2.50.1

Re: [PATCH] net: phonet: do not BUG_ON() in pn_socket_autobind() on failed bind

Posted by Deepanshu Kartikey 1 month, 3 weeks ago

Hi Morduan,

Thanks for fixing this syzbot report!

I independently worked on the same bug and sent an
alternative patch here:

  https://lore.kernel.org/all/20260422021533.16987-1-kartikey406@gmail.com/

The key difference is that my approach checks the bound
state BEFORE calling pn_socket_bind(), rather than after:

--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -207,12 +207,11 @@ static int pn_socket_autobind(struct socket *sock)
 {
 	struct sockaddr_pn sa;
 	int err;

+	if (pn_port(pn_sk(sock->sk)->sobject))
+		return 0; /* socket was already bound */
+
 	memset(&sa, 0, sizeof(sa));
 	sa.spn_family = AF_PHONET;
 	err = pn_socket_bind(sock, (struct sockaddr_unsized *)&sa,
 			     sizeof(struct sockaddr_pn));
-	if (err != -EINVAL)
-		return err;
-	BUG_ON(!pn_port(pn_sk(sock->sk)->sobject));
-	return 0; /* socket was already bound */
+	return err;
 }

The root cause is that pn_socket_bind() returns -EINVAL
for multiple reasons:

  1. address length too short
  2. sk_state != TCP_CLOSE (without prior bind)
  3. socket already bound  <- only intended case

Your fix correctly prevents the crash. However the
ambiguous "if (err != -EINVAL)" path still remains.

By checking pn_port(sobject) BEFORE calling
pn_socket_bind(), this approach:

  - eliminates the -EINVAL ambiguity entirely
  - removes the special -EINVAL handling path
  - makes "already bound" check direct and clear
  - simplifies the overall logic flow

Both fixes prevent the crash, but this removes the
underlying ambiguity rather than working around it.

Thoughts? Happy to defer to your patch if maintainers
prefer the minimal change approach.

Thanks,
Deepanshu Kartikey

[PATCH net v2] net: phonet: do not BUG_ON() in pn_socket_autobind() on failed bind

Posted by Morduan Zang 1 month, 3 weeks ago

syzbot reported a kernel BUG triggered from pn_socket_sendmsg() via
pn_socket_autobind():

  kernel BUG at net/phonet/socket.c:213!
  RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline]
  RIP: 0010:pn_socket_sendmsg+0x240/0x250 net/phonet/socket.c:421
  Call Trace:
   sock_sendmsg_nosec+0x112/0x150 net/socket.c:797
   __sock_sendmsg net/socket.c:812 [inline]
   __sys_sendto+0x402/0x590 net/socket.c:2280
   ...

pn_socket_autobind() calls pn_socket_bind() with port 0 and, on
-EINVAL, assumes the socket was already bound and asserts that the
port is non-zero:

  err = pn_socket_bind(sock, ..., sizeof(struct sockaddr_pn));
  if (err != -EINVAL)
          return err;
  BUG_ON(!pn_port(pn_sk(sock->sk)->sobject));
  return 0; /* socket was already bound */

However pn_socket_bind() also returns -EINVAL when sk->sk_state is not
TCP_CLOSE, even when the socket has never been bound and pn_port() is
still 0.  In that case the BUG_ON() fires and panics the kernel from a
user-triggerable path.

Treat the "bind returned -EINVAL but pn_port() is still 0" case as a
regular error and propagate -EINVAL to the caller instead of crashing.
Existing callers already translate a non-zero return from
pn_socket_autobind() into -ENOBUFS/-EAGAIN, so returning -EINVAL here
only changes behaviour from panic to a normal errno.

Fixes: ba113a94b750 ("Phonet: common socket glue")
Reported-by: syzbot+706f5eb79044e686c794@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=706f5eb79044e686c794
Suggested-by: Remi Denis-Courmont <courmisch@gmail.com>
Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
Signed-off-by: zhanjun <zhanjun@uniontech.com>
---
Changes in v2:
 - Fold the extra port check into the existing -EINVAL test so that
   autobind now reads as a single compact "not already bound" guard
   using unlikely() (Remi Denis-Courmont).
 - Reword the accompanying comment accordingly; no functional change
   vs. v1 other than the code-style simplification.

v1: https://lore.kernel.org/all/81A6570B633FF6FE+20260422013807.63087-1-zhangdandan@uniontech.com/
---
 net/phonet/socket.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 4423d483c630..bbd710d95b97 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -208,9 +208,15 @@ static int pn_socket_autobind(struct socket *sock)
 	sa.spn_family = AF_PHONET;
 	err = pn_socket_bind(sock, (struct sockaddr_unsized *)&sa,
 			     sizeof(struct sockaddr_pn));
-	if (err != -EINVAL)
+	/*
+	 * pn_socket_bind() also returns -EINVAL when sk_state != TCP_CLOSE
+	 * without a prior bind, so -EINVAL alone is not sufficient to infer
+	 * that the socket was already bound.  Only treat it as "already
+	 * bound" when the port is non-zero; otherwise propagate the error
+	 * instead of crashing the kernel.
+	 */
+	if (err != -EINVAL || unlikely(!pn_port(pn_sk(sock->sk)->sobject)))
 		return err;
-	BUG_ON(!pn_port(pn_sk(sock->sk)->sobject));
 	return 0; /* socket was already bound */
 }
 
-- 
2.50.1

Re: [PATCH net v2] net: phonet: do not BUG_ON() in pn_socket_autobind() on failed bind

Posted by Rémi Denis-Courmont 1 month, 3 weeks ago


Le 23 avril 2026 04:05:57 GMT+03:00, Morduan Zang <zhangdandan@uniontech.com> a écrit :
>syzbot reported a kernel BUG triggered from pn_socket_sendmsg() via
>pn_socket_autobind():
>
>  kernel BUG at net/phonet/socket.c:213!
>  RIP: 0010:pn_socket_autobind net/phonet/socket.c:213 [inline]
>  RIP: 0010:pn_socket_sendmsg+0x240/0x250 net/phonet/socket.c:421
>  Call Trace:
>   sock_sendmsg_nosec+0x112/0x150 net/socket.c:797
>   __sock_sendmsg net/socket.c:812 [inline]
>   __sys_sendto+0x402/0x590 net/socket.c:2280
>   ...
>
>pn_socket_autobind() calls pn_socket_bind() with port 0 and, on
>-EINVAL, assumes the socket was already bound and asserts that the
>port is non-zero:
>
>  err = pn_socket_bind(sock, ..., sizeof(struct sockaddr_pn));
>  if (err != -EINVAL)
>          return err;
>  BUG_ON(!pn_port(pn_sk(sock->sk)->sobject));
>  return 0; /* socket was already bound */
>
>However pn_socket_bind() also returns -EINVAL when sk->sk_state is not
>TCP_CLOSE, even when the socket has never been bound and pn_port() is
>still 0.  In that case the BUG_ON() fires and panics the kernel from a
>user-triggerable path.
>
>Treat the "bind returned -EINVAL but pn_port() is still 0" case as a
>regular error and propagate -EINVAL to the caller instead of crashing.
>Existing callers already translate a non-zero return from
>pn_socket_autobind() into -ENOBUFS/-EAGAIN, so returning -EINVAL here
>only changes behaviour from panic to a normal errno.
>
>Fixes: ba113a94b750 ("Phonet: common socket glue")
>Reported-by: syzbot+706f5eb79044e686c794@syzkaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=706f5eb79044e686c794
>Suggested-by: Remi Denis-Courmont <courmisch@gmail.com>
>Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
>Signed-off-by: zhanjun <zhanjun@uniontech.com>

Acked-by: Rémi Denis-Courmont <remi@remlab.net>