Skip to content

Commit 6ccf64c

Browse files
itsGiaanColinIanKing
authored andcommitted
stress-ovpn: drive the remaining peer-delete reasons
The module reports why each peer was removed, and the five reasons come from genuinely different code paths. The churn was reaching only three: USERSPACE from deleting a peer, TRANSPORT_DISCONNECT from closing the transport, TEARDOWN when the interface goes away. EXPIRED and TRANSPORT_ERROR were never produced, so two teardown paths went untested while everything appeared covered - and both are worth reaching deliberately, since ovpn_tcp_peer_del_work() is where both of the bugs this stressor has found so far lived. EXPIRED comes from ovpn_peer_keepalive_work(), which needs both keepalive values non-zero and no traffic received for the timeout. The live peer never qualifies, traffic keeps arriving, and the random timeouts of up to 255s outlive any cycle. A phantom peer receives nothing by construction, so a two second timeout expires it within the cycle without having to orchestrate a pause in the traffic the live peer depends on. Setting the keepalive through PEER_NEW turned out not to be enough. ovpn_nl_peer_new_doit() applies the attributes before adding the peer to the table, and applying them arms the worker with a zero delay, so it runs while the peer is still invisible, finds no expiry to wait for, and only rearms itself when it found one. A peer whose keepalive arrived with PEER_NEW alone can therefore end up configured with nothing checking it. It is also sent a PEER_SET, which arms the worker with the peer present - the state a daemon reaches by setting keepalive after creating the peer. TRANSPORT_ERROR comes from ovpn_tcp_peer_del_work(), reached when the TCP stream cannot be parsed. ovpn frames its TCP transport with a length prefix and feeds the socket to a strparser, so writing raw bytes onto the transport makes that prefix nonsense and the module tears the peer down. UDP has no framing to corrupt, so the new case is TCP only. Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
1 parent f3383f1 commit 6ccf64c

1 file changed

Lines changed: 76 additions & 2 deletions

File tree

stress-ovpn.c

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1808,6 +1808,28 @@ static enum ovpn_cipher_alg ovpn_tun_cipher(void)
18081808
}
18091809
}
18101810

1811+
/*
1812+
* ovpn_tun_phantom_keepalive()
1813+
* give a phantom peer a keepalive short enough that it expires on its own.
1814+
*
1815+
* This is what reaches OVPN_DEL_PEER_REASON_EXPIRED, which nothing else
1816+
* here produces. ovpn_peer_keepalive_work() requires both values to be
1817+
* non-zero and then expires a peer that has received nothing for timeout
1818+
* seconds; the live peer never qualifies because traffic keeps arriving,
1819+
* and a random timeout of up to 255s outlives any cycle. A phantom peer
1820+
* receives nothing by construction, so a two second timeout expires it
1821+
* within the cycle without having to orchestrate a pause in the traffic.
1822+
*
1823+
* Worth reaching: the expiry path runs from a work item that two recent
1824+
* fixes in the module touched, and it is the route by which the NULL
1825+
* sk_socket dereference was originally reported upstream.
1826+
*/
1827+
static void ovpn_tun_phantom_keepalive(ovpn_ctx_t *o)
1828+
{
1829+
o->keepalive_interval = 1;
1830+
o->keepalive_timeout = 2;
1831+
}
1832+
18111833
/*
18121834
* ovpn_tun_phantom_rem_ip()
18131835
* random underlay address for a phantom peer. Nothing answers at it: a
@@ -2226,7 +2248,32 @@ static int ovpn_tunnel_add_peer(
22262248
return ret;
22272249
o->key_slot = OVPN_KEY_SLOT_SECONDARY;
22282250
o->key_id = 1;
2229-
return ovpn_new_key(o);
2251+
ret = ovpn_new_key(o);
2252+
if (ret < 0)
2253+
return ret;
2254+
2255+
/*
2256+
* Re-send the keepalive through PEER_SET, even though PEER_NEW already
2257+
* carried it.
2258+
*
2259+
* ovpn_nl_peer_new_doit() applies the attributes before adding the peer
2260+
* to the table, and applying them arms the keepalive worker with a zero
2261+
* delay. The worker therefore runs while the peer is still invisible,
2262+
* finds no expiry to wait for, and ovpn_peer_keepalive_work() only
2263+
* rearms itself when it found one:
2264+
*
2265+
* if (next_run > 0)
2266+
* schedule_delayed_work(&ovpn->keepalive_work, ...);
2267+
*
2268+
* so a peer whose keepalive arrived with PEER_NEW alone can end up
2269+
* configured with nothing ever checking it. PEER_SET afterwards arms it
2270+
* again with the peer present. A daemon reaches the same state by
2271+
* setting keepalive after creating the peer; doing it explicitly here is
2272+
* what makes OVPN_DEL_PEER_REASON_EXPIRED reachable at all.
2273+
*/
2274+
if (o->keepalive_interval && o->keepalive_timeout)
2275+
(void)ovpn_set_peer(o);
2276+
return 0;
22302277
}
22312278

