Page MenuHomeFreeBSD

wg: don't unlink an unlinked peer
AbandonedPublic

Authored by kevans on Sep 14 2024, 3:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 12, 3:41 PM
Unknown Object (File)
Sat, Oct 11, 2:32 AM
Unknown Object (File)
Thu, Oct 2, 11:29 PM
Unknown Object (File)
Thu, Oct 2, 1:19 PM
Unknown Object (File)
Tue, Sep 30, 8:58 AM
Unknown Object (File)
Sep 12 2025, 9:43 PM
Unknown Object (File)
Aug 4 2025, 8:44 AM
Unknown Object (File)
Jun 16 2025, 4:53 PM

Details

Summary

This can happen in the case of pretty much any failure in wg_peer_add(),
we end up underflowing sc_peers_num and doing bad things with the tailq
because peers are only accounted for and added at the very end of
wg_peer_add().

Just add a parameter to wg_peer_destroy() to indicate whether the peer
was actually linked in or not, so that we can avoid botching the
accounting. The other parts of wg_peer_destroy() are still generally
applicable.

This path is somewhat hard to exercise since wg(8) does validation of
most things that can cause a failure, so no test is added. The easiest
reproducer is to build a kernel with INET removed and configure a peer
with an IPv4 allowed-ip.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59481
Build 56368: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Sep 14 2024, 3:26 PM

Hi. Sorry for my delayed reply.

I actually noticed this issue (inconsistency between sc_peers_num and sc_peers TAILQ) and I have a TODO to work on a patch for FreeBSD.

In DragonFly, I fixed the issue in another way (by referring to OpenBSD's) as you may find in commit: https://github.com/DragonFlyBSD/DragonFlyBSD/commit/902964ab24ba9d2c978017d369c0faa8d2fe0f9e

(I found the issue when I was working on replacing nvlist with ioctl.)

Hi. Sorry for my delayed reply.

I actually noticed this issue (inconsistency between sc_peers_num and sc_peers TAILQ) and I have a TODO to work on a patch for FreeBSD.

In DragonFly, I fixed the issue in another way (by referring to OpenBSD's) as you may find in commit: https://github.com/DragonFlyBSD/DragonFlyBSD/commit/902964ab24ba9d2c978017d369c0faa8d2fe0f9e

(I found the issue when I was working on replacing nvlist with ioctl.)

I think your approach is nicer. (In general I kind of dislike using bool flags to modulate functions this way: it's hard to see what's going on without jumping to the function definition, and the control flow becomes more complicated. Here it's relatively straightforward, but refactoring a bit is nicer IMHO.)

Hi. Sorry for my delayed reply.

I actually noticed this issue (inconsistency between sc_peers_num and sc_peers TAILQ) and I have a TODO to work on a patch for FreeBSD.

In DragonFly, I fixed the issue in another way (by referring to OpenBSD's) as you may find in commit: https://github.com/DragonFlyBSD/DragonFlyBSD/commit/902964ab24ba9d2c978017d369c0faa8d2fe0f9e

(I found the issue when I was working on replacing nvlist with ioctl.)

I think your approach is nicer. (In general I kind of dislike using bool flags to modulate functions this way: it's hard to see what's going on without jumping to the function definition, and the control flow becomes more complicated. Here it's relatively straightforward, but refactoring a bit is nicer IMHO.)

+1- I'm intending to port that over here because that seems like the much better approach in hindsight, but I haven't found time just yet.

The alternative (better) approach has been committed.