| Message ID | 20200724045040.20070-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] qrtr: orphan skb before queuing in xmit | expand |
On 7/23/20 9:50 PM, Cong Wang wrote: > Similar to tun_net_xmit(), we have to orphan the skb > before queuing it, otherwise we may use the socket when > purging the queue after it is freed by user-space. Which socket ? By not calling skb_orphan(skb), this skb should own a reference on skb->sk preventing skb->sk to disappear. It seems that instead of skb_orphan() here, we could avoid calling skb_set_owner_w() in the first place, because this is confusing. Also, there seems to be no limit on number of skbs that can be queued, this looks rather dangerous. diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c index 0cb4adfc6641da0b1add7a8b40957fd23df1e075..9b180768dbfe9ea100ad008d92522e340f73101e 100644 --- a/net/qrtr/qrtr.c +++ b/net/qrtr/qrtr.c @@ -661,7 +661,6 @@ static void qrtr_port_remove(struct qrtr_sock *ipc) pkt->client.node = cpu_to_le32(ipc->us.sq_node); pkt->client.port = cpu_to_le32(ipc->us.sq_port); - skb_set_owner_w(skb, &ipc->sk); qrtr_bcast_enqueue(NULL, skb, QRTR_TYPE_DEL_CLIENT, &ipc->us, &to); } @@ -855,7 +854,6 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb, skbn = skb_clone(skb, GFP_KERNEL); if (!skbn) break; - skb_set_owner_w(skbn, skb->sk); qrtr_node_enqueue(node, skbn, type, from, to); } mutex_unlock(&qrtr_node_lock); > > Reported-and-tested-by: syzbot+6720d64f31c081c2f708@syzkaller.appspotmail.com > Fixes: 28fb4e59a47d ("net: qrtr: Expose tunneling endpoint to user space") > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/qrtr/tun.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c > index 15ce9b642b25..54a565dcfef3 100644 > --- a/net/qrtr/tun.c > +++ b/net/qrtr/tun.c > @@ -20,6 +20,7 @@ static int qrtr_tun_send(struct qrtr_endpoint *ep, struct sk_buff *skb) > { > struct qrtr_tun *tun = container_of(ep, struct qrtr_tun, ep); > > + skb_orphan(skb); > skb_queue_tail(&tun->queue, skb); > > /* wake up any blocking processes, waiting for new data */ >
On Thu, Jul 23, 2020 at 10:35 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 7/23/20 9:50 PM, Cong Wang wrote: > > Similar to tun_net_xmit(), we have to orphan the skb > > before queuing it, otherwise we may use the socket when > > purging the queue after it is freed by user-space. > > Which socket ? sk->sk_wq points to &sock->wq. The socket is of course from qrtr_create(). > > By not calling skb_orphan(skb), this skb should own a reference on skb->sk preventing > skb->sk to disappear. > I said socket, not sock. I believe the socket can be gone while the sock is still there. > It seems that instead of skb_orphan() here, we could avoid calling skb_set_owner_w() in the first place, > because this is confusing. Not sure about this, at least tun calls skb_set_owner_w() too. More importantly, sock_alloc_send_skb() calls it too. :)
On Thu, Jul 23, 2020 at 11:00 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > I said socket, not sock. I believe the socket can be gone while the sock is > still there. Hmm, looks llike sock_orphan() should be called...
On 7/23/20 11:00 PM, Cong Wang wrote: > On Thu, Jul 23, 2020 at 10:35 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: >> >> >> >> On 7/23/20 9:50 PM, Cong Wang wrote: >>> Similar to tun_net_xmit(), we have to orphan the skb >>> before queuing it, otherwise we may use the socket when >>> purging the queue after it is freed by user-space. >> >> Which socket ? > > sk->sk_wq points to &sock->wq. The socket is of course from > qrtr_create(). > >> >> By not calling skb_orphan(skb), this skb should own a reference on skb->sk preventing >> skb->sk to disappear. >> > > I said socket, not sock. I believe the socket can be gone while the sock is > still there. > > >> It seems that instead of skb_orphan() here, we could avoid calling skb_set_owner_w() in the first place, >> because this is confusing. > > Not sure about this, at least tun calls skb_set_owner_w() too. More > importantly, sock_alloc_send_skb() calls it too. :) > tun is very different : skbs reaching it came come from all over the places, like TCP stack. Their skb->sk is not pointing to the tun sock. Here, the skbs are cooked from net/qrtr/qrtr.c locations only.
diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c index 15ce9b642b25..54a565dcfef3 100644 --- a/net/qrtr/tun.c +++ b/net/qrtr/tun.c @@ -20,6 +20,7 @@ static int qrtr_tun_send(struct qrtr_endpoint *ep, struct sk_buff *skb) { struct qrtr_tun *tun = container_of(ep, struct qrtr_tun, ep); + skb_orphan(skb); skb_queue_tail(&tun->queue, skb); /* wake up any blocking processes, waiting for new data */
Similar to tun_net_xmit(), we have to orphan the skb before queuing it, otherwise we may use the socket when purging the queue after it is freed by user-space. Reported-and-tested-by: syzbot+6720d64f31c081c2f708@syzkaller.appspotmail.com Fixes: 28fb4e59a47d ("net: qrtr: Expose tunneling endpoint to user space") Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/qrtr/tun.c | 1 + 1 file changed, 1 insertion(+)