| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| |
|
|
|
|
|
| |
This is weird and no library should implement it. Bring it into wg_noise
instead.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: John Baldwin <jhb@FreeBSD.org>
|
| |
|
|
|
|
| |
This permits returning different error codes for different conditions.
Signed-off-by: John Baldwin <jhb@FreeBSD.org>
|
| |
|
|
| |
Signed-off-by: John Baldwin <jhb@FreeBSD.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
There were multiple places where a rwlock was used despite never
rlocking, so just change these into mtxs. This was done with the aid of
Coccinelle's spatch, using this input:
#spatch -j 4 --recursive-includes --include-headers-for-types --include-headers --in-place --macro-file <seebelow.h>
virtual after_start
@initialize:ocaml@
@@
let has_write_table = Hashtbl.create 101
let has_read_table = Hashtbl.create 101
let ok i m =
let entry = (i,m) in
Hashtbl.mem has_write_table entry && not(Hashtbl.mem has_read_table entry)
@hasw depends on !after_start@
identifier i,m;
struct i x;
@@
(
rw_wlock(&x.m)
|
rw_wunlock(&x.m)
)
@script:ocaml@
i << hasw.i;
m << hasw.m;
@@
Hashtbl.replace has_write_table (i,m) ()
@hasr depends on !after_start@
identifier i,m;
struct i x;
@@
(
rw_rlock(&x.m)
|
rw_runlock(&x.m)
)
@script:ocaml@
i << hasr.i;
m << hasr.m;
@@
Hashtbl.replace has_read_table (i,m) ()
@finalize:ocaml depends on !after_start@
wt << merge.has_write_table;
rt << merge.has_read_table;
@@
let redo ts dst =
List.iter (Hashtbl.iter (fun k _ -> Hashtbl.add dst k ())) ts in
redo wt has_write_table;
redo rt has_read_table;
let it = new iteration() in
it#add_virtual_rule After_start;
it#register()
(* ----------------------------------------------------------- *)
@depends on after_start@
identifier i;
identifier m : script:ocaml(i) { ok i m };
@@
struct i {
...
- struct rwlock m;
+ struct mtx m;
...
}
@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@
- rw_wlock
+ mtx_lock
(&x.m)
@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@
- rw_wunlock
+ mtx_unlock
(&x.m)
@depends on after_start disable fld_to_ptr@
identifier m;
expression e;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@
- rw_init(&x.m, e);
+ mtx_init(&x.m, e, NULL, MTX_DEF);
@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@
- rw_destroy
+ mtx_destroy
(&x.m)
@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@
- rw_wlock
+ mtx_lock
(&x->m)
@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@
- rw_wunlock
+ mtx_unlock
(&x->m)
@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
expression e;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@
- rw_init(&x->m, e);
+ mtx_init(&x->m, e, NULL, MTX_DEF);
@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@
- rw_destroy
+ mtx_destroy
(&x->m)
A few macros needed to be provided manually for the parser to work:
#define LIST_HEAD(x,y) int
#define TAILQ_HEAD(x,y) int
#define STAILQ_HEAD(x,y) int
#define CK_LIST_HEAD(x,y) int
#define CK_LIST_ENTRY(x) int
#define LIST_ENTRY(x) int
#define TAILQ_ENTRY(x) int
#define STAILQ_ENTRY(x) int
Co-authored-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
|
|
|
| |
Before, most uses of rwlock and mtx never called the destroy method,
which might cause problems for witness.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Otherwise CK_LIST_REMOVE might be called twice on the same element.
Running the following trigger will reproduce the bug that Manojav
reported:
#!/usr/local/bin/bash
NUM_PEER=50
peer_args=( )
for ((i=0; i<NUM_PEER; i++)); do
port="$RANDOM"
private="$(wg genkey)"
ifconfig "wg$i" create
wg set "wg$i" listen-port "$port" private-key <(echo "$private")
peer_args+=( peer "$(wg pubkey <<<"$private")" endpoint "127.0.0.1:$port" persistent-keepalive 1 )
done
for ((i=0; i<NUM_PEER; i++)); do
wg set "wg$i" "${peer_args[@]}"
ifconfig "wg$i" up
done
wg
while true; do
for ((i=0; i<NUM_PEER; i++)); do
ifconfig "wg$i" down
ifconfig "wg$i" up
done
done
Reported-by: Manojav Sridhar <manojav@manojav.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
|
|
|
|
|
|
|
| |
In the occasion that noise_begin_session returns != 0, we could
accidentally leak the remote refcount, as the caller to
consume_response only expects *rp to be set when ret == 0.
The only situation we could leak this is if we cannot allocate memory
for the new keypair.
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
| |
The lack of 64bit atomic helpers on 32bit is an annoyance.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The peer (and keypair and local) lifecycle are managed through EPOCH and
refcounts. Primarily this is used in wg_noise to keep track of active
keypairs, however we can also use it to be sure no more peer references
exist. The structures are linked as such, so noise_remote cannot be
freed until all noise_keypairs are freed, and noise_local cannot be
freed until all noise_remotes are freed.
noise_keypair -> noise_remote -> noise_local
Therefore, if you hold a keypair reference you can be sure that remote
and local will still be around.
There are three main ways peers are referenced:
1) Incoming packets
1.a) Incoming handshake packets are passed to noise_consume_*,
which will (on success) return a refcounted remote which is dropped
at the end of wg_handshake.
1.b) Incoming cookie packets will have their index looked which will (on
success) return a refcounted remote, which is also dropped at the
end of wg_handshake.
1.c) Incoming data packets will have their index looked up which will
(on success) return a refcounted keypair. This keypair will be
dropped after the packet has been passed up the network stack, or
otherwise freed.
2) Outgoing data packets
2.a) Outgoing data packets are first looked up by wg_aip_lookup, which
returns a peer pointer, with an incremented remote refcount. This
is then dropped in wg_transmit after adding the packet to the
staged queue and sending the staged queue.
2.b) Packets in the staged queue do not hold any refcount for the remote
or keypair, because they do not reference the peer in any way, they
are just in the queue.
2.c) Packets finally get a refcoutned keypair in wg_peer_send_staged,
which is dropped after the packet is sent out the UDP socket, or
otherwise freed.
3) wg_timers system
3.a) The wg_timers system holds a reference to the peer whenever a
callout is scheduled. Instead of holding a refcount, we instead
disable the peer's timers, such that no callouts can be scheduled.
Some rationale for changes here:
We move the p_{send,recv} taskqgroup_detach into peer_free_deferred as
they will NULL fields in p_{send,recv}. If there are packets being
processed in wg_{en,de}crypt, then a call tou GROUPTASK_ENQUEUE will
dereference a NULL pointer. In general, we remove all references to the
peer in wg_peer_destroy, and free/deinit all the peer members once no
more references to the remote exist, in wg_peer_free_deferred.
Currently we take a refcount in wg_aip_lookup, which is to be sure that
the peer reference is valid for the entirety of wg_transmit. We do not
care about the refcount in wg_decrypt. It might be worth considering
storing the remote pointer in the allowedip entry, but it could be
argued both ways. For the time being, this is still correct. We don't
have a refcount for the peer stored in the allowedip table, as it is
protected by the table lock. One note here is the NULL p_remote check is
necessary to support selftest/allowedips.c, which does not specify a
p_remote. If we update the tests, then we may remove this check.
There are two added p_enabled checks, in run_retry_handshake and
run_send_keepalive. This is to align them with the other callout_reset
calls. In the case of p_zero_key_material, if we have set p_enabled =
false, then we subsequently clear keypairs and handshakes (on wg_down),
or we free the peer which will clear the keypairs for us.
We want to hold a refcount of remote in wg_{en,de}crypt to ensure that
the peer is still valid in the call to GROUPTASK_ENQUEUE. If we don't
then peer may become invalid after setting p_state. Another thread may
take the packet, put the keypair refcount and free the peer prior to the
call to GROUPTASK_ENQUEUE.
We no longer need to hold (haven't for a while) the EPOCH in
wg_send_initiation and wg_send_response, as we hold valid references for
the duration. This could be either a refcount of a remote or through the
wg_timers system as described above.
We also fix some refcount leaks in wgc_set.
Notes:
We may want to pull NET_EPOCH_WAIT out of wg_timers_disable, to improve
performance. However, we can destroy 20000 peers in less than 20ms so
the performance is not critical for this snapshot and can be addressed
later.
Finally, there is the special case of noise_remote_arg, which stores the
corresponding peer pointer. The peer is not refcounted however it will
have the same scope as the remote. In otherwords it is valid until we
call noise_remote_put on the remote.
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
|
| |
Good for hygiene, but also, lock hardening traps on double initialization
if we don't do this.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
So the last change broke consuming responses, as it may return an
invalid remote pointer. Thanks for the catch zx2c4. We just pass a flag
"lookup_keypair" which will lookup the keypair when we want (for cookie)
and will not when we don't (for consuming responses).
It would be possible to merge both noise_remote_index_lookup and
noise_keypair_lookup, but the result would probably need to return a
void * (for both keypair and remote) or a noise_index * which would need
to be cast to the relevant type somewhere. The trickiest thing here
would be for if_wg to "put" the result of the function, as it may be a
remote or a keypair (which store their refcount in different locations).
Perhaps it would return a noise_index * which could contain the refcount
for both keypair and remote. It all seems easier to leave them separate.
The only argument for combining them would be to reduce duplication of
(similar) functions.
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
|
|
|
| |
This is needed, to remove the peer from the public key hashtable before
calling noise_remote_destroy. This will prevent any incoming handshakes
from starting in that time. It also cleans up the insert path to make it
more like it was before the wg_noise EPOCH changes.
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
| |
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
|
|
|
|
|
| |
This introduces a couple of routines to encrypt the mbufs in place. It
is likely that these will be replaced by something in opencrypto,
however for the time being this fixes a heap overflow and sets up
wg_noise for the "correct" API. When the time comes, this should make it
easier to drop in new crypto. It should be noted, this was written at
0500.
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Note: this is a partial diff, introducing temporary bugs that will be
resolved in following commits, detailed below.
This commit brings wg_noise.{c,h} up to date with wireguard-openbsd. The
primary motivator for this large patchset is to allow checking nonces
serial, requiring a reference to the receiving keypair across noise_*
calls. Due to requiring reference counting on the keypairs, we also take
this opportunity to throw away the old locking and bring in EPOCH
(roughly equivalent to SMR on OpenBSD and RCU on Linux).
The changes to if_wg.c are purely to allow it to compile, there are most
certainly refcount leaks present (to be addressed in the following
commits). Readers should review wg_noise.{c,h} in their entirety rather
than the diffs, as there are significant changes. if_wg.c can be
reviewed, but must be contextualised with the following commits
(repace wg_tag with wg_packet, encrypt mbuf in place).
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
As reported by: https://marc.info/?l=openbsd-bugs&m=161618496905444&w=2
In particular, when consuming an initiation, we don't generate the
index until creating the response (which is incorrect). If we attempt
to create an initiation between these processes, we drop any
outstanding handshake which in this case has index 0 as set when
consuming the initiation.
The fix attached is to generate the index when consuming the initiation
so that any spurious initiation creation can drop a valid index. The
patch also consolidates setting fields on the handshake.
Signed-off-by: Matt Dunwoodie <ncon@noconroy.net>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This should allow us to get more testing coverage earlier.
This port here is also a bit janky. I really don't like the taskqgroup
business, having to copy and paste those structs. And this isn't well
tested, either. But, it's a start.
This distinguishes between compat.h and support.h, though both header
files are intended to operate in more or less the same way. It's
important to keep some discipline between things that we're backporting
and things that aren't _yet_ upstream or are shims for OpenBSD.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
|
|
There's still more to do with wiring this up properly.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
|
Jason A. Donenfeld
John Baldwin
Matt Dunwoodie