[PATCH] 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

[Morduan Zang]

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

[Deepanshu Kartikey]

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