On Tue, 1 Dec 2020 10:32:29 +0100
Sebastien Marie <sema...@online.fr> wrote:
> Jason, Matt,
>
> sthen@ told me that the same lock is reported several times (exactly,
> two locks are reported several times: lock1, lock2, lock1, lock2...)
>
> witness(4) reports when a lock doesn't have LO_INITIALIZED flag set in
> internal part of `struct rwlock'. Next it sets it, and skip futher
> analysis for this particular lock.
>
> if the same lock is reported several times, it means the memory is
> zeroed (and so the flag removed). it could be intented or not. but in
> all cases, a rwlock should be properly initialized with rw_init() or
> RWLOCK_INITIALIZER() before use.
>
> I don't have enough experiency with wg(4) stuff to well understand the
> issue. in wg_decap(), the peer is get from a `struct wg_tag` (from
> wg_tag_get()). If i didn't mess myself, the p_remote could come from
> wg_send_initiation() or wg_send_response(). but for both, it comes
> from wg_peer_create(), and p_remote is initialized with
> noise_remote_init() (and so lock have proper rw_init()).
>
> do you have idea on the cause of the lost of the rwlock flag ?
>
> if you want to test it with witness(4) enabled, you will need to build
> a kernel with "option WITNESS" in config file. you could uncomment it
> from sys/arch/amd64/conf/GENERIC.MP, and run make config, make clean,
> make
Hi,
This certainly isn't intentional.
I had a quick look and believe the following patch should fix it. The
rwlock in the antireplay counter is rw_init'ed and then bzero'ed,
matching the symptoms. It also explains why lock1,lock2,lock1,... was
seen as there are generally two counters in use (one for the current
session and one for the previous session). It wouldn't make sense for
r_keypair_lock to be in two different locations for the same peer.
I haven't tested it my side but I'm quite confident it should fix the
unitialised lock issue.
Cheers,
Matt
--- net/wg_noise.c
+++ net/wg_noise.c
@@ -467,8 +467,8 @@ noise_remote_begin_session(struct noise_remote *r)
kp.kp_local_index = hs->hs_local_index;
kp.kp_remote_index = hs->hs_remote_index;
getnanouptime(&kp.kp_birthdate);
- rw_init(&kp.kp_ctr.c_lock, "noise_counter");
bzero(&kp.kp_ctr, sizeof(kp.kp_ctr));
+ rw_init(&kp.kp_ctr.c_lock, "noise_counter");
/* Now we need to add_new_keypair */
rw_enter_write(&r->r_keypair_lock);
