From: Antonio Quartulli <[email protected]>
The TCP error paths in ovpn_tcp_rcv() and ovpn_tcp_send_sock() take a
peer reference and then schedule the deferred-delete work:
ovpn_peer_hold(peer);
schedule_work(&peer->tcp.defer_del_work);
ovpn_tcp_peer_del_work() drops exactly one reference per run, but
schedule_work() returns false and does not re-queue when the work is
already pending. The reference, however, was taken unconditionally, so
every hold+schedule that lands on an already-pending work leaks one peer
reference.
ovpn_tcp_rcv() is the strparser receive callback and has no guard against
this: a TCP segment packed with packets whose length header is valid for
the stream parser but whose payload is smaller than the opcode size
passes ovpn_tcp_parse() and hits the error path. strparser delivers all
complete messages in a loop, so many error invocations run before the
scheduled work executes, leaking one reference each. A remote peer can
exploit this to pin the peer (and the netdev reference it holds) forever,
preventing interface teardown - a denial of service.
Take the reference only when schedule_work() actually queues the work.
schedule_work() flips the work pending bit atomically, so exactly one
caller - even across the concurrent RX and TX paths - observes the
idle->pending transition and acquires the single reference that the lone
ovpn_peer_put() in the worker balances. ovpn_peer_del() is idempotent
(ovpn_peer_remove() bails on an already-unhashed peer), so a work item
re-queued while running stays refcount-balanced.
Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Signed-off-by: Antonio Quartulli <[email protected]>
---
drivers/net/ovpn/tcp.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 433bd07a4f1b..6cf684699ada 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -148,10 +148,14 @@ static void ovpn_tcp_rcv(struct strparser *strp, struct
sk_buff *skb)
ovpn_recv(peer, skb);
return;
err:
- /* take reference for deferred peer deletion. should never fail */
- if (WARN_ON(!ovpn_peer_hold(peer)))
- goto err_nopeer;
- schedule_work(&peer->tcp.defer_del_work);
+ /* schedule deferred peer deletion and take a reference only if the
+ * work was actually queued: the matching ovpn_peer_put() in
+ * ovpn_tcp_peer_del_work() runs once per queued work, so re-arming an
+ * already-pending work must not take another reference (it would be
+ * leaked, e.g. on a flood of invalid packets)
+ */
+ if (schedule_work(&peer->tcp.defer_del_work))
+ ovpn_peer_hold(peer);
ovpn_dev_dstats_rx_dropped(peer->ovpn->dev);
err_nopeer:
kfree_skb(skb);
@@ -280,15 +284,20 @@ static void ovpn_tcp_send_sock(struct ovpn_peer *peer,
struct sock *sk)
peer->id, ret);
/* in case of TCP error we can't recover the VPN
- * stream therefore we abort the connection
+ * stream therefore we abort the connection.
+ *
+ * Take a reference only if the work was actually
+ * queued: ovpn_tcp_peer_del_work() drops exactly one
+ * reference per run, so re-arming an already-pending
+ * work (e.g. already scheduled from the RX path) must
+ * not take another reference (it would be leaked).
*/
- ovpn_peer_hold(peer);
- schedule_work(&peer->tcp.defer_del_work);
+ if (schedule_work(&peer->tcp.defer_del_work))
+ ovpn_peer_hold(peer);
/* we bail out immediately and keep tx_in_progress set
- * to true. This way we prevent more TX attempts
- * which would lead to more invocations of
- * schedule_work()
+ * to true, so that no further TX is attempted on the
+ * aborted stream
*/
return;
}
--
2.53.0
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