22322279
/*
@@ -2373,7 +2420,7 @@ static void ovpn_tunnel_churn(
23732420
* buries the log in tens of thousands of expected ENOENTs.
23742421
* Unused on the client path below, which has its own selection.
23752422
*/
2376-
const int op = (int)(stress_mwc8() % (destructive ? 14 : 7));
2423+
const int op = (int)(stress_mwc8() % (destructive ? 15 : 7));
23772424

23782425
o->expect_failure = false;
23792426
if (!is_server) {
@@ -2438,6 +2485,7 @@ static void ovpn_tunnel_churn(
24382485

24392486
ovpn_tun_phantom_rem_ip(&rem, v6);
24402487
ovpn_tun_phantom_vpn_ip(&vpn, v6);
2488+
ovpn_tun_phantom_keepalive(o);
24412489

24422490
/* phantom peers need no real connection; only meaningful
24432491
* for UDP (a TCP peer is 1:1 with an accepted socket) */
@@ -2503,6 +2551,31 @@ static void ovpn_tunnel_churn(
25032551
o->socket = -1;
25042552
}
25052553
break;
2554+
case 14:
2555+
/*
2556+
* Corrupt the TCP stream under the module. ovpn frames
2557+
* its TCP transport with a length prefix and hands the
2558+
* socket to a strparser; raw bytes written here make
2559+
* that prefix nonsense, and the module tears the peer
2560+
* down with OVPN_DEL_PEER_REASON_TRANSPORT_ERROR.
2561+
*
2562+
* That is the one delete reason nothing else here
2563+
* reaches, and it is worth reaching deliberately: the
2564+
* work item it runs from, ovpn_tcp_peer_del_work(), is
2565+
* where both of the bugs this stressor has found so far
2566+
* lived - a NULL sk_socket dereference and a peer
2567+
* refcount leak.
2568+
*
2569+
* UDP has no framing to corrupt, so this is TCP only.
2570+
*/
2571+
if (is_tcp && (o->socket >= 0)) {
2572+
uint8_t junk[64];
2573+
2574+
stress_rndbuf((char *)junk, sizeof(junk));
2575+
VOID_RET(ssize_t, write(o->socket, junk,
2576+
1 + (stress_mwc8() % sizeof(junk))));
2577+
}
2578+
break;
25062579
}
25072580
}
25082581
o->expect_failure = false;
@@ -2848,6 +2921,7 @@ static int ovpn_tunnel_endpoint(
28482921

28492922
ovpn_tun_phantom_rem_ip(&rem, v6);
28502923
ovpn_tun_phantom_vpn_ip(&vpn, v6);
2924+
ovpn_tun_phantom_keepalive(o);
28512925

28522926
(void)ovpn_tunnel_add_peer(o, 2 + i, &rem,
28532927
(uint16_t)(1024 + (stress_mwc16() % 60000)), &vpn, true, false);

0 commit comments

Comments
 (0)