Message ID | 1566577920-20956-1-git-send-email-loyou85@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | net: fix skb use after free in netpoll_send_skb_on_dev | expand |
From: Feng Sun <loyou85@gmail.com> Date: Sat, 24 Aug 2019 00:32:00 +0800 > After commit baeababb5b85d5c4e6c917efe2a1504179438d3b > ("tun: return NET_XMIT_DROP for dropped packets"), > when tun_net_xmit drop packets, it will free skb and return NET_XMIT_DROP, > netpoll_send_skb_on_dev will run into two use after free cases: I don't know what to do here. Really, the intention of the design is that the only valid ->ndo_start_xmit() values are those with macro names fitting the pattern NETDEV_TX_*, which means only NETDEV_TX_OK and NETDEV_TX_BUSY are valid. NET_XMIT_* values are for qdisc ->enqueue() methods. Note, particularly, that when ->ndo_start_xmit() values are propagated through ->enqueue() calls they get masked out with NET_XMIT_MASK. However, I see that most of the code doing enqueueing and invocation of ->ndo_start_xmit() use the dev_xmit_complete() helper to check this condition. So probably that is what netpoll should be using as well.
diff --git a/net/core/netpoll.c b/net/core/netpoll.c index 2cf27da..b4bffe6 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -335,7 +335,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb, HARD_TX_UNLOCK(dev, txq); - if (status == NETDEV_TX_OK) + if (status == NETDEV_TX_OK || status == NET_XMIT_DROP) break; } @@ -352,7 +352,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb, } - if (status != NETDEV_TX_OK) { + if (status != NETDEV_TX_OK && status != NET_XMIT_DROP) { skb_queue_tail(&npinfo->txq, skb); schedule_delayed_work(&npinfo->tx_work,0); }