fix: check IS_ERR(wg) before using the device in wg_set_device - #206
Open
ExzoTikStyle wants to merge 2 commits into
Open
fix: check IS_ERR(wg) before using the device in wg_set_device#206ExzoTikStyle wants to merge 2 commits into
ExzoTikStyle wants to merge 2 commits into
Conversation
has_protection was initialised from awg_has_header_protection(wg) in the declaration block, before the IS_ERR(wg) check below it. For a request naming an interface that does not exist, lookup_interface() returns an ERR_PTR and the helper dereferences it immediately. The oops lands inside down_read(), while genl_rcv_msg() holds the genl family lock across the doit callback, so the lock is never released. Every subsequent netlink operation on the family then blocks in uninterruptible sleep — including awg setconf for interfaces that were working — and genl_unregister_family() hangs too, so the module can neither be used nor unloaded. Only a reboot recovers the host. BUG: kernel NULL pointer dereference, address: 000000000000050d RIP: 0010:down_read+0x1e/0xc0 Call Trace: awg_has_header_protection+0x19/0x50 [amneziawg] wg_set_device+0x43/0xe50 [amneziawg] genl_family_rcv_msg_doit+0xfa/0x160 genl_rcv_msg+0x4c/0xb0 Reproduced with: awg set doesnotexist header-protection-key /dev/null Note the call is the left operand of the ||, so it runs whether or not WGDEVICE_A_HEADER_PROTECTION_KEY was passed. Also guard awg_has_header_protection() itself, since it takes a device pointer from several call sites.
awg_header_protection_set_key() mutates p->key[] and p->has_protection while holding only a read lock, so concurrent setters can interleave and awg_header_protection_init() can read a half-updated key while building the chacha state. The failure mode is silent: header protection is applied with a wrong key for some packets rather than reporting an error. Unrelated to the crash fixed in the previous commit; drop this commit if you would rather keep that one on its own.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wg_set_device()initialiseshas_protectionfromawg_has_header_protection(wg)in the declaration block, above the
IS_ERR(wg)check. When the request names aninterface that doesn't exist,
lookup_interface()returns anERR_PTRand thehelper dereferences it right away.
The oops lands inside
down_read(), andgenl_rcv_msg()holds the genl familylock across the
doitcallback, so the lock is never released. Everything on theamneziawgfamily blocks in uninterruptible sleep from then on — includingawg setconffor tunnels that were up and working — andgenl_unregister_family()hangs too, so the module can neither be used nor unloaded. Only a reboot clears it.
rmmod -fisn't a way out either, since stock Ubuntu kernels build withoutCONFIG_MODULE_FORCE_UNLOAD.CR2being0x50drather than a near-zero address is theERR_PTRgiveaway:(void *)-19plus the offset ofheader_protection.lockinstruct wg_device.The call is the left operand of the
||, so it runs whether or notWGDEVICE_A_HEADER_PROTECTION_KEYwas passed — a plainawg set doesnotexist listen-port 51820reaches it too. Introduced in #192.The first commit moves the check above the use. It also adds an
IS_ERR_OR_NULL()guard insideawg_has_header_protection(), since it takes adevice pointer from several call sites.
The second commit is unrelated to the crash but in the same file:
awg_header_protection_set_key()writesp->key[]andp->has_protectionwhile holding only
down_read(). Concurrent setters can interleave, andawg_header_protection_init()can read a half-updated key while building thechacha state — silently wrong header protection rather than a reported error.
Happy to split it out if you'd rather keep this PR to the crash.
Verification
Built and run against 6.8.0-136-generic (Ubuntu 22.04.5), on the same host
where the oops was originally hit.
Before, on
3.0.20260731-04fromppa:amnezia/ppa:After, with this branch:
dmesgis clean, and an existing AWG 3 tunnel (withHeaderProtectionKeyset)comes back up on the patched module and passes traffic as before.