Commit a5ec7baa authored by Ralf Lici's avatar Ralf Lici Committed by Antonio Quartulli
Browse files

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: 08857b5e ("ovpn: implement basic TX path (UDP)")
Signed-off-by: default avatarRalf Lici <ralf@mandelbit.com>
Reviewed-by: default avatarSabrina Dubroca <sd@queasysnail.net>
Signed-off-by: default avatarAntonio Quartulli <antonio@openvpn.net>
parent 93686c47

drivers/net/ovpn/io.c

+31 −21
Original line number Diff line number Diff line
@@ -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);