The 'l_identity_lock' lock must be acquired to access 'l_has_identity' and 'l_private' members; i.e., noise_precompute_ss() must be called with the 'l_identity_lock' locked. Fix noise_remote_alloc() to acquire the lock before calling noise_precompute_ss(). Meanwhile, add an assertion to the latter to assert the required lock is held. Below is my suggested patch: --- wg_noise.c.orig 2024-01-16 22:53:33.518906792 +0800 +++ wg_noise.c 2024-01-16 23:21:16.069687841 +0800 @@ -281,6 +281,8 @@ noise_local_keys(struct noise_local *l, static void noise_precompute_ss(struct noise_local *l, struct noise_remote *r) { + rw_assert(&l->l_identity_lock, RA_LOCKED); + rw_wlock(&r->r_handshake_lock); if (!l->l_has_identity || !curve25519(r->r_ss, l->l_private, r->r_public)) @@ -302,7 +304,10 @@ noise_remote_alloc(struct noise_local *l r->r_handshake_state = HANDSHAKE_DEAD; r->r_last_sent = TIMER_RESET; r->r_last_init_recv = TIMER_RESET; + + rw_wlock(&l->l_identity_lock); noise_precompute_ss(l, r); + rw_wunlock(&l->l_identity_lock); refcount_init(&r->r_refcnt, 1); r->r_local = noise_local_ref(l);
That's fair, though this should likely just rlock it rather than wlock it.
Yes, it should be a read lock, and that’s why I use RA_LOCKED in the added rw_assert(). I made a typo in the previous patch. Thanks. --- wg_noise.c.orig 2024-01-16 22:53:33.518906792 +0800 +++ wg_noise.c 2024-01-16 23:21:16.069687841 +0800 @@ -281,6 +281,8 @@ noise_local_keys(struct noise_local *l, static void noise_precompute_ss(struct noise_local *l, struct noise_remote *r) { + rw_assert(&l->l_identity_lock, RA_LOCKED); + rw_wlock(&r->r_handshake_lock); if (!l->l_has_identity || !curve25519(r->r_ss, l->l_private, r->r_public)) @@ -302,7 +304,10 @@ noise_remote_alloc(struct noise_local *l r->r_handshake_state = HANDSHAKE_DEAD; r->r_last_sent = TIMER_RESET; r->r_last_init_recv = TIMER_RESET; + + rw_rlock(&l->l_identity_lock); noise_precompute_ss(l, r); + rw_runlock(&l->l_identity_lock); refcount_init(&r->r_refcnt, 1); r->r_local = noise_local_ref(l);
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=7a4d1d1df0b2e369adcb32aea9ef8c180f885751 commit 7a4d1d1df0b2e369adcb32aea9ef8c180f885751 Author: Aaron LI <aly@aaronly.me> AuthorDate: 2024-01-17 23:29:23 +0000 Commit: Kyle Evans <kevans@FreeBSD.org> CommitDate: 2024-01-17 23:29:54 +0000 if_wg: fix access to noise_local->l_has_identity and l_private These members are protected by the identity lock, so rlock it in noise_remote_alloc() and then assert that we have it held to some extent in noise_precompute_ss(). PR: 276392 sys/dev/wg/wg_noise.c | 4 ++++ 1 file changed, 4 insertions(+)
Thanks!
In practice the device's sc_lock is xlocked in both callers and this can't really race, but I think it makes sense to be pedantic/defensive about it.
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=ce2d249b207045f27ec03719997168c92d0f6104 commit ce2d249b207045f27ec03719997168c92d0f6104 Author: Aaron LI <aly@aaronly.me> AuthorDate: 2024-01-17 23:29:23 +0000 Commit: Kyle Evans <kevans@FreeBSD.org> CommitDate: 2024-01-30 05:37:57 +0000 if_wg: fix access to noise_local->l_has_identity and l_private These members are protected by the identity lock, so rlock it in noise_remote_alloc() and then assert that we have it held to some extent in noise_precompute_ss(). PR: 276392 (cherry picked from commit 7a4d1d1df0b2e369adcb32aea9ef8c180f885751) sys/dev/wg/wg_noise.c | 4 ++++ 1 file changed, 4 insertions(+)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=b0fa37f356fd9dd5040c034c7523ec235fbb311b commit b0fa37f356fd9dd5040c034c7523ec235fbb311b Author: Aaron LI <aly@aaronly.me> AuthorDate: 2024-01-17 23:29:23 +0000 Commit: Kyle Evans <kevans@FreeBSD.org> CommitDate: 2024-01-30 05:37:33 +0000 if_wg: fix access to noise_local->l_has_identity and l_private These members are protected by the identity lock, so rlock it in noise_remote_alloc() and then assert that we have it held to some extent in noise_precompute_ss(). PR: 276392 (cherry picked from commit 7a4d1d1df0b2e369adcb32aea9ef8c180f885751) sys/dev/wg/wg_noise.c | 4 ++++ 1 file changed, 4 insertions(+)
MFC'd to all supported branches; thanks!