Skip to content

Commit 55b6c73

Browse files
zx2c4Paolo Abeni
authored and
Paolo Abeni
committed
wireguard: netlink: check for dangling peer via is_dead instead of empty list
If all peers are removed via wg_peer_remove_all(), rather than setting peer_list to empty, the peer is added to a temporary list with a head on the stack of wg_peer_remove_all(). If a netlink dump is resumed and the cursored peer is one that has been removed via wg_peer_remove_all(), it will iterate from that peer and then attempt to dump freed peers. Fix this by instead checking peer->is_dead, which was explictly created for this purpose. Also move up the device_update_lock lockdep assertion, since reading is_dead relies on that. It can be reproduced by a small script like: echo "Setting config..." ip link add dev wg0 type wireguard wg setconf wg0 /big-config ( while true; do echo "Showing config..." wg showconf wg0 > /dev/null done ) & sleep 4 wg setconf wg0 <(printf "[Peer]\nPublicKey=$(wg genkey)\n") Resulting in: BUG: KASAN: slab-use-after-free in __lock_acquire+0x182a/0x1b20 Read of size 8 at addr ffff88811956ec70 by task wg/59 CPU: 2 PID: 59 Comm: wg Not tainted 6.8.0-rc2-debug+ #5 Call Trace: <TASK> dump_stack_lvl+0x47/0x70 print_address_description.constprop.0+0x2c/0x380 print_report+0xab/0x250 kasan_report+0xba/0xf0 __lock_acquire+0x182a/0x1b20 lock_acquire+0x191/0x4b0 down_read+0x80/0x440 get_peer+0x140/0xcb0 wg_get_device_dump+0x471/0x1130 Cc: [email protected] Fixes: e7096c1 ("net: WireGuard secure network tunnel") Reported-by: Lillian Berry <[email protected]> Signed-off-by: Jason A. Donenfeld <[email protected]> Reviewed-by: Jiri Pirko <[email protected]> Signed-off-by: Paolo Abeni <[email protected]>
1 parent df9bbb5 commit 55b6c73

File tree

1 file changed

+3
-3
lines changed

1 file changed

+3
-3
lines changed

drivers/net/wireguard/netlink.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -255,17 +255,17 @@ static int wg_get_device_dump(struct sk_buff *skb, struct netlink_callback *cb)
255255
if (!peers_nest)
256256
goto out;
257257
ret = 0;
258-
/* If the last cursor was removed via list_del_init in peer_remove, then
258+
lockdep_assert_held(&wg->device_update_lock);
259+
/* If the last cursor was removed in peer_remove or peer_remove_all, then
259260
* we just treat this the same as there being no more peers left. The
260261
* reason is that seq_nr should indicate to userspace that this isn't a
261262
* coherent dump anyway, so they'll try again.
262263
*/
263264
if (list_empty(&wg->peer_list) ||
264-
(ctx->next_peer && list_empty(&ctx->next_peer->peer_list))) {
265+
(ctx->next_peer && ctx->next_peer->is_dead)) {
265266
nla_nest_cancel(skb, peers_nest);
266267
goto out;
267268
}
268-
lockdep_assert_held(&wg->device_update_lock);
269269
peer = list_prepare_entry(ctx->next_peer, &wg->peer_list, peer_list);
270270
list_for_each_entry_continue(peer, &wg->peer_list, peer_list) {
271271
if (get_peer(peer, skb, ctx)) {

0 commit comments

Comments
 (0)