From: Vito Caputo <vcaputo@pengaru.com>
To: "Rémi Denis-Courmont" <remi@remlab.net>
Cc: netdev@vger.kernel.org, courmisch@gmail.com, davem@davemloft.net,
kuba@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: kernel BUG at net/phonet/socket.c:LINE!
Date: Sun, 12 Apr 2020 22:49:14 -0700 [thread overview]
Message-ID: <20200413054914.euz7g2fcxsr74lfm@shells.gnugeneration.com> (raw)
In-Reply-To: <1806223.auBmcZeozp@basile.remlab.net>
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
next prev parent reply other threads:[~2020-04-13 5:54 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200413054914.euz7g2fcxsr74lfm@shells.gnugeneration.com \
--to=vcaputo@pengaru.com \
--cc=courmisch@gmail.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=remi@remlab.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).