ovpn: fix possible use-after-free in ovpn_net_xmit

When building the skb_list in ovpn_net_xmit, skb_share_check will free
the original skb if it is shared. The current implementation continues
to use the stale skb pointer for subsequent operations:
- peer lookup,
- skb_dst_drop (even though all segments produced by skb_gso_segment
  will have a dst attached),
- ovpn_peer_stats_increment_tx.

Fix this by moving the peer lookup and skb_dst_drop before segmentation
so that the original skb is still valid when used. Return early if all
segments fail skb_share_check and the list ends up empty.
Also switch ovpn_peer_stats_increment_tx to use skb_list.next; the next
patch fixes the stats logic.

Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 3e9e7f8444b34465ec08cc1dcf6c70b657e5937d..f70c58b10599bc9f003115f89813a463c9ecf445 100644 (file)
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -365,7 +365,27 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
        /* verify IP header size in network packet */
        proto = ovpn_ip_check_protocol(skb);
        if (unlikely(!proto || skb->protocol != proto))
-               goto drop;
+               goto drop_no_peer;
+
+       /* retrieve peer serving the destination IP of this packet */
+       peer = ovpn_peer_get_by_dst(ovpn, skb);
+       if (unlikely(!peer)) {
+               switch (skb->protocol) {
+               case htons(ETH_P_IP):
+                       net_dbg_ratelimited("%s: no peer to send data to dst=%pI4\n",
+                                           netdev_name(ovpn->dev),
+                                           &ip_hdr(skb)->daddr);
+                       break;
+               case htons(ETH_P_IPV6):
+                       net_dbg_ratelimited("%s: no peer to send data to dst=%pI6c\n",
+                                           netdev_name(ovpn->dev),
+                                           &ipv6_hdr(skb)->daddr);
+                       break;
+               }
+               goto drop_no_peer;
+       }
+       /* dst was needed for peer selection - it can now be dropped */
+       skb_dst_drop(skb);
 
        if (skb_is_gso(skb)) {
                segments = skb_gso_segment(skb, 0);
@@ -396,34 +416,24 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
 
                __skb_queue_tail(&skb_list, curr);
        }
-       skb_list.prev->next = NULL;
 
-       /* retrieve peer serving the destination IP of this packet */
-       peer = ovpn_peer_get_by_dst(ovpn, skb);
-       if (unlikely(!peer)) {
-               switch (skb->protocol) {
-               case htons(ETH_P_IP):
-                       net_dbg_ratelimited("%s: no peer to send data to dst=%pI4\n",
-                                           netdev_name(ovpn->dev),
-                                           &ip_hdr(skb)->daddr);
-                       break;
-               case htons(ETH_P_IPV6):
-                       net_dbg_ratelimited("%s: no peer to send data to dst=%pI6c\n",
-                                           netdev_name(ovpn->dev),
-                                           &ipv6_hdr(skb)->daddr);
-                       break;
-               }
-               goto drop;
+       /* no segments survived: don't jump to 'drop' because we already
+        * incremented the counter for each failure in the loop
+        */
+       if (unlikely(skb_queue_empty(&skb_list))) {
+               ovpn_peer_put(peer);
+               return NETDEV_TX_OK;
        }
-       /* dst was needed for peer selection - it can now be dropped */
-       skb_dst_drop(skb);
+       skb_list.prev->next = NULL;
 
-       ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb->len);
+       ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb_list.next->len);
        ovpn_send(ovpn, skb_list.next, peer);
 
        return NETDEV_TX_OK;
 
 drop:
+       ovpn_peer_put(peer);
+drop_no_peer:
        dev_dstats_tx_dropped(ovpn->dev);
        skb_tx_error(skb);
        kfree_skb_list(skb);