[PATCH] wireguard: release correct lock on exceptional case

[Jason A. Donenfeld]

Backtrace from Jesper:

ddb{0}> show panic
noise_keypair: lock not held
ddb{0}> trace
db_enter() at db_enter+0x10
panic(81db9b58) at panic+0x12a
rw_exit_write(8801ed10) at rw_exit_write+0xb5
noise_remote_begin_session(8801ec10) at noise_remote_begin_session+0x3c
1
wg_send_response(8801ebe0) at wg_send_response+0x7b
wg_handshake(80588000,fd800e7b5a00) at wg_handshake+0x576
wg_handshake_worker(80588000) at wg_handshake_worker+0x48
taskq_thread(80049200) at taskq_thread+0x81
end trace frame: 0x0, count: -8
ddb{0}> machine ddbcpu 1

Reported-by: Jasper Lievisse Adriaanse 
---
 sys/net/wg_noise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git sys/net/wg_noise.c sys/net/wg_noise.c
index 66bdecee80e..adb00568eb4 100644
--- sys/net/wg_noise.c
+++ sys/net/wg_noise.c
@@ -459,7 +459,7 @@ noise_remote_begin_session(struct noise_remote *r)
NOISE_SYMMETRIC_KEY_LEN, NOISE_SYMMETRIC_KEY_LEN, 0, 0,
hs->hs_ck);
} else {
-   rw_exit_write(>r_keypair_lock);
+   rw_exit_write(>r_handshake_lock);
return EINVAL;
}
 
-- 
2.29.1

[PATCH] tcpdump: special case dumping of layer 3 packets

[Jason A. Donenfeld]

Different link types get different printers. But until now, different
link types would get the same pcap dumper. This is mostly fine, except
for on LOOP-type interfaces, which have raw layer 3 packets. On these,
the first four bytes of the packet represent the AF family, which the
pcap data format doesn't care for. So, trying to open the result in
Wireshark is gibberish. This patch makes packet dumping act like packet
printing, which certain link types special cased. In this case,
LOOP-type packets get the first four bytes shaved off.
---
 usr.sbin/tcpdump/interface.h  |  1 +
 usr.sbin/tcpdump/print-null.c |  9 +
 usr.sbin/tcpdump/tcpdump.c| 19 ++-
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git usr.sbin/tcpdump/interface.h usr.sbin/tcpdump/interface.h
index 602be405723..147036f7d1e 100644
--- usr.sbin/tcpdump/interface.h
+++ usr.sbin/tcpdump/interface.h
@@ -249,6 +249,7 @@ extern void nfsreply_print(const u_char *, u_int, const 
u_char *);
 extern void nfsreq_print(const u_char *, u_int, const u_char *);
 extern void ns_print(const u_char *, u_int, int);
 extern void ntp_print(const u_char *, u_int);
+extern void loop_if_dump(u_char *, const struct pcap_pkthdr *, const u_char *);
 extern void loop_if_print(u_char *, const struct pcap_pkthdr *, const u_char 
*);
 extern void null_if_print(u_char *, const struct pcap_pkthdr *, const u_char 
*);
 extern void ospf_print(const u_char *, u_int, const u_char *);
diff --git usr.sbin/tcpdump/print-null.c usr.sbin/tcpdump/print-null.c
index f90f5e9030c..94457532c90 100644
--- usr.sbin/tcpdump/print-null.c
+++ usr.sbin/tcpdump/print-null.c
@@ -96,6 +96,15 @@ null_print(const u_char *p, const struct ip *ip, u_int 
length)
}
 }
 
+void
+loop_if_dump(u_char *user, const struct pcap_pkthdr *h, const u_char *p)
+{
+   struct pcap_pkthdr h_mod = *h;
+   h_mod.caplen -= sizeof(u_int);
+   h_mod.len -= sizeof(u_int);
+   pcap_dump(user, _mod, p + sizeof(u_int));
+}
+
 void
 loop_if_print(u_char *user, const struct pcap_pkthdr *h, const u_char *p)
 {
diff --git usr.sbin/tcpdump/tcpdump.c usr.sbin/tcpdump/tcpdump.c
index bbdba4d4c48..40323870ed9 100644
--- usr.sbin/tcpdump/tcpdump.c
+++ usr.sbin/tcpdump/tcpdump.c
@@ -110,6 +110,23 @@ struct printer {
 #define DLT_ATM_RFC1483 11
 #endif
 
+static struct printer dumpers[] = {
+   { loop_if_dump, DLT_LOOP },
+   { NULL, 0 },
+};
+
+static pcap_handler
+lookup_dumper(int type)
+{
+   struct printer *p;
+
+   for (p = dumpers; p->f; ++p) {
+   if (type == p->type)
+   return p->f;
+   }
+   return pcap_dump;
+}
+
 static struct printer printers[] = {
{ ether_if_print,   DLT_EN10MB },
{ ether_if_print,   DLT_IEEE802 },
@@ -507,7 +524,7 @@ main(int argc, char **argv)
fflush(fp);
setvbuf(fp, NULL, _IONBF, 0);
}
-   printer = pcap_dump;
+   printer = lookup_dumper(pcap_datalink(pd));
pcap_userdata = (u_char *)p;
} else {
printer = lookup_printer(pcap_datalink(pd));
-- 
2.29.1

[PATCH] ifconfig: keep track of allowed ips pointer correctly

[Jason A. Donenfeld]

Somebody on IRC mentioned that using ifconfig to set wgallowedips wasn't
working on macppc. I don't have a macppc to test this on, but it seems
like the code is assuming that the two values printed out by this test
program must always be the same:

  struct s {
  int i;
  };

  struct p {
  long l;
  char c;
  struct s a[];
  };

  int main(int argc, char *argv[])
  {
  printf("%zu %zu\n", sizeof(struct p), (size_t)&((struct p *)0)->a[0]);
  return 0;
  }

But actually, on my amd64 system, that little test prints out "16 12".
This patch fixes up ifconfig.c to do the right thing, so that it
corresponds with how the kernel handles iteration.

I don't have a macppc in order to test this, but it works on amd64.
---
 sbin/ifconfig/ifconfig.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git sbin/ifconfig/ifconfig.c sbin/ifconfig/ifconfig.c
index 2ccbac6f4ce..ade6e4943b7 100644
--- sbin/ifconfig/ifconfig.c
+++ sbin/ifconfig/ifconfig.c
@@ -5696,11 +5696,10 @@ ensurewginterface(void)
err(1, "calloc");
 }
 
-void *
+void
 growwgdata(size_t by)
 {
ptrdiff_t peer_offset, aip_offset;
-   void *ret;
 
if (wg_interface == NULL)
wgdata.wgd_size = sizeof(*wg_interface);
@@ -5721,16 +5720,18 @@ growwgdata(size_t by)
if (wg_aip != NULL)
wg_aip = (void *)wg_interface + aip_offset;
 
-   ret = (void *)wg_interface + wgdata.wgd_size - by;
-   bzero(ret, by);
-
-   return ret;
+   bzero((void *)wg_interface + wgdata.wgd_size - by, by);
 }
 
 void
 setwgpeer(const char *peerkey_b64, int param)
 {
-   wg_peer = growwgdata(sizeof(*wg_peer));
+   growwgdata(sizeof(*wg_peer));
+   if (wg_aip)
+   wg_peer = (struct wg_peer_io *)wg_aip;
+   else
+   wg_peer = _interface->i_peers[0];
+   wg_aip = _peer->p_aips[0];
wg_peer->p_flags |= WG_PEER_HAS_PUBLIC;
WG_LOAD_KEY(wg_peer->p_public, peerkey_b64, "wgpeer");
wg_interface->i_peers_count++;
@@ -5743,7 +5744,7 @@ setwgpeeraip(const char *aip, int param)
if (wg_peer == NULL)
errx(1, "wgaip: wgpeer not set");
 
-   wg_aip = growwgdata(sizeof(*wg_aip));
+   growwgdata(sizeof(*wg_aip));
 
if ((res = inet_net_pton(AF_INET, aip, _aip->a_ipv4,
sizeof(wg_aip->a_ipv4))) != -1) {
@@ -5759,6 +5760,8 @@ setwgpeeraip(const char *aip, int param)
 
wg_peer->p_flags |= WG_PEER_REPLACE_AIPS;
wg_peer->p_aips_count++;
+
+   wg_aip++;
 }
 
 void
-- 
2.29.1

Re: fix build without pf

[Jason A. Donenfeld]

ok zx2c4

Re: get public key as non-root

[Jason A. Donenfeld]

On Fri, Jul 3, 2020 at 11:47 AM Klemens Nanni  wrote:
> Is there any particular reason why an interface's *public* key is only
> shown to the root user in ifconfig?

Yes, there is a reason for this.

The WireGuard protocol has a property called "identity hiding". See
section 3.4 and 4.3.4 lemma 7 of

or section 7.8 of
. The mac1
value also relies on this identity hiding property. In other words,
public keys should not be easily broadcasted and should not be
accessible to unprivileged users.

Re: wg(4): encapsulated transport checksums

[Jason A. Donenfeld]

Hi Richard,

Thanks for the patch. I had problems parsing some terminology in your
description, so I thought I'd lay out my understanding of the matter,
and you can let me know whether or not this corresponds with what you
had in mind:

- On egress, we must compute the packet checksum, because it may well
be forwarded by the receiving end after decapsulation. That doesn't
concern this patch, however.

- On ingress, we've already checked the poly1305 sum, so we have no
doubt that the packet has arrived without corruption.
- Therefore, it's not necessary to check the IP checksum on ingress because:
  * If the packet originated on the peer that did the encapsulation,
there's no chance for corruption;
  * If the packet did not originate on the peer that did the
encapsulation, it was that peer's responsibility to drop it if the
checksum was wrong;
  * If the packet does have an incorrect checksum, because the
originating peer did not check it, and we forward it along, the
machine we forward it to will drop it.

It seemed like from your message that you had a case in mind in which
it actually would be necessary to check the IP checksum on ingress,
but I didn't quite divine what you had in mind.

Jason

On Fri, Jun 26, 2020 at 10:03 PM  wrote:
>
> Hi,
>
> On its receive path, wg(4) uses the same mbuf for both the encrypted
> capsule and its encapsulated packet, which it passes up to the stack. We
> must therefore clear this mbuf's checksum status flags, as although the
> capsule may have been subject to hardware offload, its encapsulated packet
> was not.
>
> This ensures that the transport checksums of packets bound for local
> delivery are verified. That is necessary because, although the tunnel
> provides stronger integrity checks, the tunnel endpoints and the
> transport endpoints needn't coincide.
>
> However, as the network and tunnel endpoints _do_ conincide, it remains
> unncessary to check the per-hop IPv4 checksum.
>
> ok?
>
> Index: net/if_wg.c
> ===
> RCS file: /cvs/src/sys/net/if_wg.c,v
> retrieving revision 1.7
> diff -u -p -u -p -r1.7 if_wg.c
> --- net/if_wg.c 23 Jun 2020 10:03:49 -  1.7
> +++ net/if_wg.c 27 Jun 2020 02:48:37 -
> @@ -1660,14 +1660,10 @@ wg_decap(struct wg_softc *sc, struct mbu
> goto error;
> }
>
> -   /*
> -* We can mark incoming packet csum OK. We mark all flags OK
> -* irrespective to the packet type.
> -*/
> -   m->m_pkthdr.csum_flags |= (M_IPV4_CSUM_IN_OK | M_TCP_CSUM_IN_OK |
> -   M_UDP_CSUM_IN_OK | M_ICMP_CSUM_IN_OK);
> -   m->m_pkthdr.csum_flags &= ~(M_IPV4_CSUM_IN_BAD | M_TCP_CSUM_IN_BAD |
> -   M_UDP_CSUM_IN_BAD | M_ICMP_CSUM_IN_BAD);
> +   /* tunneled packet was not offloaded */
> +   m->m_pkthdr.csum_flags = 0;
> +   /* optimise: the tunnel provided a stronger integrity check */
> +   m->m_pkthdr.csum_flags |= M_IPV4_CSUM_IN_OK;
>
> m->m_pkthdr.ph_ifidx = sc->sc_if.if_index;
> m->m_pkthdr.ph_rtableid = sc->sc_if.if_rdomain;

Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

[Jason A. Donenfeld]

On Thu, Jun 25, 2020 at 3:54 AM Vitaliy Makkoveev
 wrote:
> ifp = ifunit(name);
> if (ifp == NULL)
> return (ENXIO);
> +   ifp->if_dying = 1;

Reference counting, plus an explicit tear-down window, and wait
period, like you've proposed sounds like a good idea. You'll probably
want to make if_dying volatile or cast it to that so that the compiler
doesn't reorder it. But I wonder, at this point, why not go full-on
srp/rcu?

Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

[Jason A. Donenfeld]

On Thu, Jun 25, 2020 at 2:49 AM Martin Pieuchot  wrote:
>
> On 24/06/20(Wed) 19:54, Jason A. Donenfeld wrote:
> > On Wed, Jun 24, 2020 at 4:02 AM Martin Pieuchot  wrote:
> > > Yes, that might be a better way.  If I understood your original mail the
> > > issue is related to ifunit(), right?  ifunit() is not used in packet-
> > > processing contexts, that's why we did not protect it by the NET_LOCK().
> > >
> > > I'm not sure if all the ifunit() usages are necessary, many of them are
> > > certainly exposing races with attach/destroy.
> >
> > No, not _just_ ifunit, but ifunit is one of the places where this is
> > hit. But just one.
>
> Which ones then?  I'd be delighted if you could share those facts.

You make it sound as if I'm deliberately concealing it in order to
dangle a tasty orange in front of you, but that's not what I meant.
Just look around in the source code at all of the code that uses an
ifp outside of a reference count or other locking semantics. Grepping
around, for example, I found
ifioctl->ifioctl_get->ifconf->if_list->kaboom.

There's lots of interesting behavior in there, and a pretty good
indication that you really don't want ioctls racing with either clone
or destroy, which is what my patch fixes.

Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

[Jason A. Donenfeld]

Hi Martin,

On Wed, Jun 24, 2020 at 4:02 AM Martin Pieuchot  wrote:
> Yes, that might be a better way.  If I understood your original mail the
> issue is related to ifunit(), right?  ifunit() is not used in packet-
> processing contexts, that's why we did not protect it by the NET_LOCK().
>
> I'm not sure if all the ifunit() usages are necessary, many of them are
> certainly exposing races with attach/destroy.

No, not _just_ ifunit, but ifunit is one of the places where this is
hit. But just one.
> Taking a rwlock for all ioctl(2) has a big impact.  The 'default' case
> of the switch in ifioctl() goes into other subsystems and per-ifp ioctl
> handler.
>
> Many drivers sleep in their ioctl(2) handler, most USB and wireless one
> to name a few.  Past experience showed that holding a rwlock around all
> the handlers, led to (temporarily) "deadlock".  This is why vio(4) mailbox
> sleep has been turned into a rwsleep releasing the `netlock'.
>
> One can argue that such deadlocks happen because the scope of the lock is
> huge.  This is easy to understand with the `netlock' and questionable,
> at the time of the diff, with the proposed `if_list_lock'.  But saying
> so would miss the point: throwing a lock around a huge part of code to
> make symptoms disappear is a big hammer.

Oh, that's your concern. I understand what you're concerned with now.
However, I think that in light of that, you've misunderstood the
original patch, and I'm now more convinced that the original patch is
the correct route.

The original patchset:
a. Uses an exclusive lock for clone/destroy.
b. Uses a shared lock for all other ioctls.

This means that all ioctls except clone/destroy can run without
blocking each other. So, there's no deadlock issues, and no speed
issues, and no lack of coarseness of locking. What this patch set does
add is:
1. Other ioctls are not permitted to run at the same time as clone/destroy.
2. Clone and destroy are not allowed to run at the same time as each other.
However:
3. Other ioctls ARE allowed to run at the same time as other ioctls,
so no blocking or deadlock issues.

Given the object lifetime and lookup structure design, these seem to
be the optimal set of circumstances.

Jason

Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

[Jason A. Donenfeld]

On Tue, Jun 23, 2020 at 8:21 AM Martin Pieuchot  wrote:
> I'd argue this is a related problem but a different one.  The diff I
> sent serializes cloning/destroying pseudo-interfaces.  It has value on
> its own because *all* if_clone_*() operations are now serialized.
>
> Now you correctly points out that it doesn't fix all the races in the
> ioctl path.  That's a fact.  However without the informations of the
> crashes it is hard to reason about the uncounted reference returned by
> ifunit().
>
> Taking a rwlock around all ioctl(2) can have effects that are hard to
> debug.

We're talking about the same resource and lookup structure, so
generally it makes sense to protect that the same way, right? Adding
and removing ifps, and adding and them and removing them from the list
of ifps, all need to be synchronized with the read-only usage of those
ifps. The other way to fix it would be avoiding a critical section
entirely by incrementing a refcount during the if_list lookup.

Either way, it sounds like the big problem you're pointing out with my
patch is that you fear some of those ioctls need to be callable from
contexts that cannot sleep, which means using an rwlock is wrong. It
doesn't look like the mutex spinlock has a r/w variant though. Or am I
missing that?

Jason

Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

[Jason A. Donenfeld]

On 6/23/20, Martin Pieuchot  wrote:
> On 23/06/20(Tue) 01:00, Jason A. Donenfeld wrote:
>> You can crash a system by running something like:
>>
>> for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig
>> bridge0 destroy& done& done
>>
>> This works with every type of interface I've tried. It appears that
>> if_clone_destroy and if_clone_create race with other ioctls, which
>> causes a variety of different UaFs or just general logic errors.
>
> Thanks for the report.  This is a long standing & known issue.
>
>> One common root cause appears to be that most ifioctl functions use
>> ifunit() to find an interface by name, which traverses if_list. Writes
>> to if_list are protected by a lock, but reads are apparently
>> unprotected. There's also the question of the life time of the object
>> returned from ifunit(). Most things that access 's if_list are
>> done without locking, and even if those accesses were to be locked, the
>> lock would be released before the object is no longer used, causing the
>> UaF in that case as well.
>
> Any sleeping point between the first ifunit() and the end of `ifc_create'
> or `ifc_destroy' respectively can lead to races.
>
>> This patch fixes the issue by making if_clone_{create,destroy} exclusive
>> with all other ifioctls.
>
> Diff below achieves the same but moves the locking inside the if_clone*
> functions such that consumers like tun(4), bridge(4) and switch(4) which
> clone interfaces upon open(2) are also serialized.
>
> I also added a NET_ASSERT_UNLOCKED() in both functions because the new
> lock must be grabbed before the NET_LOCK() in order to allow ifc_create
> and ifc_destroy to manipulate data structures protected by this lock.
>
> Comments, ok?

Not okay. This is the first thing I tried, and it still races with
ifioctl_get, causing UaF crashes. It's harder to trigger, but still
possible after a few minutes of races. This structure here needs to be
a read/write lock, which is what my original patch provides. (I guess
I forgot to add the "ok?" epilogue to it.)


>
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.610
> diff -u -p -r1.610 if.c
> --- net/if.c  22 Jun 2020 09:45:13 -  1.610
> +++ net/if.c  23 Jun 2020 10:25:41 -
> @@ -224,6 +224,7 @@ void  if_idxmap_remove(struct ifnet *);
>
>  TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head);
>
> +struct rwlock if_clone_lock = RWLOCK_INITIALIZER("if_clone_lock");
>  LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners);
>  int if_cloners_count;
>
> @@ -1246,24 +1247,36 @@ if_clone_create(const char *name, int rd
>   struct ifnet *ifp;
>   int unit, ret;
>
> - ifc = if_clone_lookup(name, );
> - if (ifc == NULL)
> - return (EINVAL);
> + NET_ASSERT_UNLOCKED();
> + rw_enter_write(_clone_lock);
>
> - if (ifunit(name) != NULL)
> - return (EEXIST);
> + ifc = if_clone_lookup(name, );
> + if (ifc == NULL) {
> + ret = EINVAL;
> + goto out;
> + }
> + if (ifunit(name) != NULL) {
> + ret = EEXIST;
> + goto out;
> + }
>
>   ret = (*ifc->ifc_create)(ifc, unit);
> + if (ret != 0)
> + goto out;
>
> - if (ret != 0 || (ifp = ifunit(name)) == NULL)
> - return (ret);
> + ifp = ifunit(name);
> + if (ifp == NULL) {
> + ret = EAGAIN;
> + goto out;
> + }
>
>   NET_LOCK();
>   if_addgroup(ifp, ifc->ifc_name);
>   if (rdomain != 0)
>   if_setrdomain(ifp, rdomain);
>   NET_UNLOCK();
> -
> +out:
> + rw_exit_write(_clone_lock);
>   return (ret);
>  }
>
> @@ -1277,16 +1290,23 @@ if_clone_destroy(const char *name)
>   struct ifnet *ifp;
>   int ret;
>
> - ifc = if_clone_lookup(name, NULL);
> - if (ifc == NULL)
> - return (EINVAL);
> + NET_ASSERT_UNLOCKED();
> + rw_enter_write(_clone_lock);
>
> + ifc = if_clone_lookup(name, NULL);
> + if (ifc == NULL) {
> + ret = EINVAL;
> + goto out;
> + }
>   ifp = ifunit(name);
> - if (ifp == NULL)
> - return (ENXIO);
> -
> - if (ifc->ifc_destroy == NULL)
> - return (EOPNOTSUPP);
> + if (ifp == NULL) {
> + ret = ENXIO;
> + goto out;
> + }
> + if (ifc->ifc_destroy == NULL) {
> + ret = EOPNOTSUPP;
> + goto out;
> + }
>
>   NET_LOCK();
>   if (ifp->if_flags & IFF_UP) {
> @@ -1297,7 +1317,8 @@ if_clone_destroy(const char *name)
>   }
>   NET_UNLOCK();
>   ret = (*ifc->ifc_destroy)(ifp);
> -
> +out:
> + rw_exit_write(_clone_lock);
>   return (ret);
>  }
>
>

[PATCH] net: prevent if_clone_destroy from racing with rest of stack

[Jason A. Donenfeld]

You can crash a system by running something like:

for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig bridge0 
destroy& done& done

This works with every type of interface I've tried. It appears that
if_clone_destroy and if_clone_create race with other ioctls, which
causes a variety of different UaFs or just general logic errors.

One common root cause appears to be that most ifioctl functions use
ifunit() to find an interface by name, which traverses if_list. Writes
to if_list are protected by a lock, but reads are apparently
unprotected. There's also the question of the life time of the object
returned from ifunit(). Most things that access 's if_list are
done without locking, and even if those accesses were to be locked, the
lock would be released before the object is no longer used, causing the
UaF in that case as well.

This patch fixes the issue by making if_clone_{create,destroy} exclusive
with all other ifioctls.
---
 sys/net/if.c | 36 +++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git sys/net/if.c sys/net/if.c
index fb2f86f4a7c..9eedea83d32 100644
--- sys/net/if.c
+++ sys/net/if.c
@@ -246,6 +246,11 @@ struct task if_input_task_locked = 
TASK_INITIALIZER(if_netisr, NULL);
  */
 struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
 
+/*
+ * Protect modifications to objects owned by ifnet's if_list
+ */
+struct rwlock iflist_obj_lock = RWLOCK_INITIALIZER("iflist_obj_lock");
+
 /*
  * Network interface utility routines.
  */
@@ -1905,7 +1910,7 @@ if_setrdomain(struct ifnet *ifp, int rdomain)
  * Interface ioctls.
  */
 int
-ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
+ifioctl_unlocked(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
 {
struct ifnet *ifp;
struct ifreq *ifr = (struct ifreq *)data;
@@ -2432,6 +2437,35 @@ ifioctl_get(u_long cmd, caddr_t data)
return (error);
 }
 
+int
+ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
+{
+   int ret;
+
+   switch (cmd) {
+   case SIOCIFCREATE:
+   case SIOCIFDESTROY:
+   rw_enter_write(_obj_lock);
+   break;
+   default:
+   rw_enter_read(_obj_lock);
+   }
+
+   ret = ifioctl_unlocked(so, cmd, data, p);
+
+   switch (cmd) {
+   case SIOCIFCREATE:
+   case SIOCIFDESTROY:
+   rw_exit_write(_obj_lock);
+   break;
+   default:
+   rw_exit_read(_obj_lock);
+   }
+
+   return (ret);
+}
+
+
 static int
 if_sffpage_check(const caddr_t data)
 {
-- 
2.27.0

[PATCH 2/3] wireguard: increase if_txmit to 64

[Jason A. Donenfeld]

This increases throughput by keeping the worker threads active for
longer.
---
 sys/net/if_wg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git sys/net/if_wg.c sys/net/if_wg.c
index e27a575cc1b..5b7550840f8 100644
--- sys/net/if_wg.c
+++ sys/net/if_wg.c
@@ -2651,6 +2651,7 @@ wg_clone_create(struct if_clone *ifc, int unit)
ifp->if_mtu = DEFAULT_MTU;
ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST | IFF_NOARP;
ifp->if_xflags = IFXF_CLONED;
+   ifp->if_txmit = 64; /* Keep our workers active for longer. */
 
ifp->if_ioctl = wg_ioctl;
ifp->if_start = wg_start;
-- 
2.27.0

[PATCH 1/3] conf: build wg(4) with ordinary kernel config

[Jason A. Donenfeld]

This will help us get a bit of testing in the snapshot builds.
---
 sys/conf/GENERIC | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git sys/conf/GENERIC sys/conf/GENERIC
index 34d608cff44..f0eff16d2b8 100644
--- sys/conf/GENERIC
+++ sys/conf/GENERIC
@@ -109,7 +109,7 @@ pseudo-device   vether  # Virtual ethernet
 pseudo-device  vxlan   # Virtual extensible LAN
 pseudo-device  vlan# IEEE 802.1Q VLAN
 pseudo-device  switch  # Switch
-#pseudo-device wg  # WireGuard
+pseudo-device  wg  # WireGuard
 
 pseudo-device  bio 1   # ioctl multiplexing device
 
-- 
2.27.0

[PATCH 3/3] wireguard: mark interface as MPSAFE

[Jason A. Donenfeld]

This enables us to process queueing of different packet queues without
KERNEL_LOCK. Combined with the increase in txlen, this keeps our
encryption workers more active. This results in a ~30% performance
boost.
---
 sys/net/if_wg.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git sys/net/if_wg.c sys/net/if_wg.c
index 5b7550840f8..25f79234d7b 100644
--- sys/net/if_wg.c
+++ sys/net/if_wg.c
@@ -1832,8 +1832,8 @@ wg_queue_out(struct wg_softc *sc, struct wg_peer *peer)
 
/*
 * We delist all staged packets and then add them to the queues. This
-* can race with wg_start when called from wg_send_keepalive, however
-* wg_start will not race as it is serialised.
+* can race with wg_qstart when called from wg_send_keepalive, however
+* wg_qstart will not race as it is serialised.
 */
mq_delist(>p_stage_queue, );
ml_init(_free);
@@ -2067,8 +2067,9 @@ wg_input(void *_sc, struct mbuf *m, struct ip *ip, struct 
ip6_hdr *ip6,
 }
 
 void
-wg_start(struct ifnet *ifp)
+wg_qstart(struct ifqueue *ifq)
 {
+   struct ifnet*ifp = ifq->ifq_if;
struct wg_softc *sc = ifp->if_softc;
struct wg_peer  *peer;
struct wg_tag   *t;
@@ -2079,11 +2080,10 @@ wg_start(struct ifnet *ifp)
 
/*
 * We should be OK to modify p_start_list, p_start_onlist in this
-* function as the interface is not IFXF_MPSAFE and therefore should
-* only be one instance of this function running at a time. These
-* values are not modified anywhere else.
+* function as there should only be one ifp->if_qstart invoked at a
+* time.
 */
-   while ((m = ifq_dequeue(>if_snd)) != NULL) {
+   while ((m = ifq_dequeue(ifq)) != NULL) {
t = wg_tag_get(m);
peer = t->t_peer;
if (mq_push(>p_stage_queue, m) != 0)
@@ -2163,7 +2163,7 @@ wg_output(struct ifnet *ifp, struct mbuf *m, struct 
sockaddr *sa,
 * delayed packet without doing some refcnting. If a peer is removed
 * while a delayed holds a reference, bad things will happen. For the
 * time being, delayed packets are unsupported. This may be fixed with
-* another aip_lookup in wg_start, or refcnting as mentioned before.
+* another aip_lookup in wg_qstart, or refcnting as mentioned before.
 */
if (m->m_pkthdr.pf.delay > 0) {
DPRINTF(sc, "PF Delay Unsupported");
@@ -2178,7 +2178,7 @@ wg_output(struct ifnet *ifp, struct mbuf *m, struct 
sockaddr *sa,
 
/*
 * We still have an issue with ifq that will count a packet that gets
-* dropped in wg_start, or not encrypted. These get counted as
+* dropped in wg_qstart, or not encrypted. These get counted as
 * ofails or oqdrops, so the packet gets counted twice.
 */
return if_enqueue(ifp, m);
@@ -2650,11 +2650,11 @@ wg_clone_create(struct if_clone *ifc, int unit)
 
ifp->if_mtu = DEFAULT_MTU;
ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST | IFF_NOARP;
-   ifp->if_xflags = IFXF_CLONED;
+   ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
ifp->if_txmit = 64; /* Keep our workers active for longer. */
 
ifp->if_ioctl = wg_ioctl;
-   ifp->if_start = wg_start;
+   ifp->if_qstart = wg_qstart;
ifp->if_output = wg_output;
 
ifp->if_type = IFT_WIREGUARD;
-- 
2.27.0

Re: use libc base64 code instead of libcrypt for ifconfig wg key handling

[Jason A. Donenfeld]

On Sun, Jun 21, 2020 at 7:01 PM David Gwynne  wrote:
>
> libc has undocumented base64 encoding and decoding funtionality. this
> cuts ifconfig over to using it instead of the code in libcrypto.
>
> whether the libc functionality should be "blessed" and documented is a
> separate issue.
>
> ok?

OK zx2c4

But, if you really want to get mega-crypto-nerd on this, we could
replace it with the constant time base64 functions from
wireguard-tools (I'm happy to s/gpl/mit/):
https://git.zx2c4.com/wireguard-tools/tree/src/encoding.c key_{to,from}_base64
Bitsliced fixed-length base64 like this prevents potential cache
timing attacks from the usual lookup table-based implementation.
However, by most reasonable measures from most reasonable people, it's
totally overkill.

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

On Mon, Jun 1, 2020 at 2:37 PM Mark Cave-Ayland
 wrote:
> Oh wow it looks great!

Totally. I was super impressed as well.

> I also have commit access to OpenBIOS so I can tidy that up
> and get it posted over on the OpenBIOS mailing list. Probably the main thing 
> is to
> figure out what to do if the specified word doesn't exist. I'll also try and 
> find a
> few mins to fire up my Mac Mini to see if it exists there to work out if it 
> should be
> restricted to SPARC only.
>
> Note that I did my last merge a few days ago so it will be a little while 
> before it
> hits QEMU git master, but I can certainly get it added in time for the next 
> official
> QEMU release.

Oh, whoops, I just sent it to qemu-devel. Feel free to discard that
and commit it to the proper place. Given your forth chops, it makes
sense that you're in fact an OpenBIOS committer. :)

Jason

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

On Mon, Jun 1, 2020 at 2:08 PM Jason A. Donenfeld  wrote:
>
> Hi Mark,
>
> On Mon, Jun 1, 2020 at 1:54 PM Mark Cave-Ayland
>  wrote:
> >
> > On 01/06/2020 08:23, Jason A. Donenfeld wrote:
> >
> > > On Sun, May 31, 2020 at 3:18 AM Mark Cave-Ayland
> > >  wrote:
> > >>
> > >> AFAICT the problem here is the Forth being used at
> > >> https://github.com/openbsd/src/blob/master/sys/arch/sparc64/dev/fb.c#L511:
> > >>  since the
> > >> addr word isn't part of the IEEE-1275 specification, it is currently 
> > >> unimplemented in
> > >> OpenBIOS.
> > >
> > > Actually, it looks to me like after this line runs:
> > >
> > > OF_interpret("stdout @ is my-self "
> > > "addr char-height addr char-width "
> > > "addr window-top addr window-left",
> > > 4, , , , );
> > >
> > > windowleft and windowtop contain legit addresses, but romwidth and
> > > romheight have garbage in them. It might be possible to chalk this up
> > > to bogus QEMU firmware, in which case, whatever.
> >
> > Sadly I think that's more due to luck than anything else. If you have a 
> > working boot
> > loader then can you try booting qemu-system-sparc64 with -prom-env 
> > 'auto-boot?=false'
> > and then entering the following definition of addr at the Forth prompt:
> >
> > : addr
> >   parse-word $find if
> > cell +
> >   then
> > ;
> >
> > followed by:
> >
> > boot
> >
> > That should give you a definition of addr that will return the address of a 
> > value
> > type in Forth.
>
> Wow, that's magic, and works perfectly:
> https://data.zx2c4.com/openbsd-qemu-sparc64-pretty-vga-with-serif-font.png
> Pretty font too.
>
> It sounds like the issue we're facing here is that the addr function
> is missing from QEMU's firmware? Would it be quasi interesting to
> remove use of it from OpenBSD? Or should we take this over to QEMU
> instead and get it implemented?

I just sent a patch up to qemu-devel. We'll see what they think. You
should be CC'd on it.
https://lore.kernel.org/qemu-devel/20200601203143.93424-1-ja...@zx2c4.com/

Last nit would be figuring out how to prevent it from prompting me for
"root device: ", "swap device: ", but I'm guessing that's just some
prom variable or partition label I need to fiddle with, rather than a
real bug worthy of this list.

Jason

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

Hi Mark,

On Mon, Jun 1, 2020 at 1:54 PM Mark Cave-Ayland
 wrote:
>
> On 01/06/2020 08:23, Jason A. Donenfeld wrote:
>
> > On Sun, May 31, 2020 at 3:18 AM Mark Cave-Ayland
> >  wrote:
> >>
> >> AFAICT the problem here is the Forth being used at
> >> https://github.com/openbsd/src/blob/master/sys/arch/sparc64/dev/fb.c#L511: 
> >> since the
> >> addr word isn't part of the IEEE-1275 specification, it is currently 
> >> unimplemented in
> >> OpenBIOS.
> >
> > Actually, it looks to me like after this line runs:
> >
> > OF_interpret("stdout @ is my-self "
> > "addr char-height addr char-width "
> > "addr window-top addr window-left",
> > 4, , , , );
> >
> > windowleft and windowtop contain legit addresses, but romwidth and
> > romheight have garbage in them. It might be possible to chalk this up
> > to bogus QEMU firmware, in which case, whatever.
>
> Sadly I think that's more due to luck than anything else. If you have a 
> working boot
> loader then can you try booting qemu-system-sparc64 with -prom-env 
> 'auto-boot?=false'
> and then entering the following definition of addr at the Forth prompt:
>
> : addr
>   parse-word $find if
> cell +
>   then
> ;
>
> followed by:
>
> boot
>
> That should give you a definition of addr that will return the address of a 
> value
> type in Forth.

Wow, that's magic, and works perfectly:
https://data.zx2c4.com/openbsd-qemu-sparc64-pretty-vga-with-serif-font.png
Pretty font too.

It sounds like the issue we're facing here is that the addr function
is missing from QEMU's firmware? Would it be quasi interesting to
remove use of it from OpenBSD? Or should we take this over to QEMU
instead and get it implemented?

Jason

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

On Sun, May 31, 2020 at 7:22 AM Otto Moerbeek  wrote:
> Thanks, the following works indeed.
>
> -Otto
>
> Index: bootblk.fth
> ===
> RCS file: /cvs/src/sys/arch/sparc64/stand/bootblk/bootblk.fth,v
> retrieving revision 1.9
> diff -u -p -r1.9 bootblk.fth
> --- bootblk.fth 2 Apr 2020 06:06:22 -   1.9
> +++ bootblk.fth 31 May 2020 13:17:25 -
> @@ -716,7 +716,8 @@ create cur-blockno -1 l, -1 l,  \ Curren
>  2drop
>  ;
>
> -" load-base " evaluate constant loader-base
> +\\ Do not overwrite bootblocks
> +" load-base " evaluate 2000 + constant loader-base
>
>  : load-file-signon ( load-file len boot-path len -- load-file len boot-path 
> len )
> ." Loading file" space 2over type cr ." from device" space 2dup type cr

I can confirm that this enabled -current to boot now perfectly, so in
case it helps:

OK zx2c4@

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

On Sun, May 31, 2020 at 3:18 AM Mark Cave-Ayland
 wrote:
>
> AFAICT the problem here is the Forth being used at
> https://github.com/openbsd/src/blob/master/sys/arch/sparc64/dev/fb.c#L511: 
> since the
> addr word isn't part of the IEEE-1275 specification, it is currently 
> unimplemented in
> OpenBIOS.

Actually, it looks to me like after this line runs:

OF_interpret("stdout @ is my-self "
"addr char-height addr char-width "
"addr window-top addr window-left",
4, , , , );

windowleft and windowtop contain legit addresses, but romwidth and
romheight have garbage in them. It might be possible to chalk this up
to bogus QEMU firmware, in which case, whatever.

Jason

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

On Fri, May 29, 2020 at 4:56 PM Jason A. Donenfeld  wrote:
>
> Oh that's a nice observation about `boot disk -V`. Doing so actually
> got me booting up entirely:
>
> $ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2
> $ qemu-img resize disk.qcow2 20G
> $ qemu-system-sparc64 -m 1024 -drive file=disk.qcow2,if=ide -net
> nic,model=ne2k_pci -net user -boot a -nographic -monitor none -serial
> stdio
> OpenBIOS for Sparc64
> Configuration device id QEMU version 1 machine id 0
> kernel cmdline
> CPUs: 1 x SUNW,UltraSPARC-IIi
> UUID: ----
> Welcome to OpenBIOS v1.1 built on Oct 28 2019 17:08
>   Type 'help' for detailed information
> Trying /obio/SUNW,fdtwo...
> No valid state has been set by load or init-program
>
> 0 > boot disk -V Not a Linux kernel image
> Not a bootable ELF image
> Not a bootable a.out image
>
> Loading FCode image...
> Loaded 6882 bytes
> entry point is 0x4000
> Evaluating FCode...
> OpenBSD IEEE 1275 Bootblock 2.0
> Booting from device /pci@1fe,0/pci@1,1/ide@3/ide@0/disk@0
> Try superblock read
> FFS v1
> ufs-open complete
> .Looking for ofwboot in directory...
> .
> ..
> ofwboot
> Found it
> .Loading 1a0e0  bytes of file...
> Copying 2000 bytes to 4000
> Copying 2000 bytes to 6000
> Copying 2000 bytes to 8000
> Copying 2000 bytes to a000
> Copying 2000 bytes to c000
> Copying 2000 bytes to e000
> Copying 2000 bytes to 1
> Copying 2000 bytes to 12000
> Copying 2000 bytes to 14000
> Copying 2000 bytes to 16000
> Copying 2000 bytes to 18000
> Copying 2000 bytes to 1a000
> Copying 2000 bytes to 1c000
> Copying 2000 bytes to 1e000
> reserved fcode word.
> >> OpenBSD BOOT 1.17
> Trying bsd...
> open /etc/random.seed: No such file or directory
> Booting /pci@1fe,0/pci@1,1/ide@3/ide@0/disk@0/bsd
> 4211776@0x100+7104@0x1404440+3247928@0x1c0+946376@0x1f18f38
> symbols @ 0xfef50340 139 start=0x100
> console is /pci@1fe,0/pci@1,1/ebus@1/su
> Copyright (c) 1982, 1986, 1989, 1991, 1993
> The Regents of the University of California.  All rights reserved.
> Copyright (c) 1995-2020 OpenBSD. All rights reserved.  https://www.OpenBSD.org
>
> OpenBSD 6.7 (RAMDISK) #298: Thu May  7 19:11:18 MDT 2020
> dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/RAMDISK
> real mem = 1073741824 (1024MB)
> avail mem = 1044135936 (995MB)
> unknown option `V'
> mainbus0 at root: OpenBiosTeam,OpenBIOS
> cpu0 at mainbus0: SUNW,UltraSPARC-IIi (rev 9.1) @ 100 MHz
> cpu0: physical 256K instruction (64 b/l), 16K data (32 b/l), 256K
> external (64 b/l)
> psycho0 at mainbus0: SUNW,sabre, impl 0, version 0, ign 7c0
> psycho0: bus range 0-2, PCI bus 0
> psycho0: dvma map c000-dfff
> pci0 at psycho0
> ppb0 at pci0 dev 1 function 1 "Sun Simba" rev 0x11
> pci1 at ppb0 bus 1
> ebus0 at pci1 dev 1 function 0 "Sun PCIO EBus2" rev 0x01
> clock1 at ebus0 addr 2000-3fff: mk48t59
> "power" at ebus0 addr 7240-7243 ivec 0x1 not configured
> "fdthree" at ebus0 addr 0- not configured
> com0 at ebus0 addr 3f8-3ff ivec 0x2b: ns16550a, 16 byte fifo
> com0: console
> pckbc0 at ebus0 addr 60-67 ivec 0x29
> pckbd0 at pckbc0 (kbd slot)
> wskbd0 at pckbd0
> "Bochs VGA" rev 0x02 at pci1 dev 2 function 0 not configured
> pciide0 at pci1 dev 3 function 0 "CMD Technology PCI0646" rev 0x07:
> DMA, channel 0 configured to native-PCI, channel 1 configured to
> native-PCI
> pciide0: using ivec 0x7e0 for native-PCI interrupt
> wd0 at pciide0 channel 0 drive 0: 
> wd0: 16-sector PIO, LBA48, 20480MB, 41943040 sectors
> wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2
> atapiscsi0 at pciide0 channel 1 drive 0
> scsibus0 at atapiscsi0: 2 targets
> cd0 at scsibus0 targ 0 lun 0:  removable
> cd0(pciide0:1:0): using PIO mode 4, Ultra-DMA mode 2
> ppb1 at pci0 dev 1 function 0 "Sun Simba" rev 0x11
> pci2 at ppb1 bus 2
> ne0 at pci2 dev 0 function 0 "Realtek 8029" rev 0x00: ivec 0x7d0,
> address 52:54:00:12:34:56
> softraid0 at root
> scsibus1 at softraid0: 256 targets
> bootpath: /pci@1fe,0/pci@1,1/ide@3,0/ide@0,0/disk@0,0
> root on rd0a swap on rd0b dump on rd0b
> WARNING: CHECK AND RESET THE DATE!
> erase ^?, werase ^W, kill ^U, intr ^C, status ^T
>
> Welcome to the OpenBSD/sparc64 6.7 installation program.
> (I)nstall, (U)pgrade, (A)utoinstall or (S)hell?

Actually, it looks like the `disk debug -V` trick works for 6.7 but
fails for -current:

snapshots/miniroot67.img

Loading FCode image...
Loaded 6882 bytes
entry point is 0x4000
Evaluating FCode...
OpenBSD IEEE 1275 Bootblock 2.0
Booting from device /pci@1fe,0/pci@1,1/ide@3/ide@0/di

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

Note that you need to run this with `-nographic`, because the kernel
crashes when trying to use vgafb on sparc64/qemu. I've witnessed two
varieties crashes:

- https://data.zx2c4.com/openbsd-6.7-sparc64-vga-panic-miniroot67.png
This happens when booting up miniroot67.fs

- https://data.zx2c4.com/openbsd-6.7-sparc64-vga-panic-after-installation.png
This happens after installation openbsd onto disk properly, and then
booting up into it.

Passing `-nographic` prevents these from happening, since vgafb doesn't
bind to anything.

I don't have a bsd.gdb in order to addr2line this, but if the miniroot
panic is related to the normal panic, and we then assume alignment
issues in fb_get_console_metrics, then I wonder if the below patch would
make a difference. On the other hand, a "data access fault" makes it
seem more likely that OF_interpret is just getting bogus addresses from
buggy qemu firmware.

I probably have another two hours to go in waiting for this thing to
build...

Jason

--- a/sys/arch/sparc64/dev/fb.c
+++ b/sys/arch/sparc64/dev/fb.c
@@ -507,6 +507,7 @@ int
 fb_get_console_metrics(int *fontwidth, int *fontheight, int *wtop, int *wleft)
 {
cell_t romheight, romwidth, windowtop, windowleft;
+   uint64_t romheight_64, romwidth_64, windowtop_64, windowleft_64;

/*
 * Get the PROM font metrics and address
@@ -520,10 +521,15 @@ fb_get_console_metrics(int *fontwidth, int *fontheight, 
int *wtop, int *wleft)
windowtop == 0 || windowleft == 0)
return (1);

-   *fontwidth = (int)*(uint64_t *)romwidth;
-   *fontheight = (int)*(uint64_t *)romheight;
-   *wtop = (int)*(uint64_t *)windowtop;
-   *wleft = (int)*(uint64_t *)windowleft;
+   memcpy(_64, (void *)romheight, sizeof(romheight_64));
+   memcpy(_64, (void *)romwidth, sizeof(romwidth_64));
+   memcpy(_64, (void *)windowtop, sizeof(windowtop_64));
+   memcpy(_64, (void *)windowleft, sizeof(windowleft_64));
+
+   *fontwidth = (int)romwidth_64;
+   *fontheight = (int)romheight_64;
+   *wtop = (int)windowtop_64;
+   *wleft = (int)windowleft_64;

return (0);
 }

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

On Fri, May 29, 2020 at 4:56 PM Jason A. Donenfeld  wrote:
>
> Oh that's a nice observation about `boot disk -V`. Doing so actually
> got me booting up entirely:
>
> $ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2

Er, copy and paste error. The below is actually from miniroot67.fs.

Re: sparc64 boot issue on qemu

[Jason A. Donenfeld]

Oh that's a nice observation about `boot disk -V`. Doing so actually
got me booting up entirely:

$ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2
$ qemu-img resize disk.qcow2 20G
$ qemu-system-sparc64 -m 1024 -drive file=disk.qcow2,if=ide -net
nic,model=ne2k_pci -net user -boot a -nographic -monitor none -serial
stdio
OpenBIOS for Sparc64
Configuration device id QEMU version 1 machine id 0
kernel cmdline
CPUs: 1 x SUNW,UltraSPARC-IIi
UUID: ----
Welcome to OpenBIOS v1.1 built on Oct 28 2019 17:08
  Type 'help' for detailed information
Trying /obio/SUNW,fdtwo...
No valid state has been set by load or init-program

0 > boot disk -V Not a Linux kernel image
Not a bootable ELF image
Not a bootable a.out image

Loading FCode image...
Loaded 6882 bytes
entry point is 0x4000
Evaluating FCode...
OpenBSD IEEE 1275 Bootblock 2.0
Booting from device /pci@1fe,0/pci@1,1/ide@3/ide@0/disk@0
Try superblock read
FFS v1
ufs-open complete
.Looking for ofwboot in directory...
.
..
ofwboot
Found it
.Loading 1a0e0  bytes of file...
Copying 2000 bytes to 4000
Copying 2000 bytes to 6000
Copying 2000 bytes to 8000
Copying 2000 bytes to a000
Copying 2000 bytes to c000
Copying 2000 bytes to e000
Copying 2000 bytes to 1
Copying 2000 bytes to 12000
Copying 2000 bytes to 14000
Copying 2000 bytes to 16000
Copying 2000 bytes to 18000
Copying 2000 bytes to 1a000
Copying 2000 bytes to 1c000
Copying 2000 bytes to 1e000
reserved fcode word.
>> OpenBSD BOOT 1.17
Trying bsd...
open /etc/random.seed: No such file or directory
Booting /pci@1fe,0/pci@1,1/ide@3/ide@0/disk@0/bsd
4211776@0x100+7104@0x1404440+3247928@0x1c0+946376@0x1f18f38
symbols @ 0xfef50340 139 start=0x100
console is /pci@1fe,0/pci@1,1/ebus@1/su
Copyright (c) 1982, 1986, 1989, 1991, 1993
The Regents of the University of California.  All rights reserved.
Copyright (c) 1995-2020 OpenBSD. All rights reserved.  https://www.OpenBSD.org

OpenBSD 6.7 (RAMDISK) #298: Thu May  7 19:11:18 MDT 2020
dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/RAMDISK
real mem = 1073741824 (1024MB)
avail mem = 1044135936 (995MB)
unknown option `V'
mainbus0 at root: OpenBiosTeam,OpenBIOS
cpu0 at mainbus0: SUNW,UltraSPARC-IIi (rev 9.1) @ 100 MHz
cpu0: physical 256K instruction (64 b/l), 16K data (32 b/l), 256K
external (64 b/l)
psycho0 at mainbus0: SUNW,sabre, impl 0, version 0, ign 7c0
psycho0: bus range 0-2, PCI bus 0
psycho0: dvma map c000-dfff
pci0 at psycho0
ppb0 at pci0 dev 1 function 1 "Sun Simba" rev 0x11
pci1 at ppb0 bus 1
ebus0 at pci1 dev 1 function 0 "Sun PCIO EBus2" rev 0x01
clock1 at ebus0 addr 2000-3fff: mk48t59
"power" at ebus0 addr 7240-7243 ivec 0x1 not configured
"fdthree" at ebus0 addr 0- not configured
com0 at ebus0 addr 3f8-3ff ivec 0x2b: ns16550a, 16 byte fifo
com0: console
pckbc0 at ebus0 addr 60-67 ivec 0x29
pckbd0 at pckbc0 (kbd slot)
wskbd0 at pckbd0
"Bochs VGA" rev 0x02 at pci1 dev 2 function 0 not configured
pciide0 at pci1 dev 3 function 0 "CMD Technology PCI0646" rev 0x07:
DMA, channel 0 configured to native-PCI, channel 1 configured to
native-PCI
pciide0: using ivec 0x7e0 for native-PCI interrupt
wd0 at pciide0 channel 0 drive 0: 
wd0: 16-sector PIO, LBA48, 20480MB, 41943040 sectors
wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2
atapiscsi0 at pciide0 channel 1 drive 0
scsibus0 at atapiscsi0: 2 targets
cd0 at scsibus0 targ 0 lun 0:  removable
cd0(pciide0:1:0): using PIO mode 4, Ultra-DMA mode 2
ppb1 at pci0 dev 1 function 0 "Sun Simba" rev 0x11
pci2 at ppb1 bus 2
ne0 at pci2 dev 0 function 0 "Realtek 8029" rev 0x00: ivec 0x7d0,
address 52:54:00:12:34:56
softraid0 at root
scsibus1 at softraid0: 256 targets
bootpath: /pci@1fe,0/pci@1,1/ide@3,0/ide@0,0/disk@0,0
root on rd0a swap on rd0b dump on rd0b
WARNING: CHECK AND RESET THE DATE!
erase ^?, werase ^W, kill ^U, intr ^C, status ^T

Welcome to the OpenBSD/sparc64 6.7 installation program.
(I)nstall, (U)pgrade, (A)utoinstall or (S)hell?

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

On Thu, May 28, 2020 at 1:19 AM Otto Moerbeek  wrote:
> Of course.., I was running it from a !wxallowed mount. BTW, qemu is in
> packages, no need to build it yourself.

Sure, but now I've been somewhat nerd sniped and am playing with this
fcode forth implementation in qemu :-P. I wonder if there's something
missing in the 64-bit extensions to IEEE 1275, in table.fs...

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

On Thu, May 28, 2020 at 12:15 AM Otto Moerbeek  wrote:
>
> On Wed, May 27, 2020 at 11:28:09PM -0600, Jason A. Donenfeld wrote:
>
> > Hi Otto,
> >
> > On Wed, May 27, 2020 at 4:07 AM Otto Moerbeek  wrote:
> > > Although I'm not terribly interested in bugs that are only seen (s0
> > > far) using emulation, please send me the details on how you set up
> > > qemu.
> >
> > Right, it could very well be a TCG bug. But maybe not. Here's how to
> > get things [not-]working:
> >
> > $ qemu-system-sparc64 --version
> > QEMU emulator version 5.0.0
> > $ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2
> > $ qemu-img resize disk.qcow2 20G
> > $ qemu-system-sparc64 \
> >         -machine sun4u \
> >         -m 1024 \
> >         -drive file=disk.qcow2,if=ide \
> >         -net nic,model=ne2k_pci -net user \
> >         -nographic -serial stdio -monitor none \
> >         -boot c
> >
> > OpenBIOS for Sparc64
> > [...]
> > Loading FCode image...
> > Loaded 5840 bytes
> > entry point is 0x4000
> > Evaluating FCode...
> > OpenBSD IEEE 1275 Bootblock 1.4
> > ..>> OpenBSD BOOT 1.14
> > Trying bsd...
> > [...]
> > OpenBSD 6.6 (RAMDISK) #84: Sat Oct 12 10:42:12 MDT 2019
> >    dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/RAMDISK
> > [...]
> > Welcome to the OpenBSD/sparc64 6.6 installation program.
> > (I)nstall, (U)pgrade, (A)utoinstall or (S)hell?
> >
> > If you swap out miniroot66.fs for miniroot67.fs, you'll get the error
> > I sent prior.
> >
> > Jason
> >
>
> Does not work for me, error message on OpenBSD/amd64:
>
> Could not allocate dynamic translator buffer
>
> ktrace snippet:
>
> 74960 qemu-system-spar CALL  
> mmap(0,0x4000,0x7 EC>,0x1002,-1,0)
> 74960 qemu-system-spar RET   mmap -1 errno 91 Not supported
>
> It's doing a RWX mapping, won't fly on OpenBSD.
>
>         -Otto

This sequence worked fine on my OpenBSD box for reproducing the maybe-bug. 
(See: mount option.) YMMV:

bart ~ # pkg_add git gmake glib2 bison sdl2 gsed bash xz
[...]
bart ~ # ftp -o - https://download.qemu.org/qemu-5.0.0.tar.xz | unxz | tar xf -
bart ~ # cd qemu-5.0.0/
bart ~/qemu-5.0.0 # mkdir build && cd build
bart ~/qemu-5.0.0/build # ../configure && gmake -j$(sysctl -n hw.ncpu)
[...]
bart ~/qemu-5.0.0/build # ftp 
https://cdn.openbsd.org/pub/OpenBSD/6.7/sparc64/miniroot67.fs
[...]
bart ~/qemu-5.0.0/build # ./qemu-img convert -O qcow2 miniroot67.fs disk.qcow2
bart ~/qemu-5.0.0/build # ./qemu-img resize disk.qcow2 20G
Image resized.
bart ~/qemu-5.0.0/build # mount
/dev/sd0a on / type ffs (local, wxallowed)
bart ~/qemu-5.0.0/build # ./sparc64-softmmu/qemu-system-sparc64 -machine sun4u 
-m 1024 -drive file=disk.qcow2,if=ide -net nic,model=ne2k_pci -net user 
-nographic -serial stdio -monitor none -boot c
OpenBIOS for Sparc64
Configuration device id QEMU version 1 machine id 0
kernel cmdline 
CPUs: 1 x SUNW,UltraSPARC-IIi
UUID: ----
Welcome to OpenBIOS v1.1 built on Oct 28 2019 17:08
  Type 'help' for detailed information
Trying disk:a...
Not a bootable ELF image
Not a bootable a.out image
Loading FCode image...
Loaded 6882 bytes
entry point is 0x4000
Evaluating FCode...
OpenBSD IEEE 1275 Bootblock 2.0
..reserved fcode word.
Unhandled Exception 0x0030
PC = 0xffd0f3ac NPC = 0xffd0f3b0
Stopping execution

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hi Otto,

On Wed, May 27, 2020 at 4:07 AM Otto Moerbeek  wrote:
> Although I'm not terribly interested in bugs that are only seen (s0
> far) using emulation, please send me the details on how you set up
> qemu.

Right, it could very well be a TCG bug. But maybe not. Here's how to
get things [not-]working:

$ qemu-system-sparc64 --version
QEMU emulator version 5.0.0
$ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2
$ qemu-img resize disk.qcow2 20G
$ qemu-system-sparc64 \
-machine sun4u \
-m 1024 \
-drive file=disk.qcow2,if=ide \
-net nic,model=ne2k_pci -net user \
-nographic -serial stdio -monitor none \
-boot c

OpenBIOS for Sparc64
[...]
Loading FCode image...
Loaded 5840 bytes
entry point is 0x4000
Evaluating FCode...
OpenBSD IEEE 1275 Bootblock 1.4
..>> OpenBSD BOOT 1.14
Trying bsd...
[...]
OpenBSD 6.6 (RAMDISK) #84: Sat Oct 12 10:42:12 MDT 2019
   dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/RAMDISK
[...]
Welcome to the OpenBSD/sparc64 6.6 installation program.
(I)nstall, (U)pgrade, (A)utoinstall or (S)hell?

If you swap out miniroot66.fs for miniroot67.fs, you'll get the error
I sent prior.

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

On Wed, May 27, 2020 at 2:12 AM Jason A. Donenfeld  wrote:
>
> Hi again Klemens,
>
> On Tue, May 26, 2020 at 5:42 PM Jason A. Donenfeld  wrote:
> >
> > On Tue, May 26, 2020 at 4:52 PM Jason A. Donenfeld  wrote:
> > > With regards to your crash, though, that's a bit more puzzling, and
> > > I'd be interested to learn more details. Because these structs are
> > > already naturally aligned, the __packed attribute, even with the odd
> > > nesting Matt had prior, should have produced all entirely aligned
> > > accesses. That makes me think your kaboom was coming from someplace
> > > else. One possibility is that you were running the git tree on the two
> > > days that I was playing with uint128_t, only to find out that some of
> > > openbsd's patches to clang miscalculate stack sizes when they're in
> > > use, so that work was shelved for another day and the commits removed;
> > > perhaps you were just unlucky? Or you hit some other bug that's
> > > lurking. Either way, output from ddb's `bt` would at least be useful.
> >
> > Do you know off hand if we're able to assume any type of alignment
> > with mbuf->m_data? mtod just casts without any address fixup, which
> > means if mbuf->m_data isn't aligned by some other mechanism, we're in
> > trouble. But I would assume there _is_ some alignment imposed, since
> > the rest of the stack appears to parse tcp headers and such directly
> > without byte-by-byte copies being made.
>
> After a day of TCG-run compilation, I've got a working sparc64 setup
> and can confirm the issue. Working on a fix.

The latest git master should work well now:

solar# arch -s
sparc64
solar# wg-quick up demo
[#] ifconfig wg0 create
[#] wg setconf wg0 /dev/fd/63
[#] ifconfig wg0 inet 192.168.4.155/24 alias
[#] ifconfig wg0 mtu 1420
[#] ifconfig wg0 up
[#] cp /etc/resolv.conf /etc/resolv.conf.wg-quick-backup.demo
[#] printf nameserver %s\n 8.8.8.8 8.8.4.4 1.1.1.1 1.0.0.1
[#] route -q -n add -inet 0.0.0.0/1 -iface 192.168.4.155
[#] route -q -n add -inet 128.0.0.0/1 -iface 192.168.4.155
[#] route -q -n delete -inet 163.172.161.0
[#] route -q -n add -inet 163.172.161.0 -gateway 10.0.2.2
[+] Backgrounding route monitor
solar# curl zx2c4.com/ip
163.172.161.0
demo.wireguard.com

One interesting quirk in doing this on qemu is that the 6.7 and
-current kernel both crash:

Loading FCode image...
Loaded 6882 bytes
entry point is 0x4000
Evaluating FCode...
OpenBSD IEEE 1275 Bootblock 2.0
Unhandled Exception 0x0030
PC = 0xffd0f3ac NPC = 0xffd0f3b0
Stopping execution

Luckily it works fine on 6.6, so that's where I debugged this issue.
But this might be a bug worth looking into. Otto's recent bootblk
patch is a possible culprit, so I've CC'd him.

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hey David,

On Wed, May 27, 2020 at 2:26 AM David Gwynne  wrote:
>
> On Tue, May 26, 2020 at 05:42:13PM -0600, Jason A. Donenfeld wrote:
> > On Tue, May 26, 2020 at 4:52 PM Jason A. Donenfeld  wrote:
> > > With regards to your crash, though, that's a bit more puzzling, and
> > > I'd be interested to learn more details. Because these structs are
> > > already naturally aligned, the __packed attribute, even with the odd
> > > nesting Matt had prior, should have produced all entirely aligned
> > > accesses. That makes me think your kaboom was coming from someplace
> > > else. One possibility is that you were running the git tree on the two
> > > days that I was playing with uint128_t, only to find out that some of
> > > openbsd's patches to clang miscalculate stack sizes when they're in
> > > use, so that work was shelved for another day and the commits removed;
> > > perhaps you were just unlucky? Or you hit some other bug that's
> > > lurking. Either way, output from ddb's `bt` would at least be useful.
>
> When you say clang patches miscalculate the stack size with uint128_t,
> do you know which one(s) specifically? Was it -msave-args?

Yep, exactly. It looked to me like that plugin was considering each
argument of a function to be register-sized, which obviously isn't the
case for uint128_t. I had planned to come back to that after the
wireguard stuff is finished.

> Anyway, tl;dr, my guess is you're reading a 64bit value out of a packet,
> but I'm pretty sure the stack probably only provides 32bit alignment.

Yep, that seems to be exactly the case. Earlier today I looked at
every struct passed to mtod for the entire kernel, and found two other
drivers that do this with 64bit types, but maybe they're in contexts
where that's okay somehow.

Now that I've got my sparc64 rig cooking away at these kernels, I can
confirm that this is indeed the case, and it's trivially fixed by just
memcpy'ing to/from a properly variable on the stack. In my experience,
this pattern,

uint64_t i;
memcpy(, somecharbuffer, sizeof(i));
return i;

optimizes to just the simple boring cast on architectures with fast
unaligned access (e.g. amd64), while doing the smart thing for
architectures that trap or have a performance penalty. IOW, the
compiler generates optimal code over that pattern, so that's what I've
done to work around the issue here.

> - The network stack accesses bits of packets directly. I'm pretty
> sure so far this is limited to 32bit word accesses for things like
> addresses in IPv4 headers, GRE protocol fields, etc. I'm not sure
> there are (currently) any 64bit accesses.

My review earlier today indeed showed all max 32bit, except for two drivers:
- sys/net/switchofp.c and sys/net/ofp.h
- sys/dev/usb/if_athn_usb.c and sys/dev/usb/if_athn_usb.h

Either those are incorrect, are limited to archs that don't trap, or
are used in contexts where other factors make them safe. But beyond
those two, I couldn't find any others.

> Sorry for the rambling. Hopefully you know a bit more about mbuf
> and network stack alignment now though.

Not a problem at all, super appreciated. Looking forward to your other
comments on the driver too.

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hi again Klemens,

On Tue, May 26, 2020 at 5:42 PM Jason A. Donenfeld  wrote:
>
> On Tue, May 26, 2020 at 4:52 PM Jason A. Donenfeld  wrote:
> > With regards to your crash, though, that's a bit more puzzling, and
> > I'd be interested to learn more details. Because these structs are
> > already naturally aligned, the __packed attribute, even with the odd
> > nesting Matt had prior, should have produced all entirely aligned
> > accesses. That makes me think your kaboom was coming from someplace
> > else. One possibility is that you were running the git tree on the two
> > days that I was playing with uint128_t, only to find out that some of
> > openbsd's patches to clang miscalculate stack sizes when they're in
> > use, so that work was shelved for another day and the commits removed;
> > perhaps you were just unlucky? Or you hit some other bug that's
> > lurking. Either way, output from ddb's `bt` would at least be useful.
>
> Do you know off hand if we're able to assume any type of alignment
> with mbuf->m_data? mtod just casts without any address fixup, which
> means if mbuf->m_data isn't aligned by some other mechanism, we're in
> trouble. But I would assume there _is_ some alignment imposed, since
> the rest of the stack appears to parse tcp headers and such directly
> without byte-by-byte copies being made.

After a day of TCG-run compilation, I've got a working sparc64 setup
and can confirm the issue. Working on a fix.

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hi Martin,

To answer a few but not all of your questions:

On Wed, May 27, 2020 at 1:34 AM Martin Pieuchot  wrote:
> First question is, is it possible to use the wg(4) interface without a
> port?

No, that is not how WireGuard works. For details on the actual
protocol particulars, please see
https://www.wireguard.com/papers/wireguard.pdf . Our rev 1 submission
has links to further links as well in the cover letter.

> Aren't wg_noise.c and wg_cookie.c meant to be used by if_wg.c only?  Or
> would other parts of the kernel benefit from them?  If wg(4) is the only
> user I'm questionnings whether putting them in crypto/ is the best choice.

Yes, they should only be used by if_wg.c. Moving them closer to
if_wg.c would make sense imo.

> Is IFT_WIREGUARD needed?  Isn't the interface a point-to-point interface?

It is a point to point interface, but it also has some important
traits that point to point interfaces do not have. In this case, that
means disabling automatically generated v6 ip addresses, which
wireguard explicitly does not support. (It does support manually
generated and cryptographically bound link local v6 addresses,
however.)

> Why did you re-implemented a routing table?  Did you consider storing
> the information you need in existing data structures?

WireGuard's cryptokey routing is a different structure with different
semantics and is intended to work separately from the normal system
routing table. Trying to merge the two is a recipe for disaster. Paper
has details.

> My overall impression is that this can be simplified and integrate
> better in the kernel :)  If you could explain the basic architecture or
> point me where you explained it, it would help.

See paper above. Also see links on cover letter of rev 1 submission.

Regards,
Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

On Tue, May 26, 2020 at 4:52 PM Jason A. Donenfeld  wrote:
> With regards to your crash, though, that's a bit more puzzling, and
> I'd be interested to learn more details. Because these structs are
> already naturally aligned, the __packed attribute, even with the odd
> nesting Matt had prior, should have produced all entirely aligned
> accesses. That makes me think your kaboom was coming from someplace
> else. One possibility is that you were running the git tree on the two
> days that I was playing with uint128_t, only to find out that some of
> openbsd's patches to clang miscalculate stack sizes when they're in
> use, so that work was shelved for another day and the commits removed;
> perhaps you were just unlucky? Or you hit some other bug that's
> lurking. Either way, output from ddb's `bt` would at least be useful.

Do you know off hand if we're able to assume any type of alignment
with mbuf->m_data? mtod just casts without any address fixup, which
means if mbuf->m_data isn't aligned by some other mechanism, we're in
trouble. But I would assume there _is_ some alignment imposed, since
the rest of the stack appears to parse tcp headers and such directly
without byte-by-byte copies being made.

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hey Klemens, Theo,

On Tue, May 26, 2020 at 2:38 PM Klemens Nanni  wrote:
>
> On Tue, May 26, 2020 at 02:23:06PM -0600, Jason A. Donenfeld wrote:
> > That's good news that it's working for you now, but I didn't change
> > anything within the last 24 hours (you mentioned "yesterday") that
> > would seem related to this. So perhaps don't get too excited just yet.
> I was surprised, too.  Perhaps something went wrong applying all those
> single patches from the wireguard-openbsd resository, but if so, I'd
> doubt that it would build in the first place...
>
> So far the interface is stable, but I keep testing.  When I find the
> time, I'll also take a look at the pieces of code which blew up earlier.

My audit is complete, and every single usage of __packed has been
removed. Those never should have been there in the first place.
WireGuard was specifically designed for kernels, and I distinctly
remember making sure things would be high performance on little mips
devices, which necessitated natural struct alignment. And indeed, all
of the structs that wireguard uses on the wire have fields that are
always aligned to each field's size, so no __packed is ever necessary.
This all has been fixed up in the git repo now.

With regards to your crash, though, that's a bit more puzzling, and
I'd be interested to learn more details. Because these structs are
already naturally aligned, the __packed attribute, even with the odd
nesting Matt had prior, should have produced all entirely aligned
accesses. That makes me think your kaboom was coming from someplace
else. One possibility is that you were running the git tree on the two
days that I was playing with uint128_t, only to find out that some of
openbsd's patches to clang miscalculate stack sizes when they're in
use, so that work was shelved for another day and the commits removed;
perhaps you were just unlucky? Or you hit some other bug that's
lurking. Either way, output from ddb's `bt` would at least be useful.

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

On Tue, May 26, 2020 at 2:33 PM Theo de Raadt  wrote:
>
> Jason A. Donenfeld  wrote:
>
> > Hey Klemens,
> >
> > On Tue, May 26, 2020 at 9:13 AM Klemens Nanni  wrote:
> > > I worked with the patches from the wireguard-openbsd repository after
> > > version one of this diff on tech@ became a bit old.
> > >
> > > That was until yesterday;  the kernel would panic due to memory
> > > alignment issues in various spots, `amd64# ping6 tunnel-ip.sparc64'
> > > would receive a single ICMP6 echo reply from the sparc64 peer before
> > > panicing its kernel very early in noise init code.
> > >
> > > I've had fixed a few bugs and ran into different panics during later
> > > code paths.
> > >
> > > Fortunately, using "rev. 2" of your diff on top of a clean checkout just
> > > works so far on sparc64 without any additional patches required - this
> > > is a very pleasnt suprise and I can start looking at it under production
> > > use cases more thoroughly now.
> >
> > That's good news that it's working for you now, but I didn't change
> > anything within the last 24 hours (you mentioned "yesterday") that
> > would seem related to this. So perhaps don't get too excited just yet.
> > I've got a decent MIPS setup here and a hacked up qemu for having a
> > bit more detail on alignment traps. I'll see if I can reproduce any
> > issues.
>
> I suspect problems around forcing __packed.
>
> __packed is very dangerous.
>
> If you know the sizes of the objects are correctly sized and placed to
> not have gap-pads, then not using __packed might be better.

Right, most times, uses of packed are wrong and can be avoided. I'll
work through that all and see what can be minimized. In a cursory look
I did see code to the effect of `struct { u32 a; u64 b; } __packed`,
which could trap.

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hey Klemens,

On Tue, May 26, 2020 at 9:13 AM Klemens Nanni  wrote:
> I worked with the patches from the wireguard-openbsd repository after
> version one of this diff on tech@ became a bit old.
>
> That was until yesterday;  the kernel would panic due to memory
> alignment issues in various spots, `amd64# ping6 tunnel-ip.sparc64'
> would receive a single ICMP6 echo reply from the sparc64 peer before
> panicing its kernel very early in noise init code.
>
> I've had fixed a few bugs and ran into different panics during later
> code paths.
>
> Fortunately, using "rev. 2" of your diff on top of a clean checkout just
> works so far on sparc64 without any additional patches required - this
> is a very pleasnt suprise and I can start looking at it under production
> use cases more thoroughly now.

That's good news that it's working for you now, but I didn't change
anything within the last 24 hours (you mentioned "yesterday") that
would seem related to this. So perhaps don't get too excited just yet.
I've got a decent MIPS setup here and a hacked up qemu for having a
bit more detail on alignment traps. I'll see if I can reproduce any
issues.

Jason

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hey Tobias,

On Tue, May 26, 2020 at 5:28 AM Tobias Heider  wrote:
> > + if (((SIZE_MAX - size) / sizeof(struct wg_aip_io)) < sc->sc_aip_num)
> > + goto error;
>
> I still think those two should return an error.  'goto error' is misleading as
> it doesn't actually set ret != 0.  'error' should probably be renamed
> to 'cleanup' to avoid confusion and ret should be set to ERANGE .

I'll change the label name to be more clear; I agree "error" is
misleading. Returning ERANGE wouldn't be correct though; we still want
the struct to be copied back to the user so that they can use the
information in it to optionally try again (optionally, because maybe
they don't care about info) with a different buffer size.

Re: WireGuard patchset for OpenBSD, rev. 2

[Jason A. Donenfeld]

Hey tech@,

A few things I thought I should add to our v2 revision:

First, the improvements we've made in the last few weeks have been
pretty substantial, and we've now got a much more faithful protocol
implementation. I've been running this on a few high traffic servers,
and I'll probably move demo.wireguard.com over to it soonish. The ioctl
stuff has also been cleaned up and finalized now.

It's this last point that I'm quite happy about: the latest
wireguard-tools package in ports now fully supports this patch. So that
means after patching your kernel you can easily run:

# pkg_add -Dsnap wireguard-tools

And it'll interface with this implementation like usual, providing wg(8)
and wg-quick(8). Go programmers will also be happy to learn that Matt
Layher has added support for the ioctl interface to his wgctrl-go
project.

As you might have surmised, I'm very interested in getting expanded
testing of this patch. We're still very early in the 6.8 cycle. I'm
wondering if it would make sense to check this v2 revision into cvs
_now_, which will then make it included in -current, so that folks can
test and use this easily. Then, Matt and I can continue to develop and
refine this in one of two ways:

  - Send (bi-?)monthly patches to tech@ with fixes we've been working
on.
  - Enable both of our commit bits for that part of the tree, and we'll
push patches directly.

Either one works, or maybe you have a third idea for that. I'm fairly
committed to working on this full time to get it perfect for 6.8.

The other thing I've been exploring is re-licensing the wireguard-tools
package as MIT or ISC in case we decide at some later point down the
road (not now) that maybe wg(8) would fit well in the base system.

Anyway, please go forth and test this! And thanks a lot for the feedback
from our v1. Looking forward to the same on v2.

Jason

Re: WireGuard patchset for OpenBSD

[Jason A. Donenfeld]

On Mon, May 25, 2020 at 2:16 PM Theo de Raadt  wrote:
>
> I'll make a comment that I am quite unhappy about this ioctl
> methodology.  I don't like void *'s and varying sizes.
>
> I would be much happier if this was a fixed structure, filled with
> known objects.
>
> It looks fragile.

Indeed the fragile linked list magic went away pretty shortly after
Matt posted the initial patchset. We'll have v2 out hopefully tomorrow
if all goes well, where we've been using a much more boring approach
of stacked structs, which in practice is very easy to program for and
deal with. Stay tuned. And sorry about the weird linked list thing.

Jason

Re: WireGuard patchset for OpenBSD

[Jason A. Donenfeld]

On Tue, May 12, 2020 at 3:48 AM Kevin Chadwick  wrote:
>
> On 2020-05-12 06:05, Matt Dunwoodie wrote:
> >  I don't want to put misleading numbers out there and every use case
> >is different, therefore you should perform your own tests. In my
> >environment (tcbbench between two Lenovo x230 (i5-3320m), em(4)
> >ethernet) I was seeing 750mbit/s. This was compared to default
> >isakmpd(8) with a basic ike psk configuration, which achieved
> >380mbit/s. Different configurations will behave differently, of
> >course, but I think we're off to a pretty good start here.
>
> That is certainly more than fast enough for my purposes and at slower speeds
> will cause no issue with the bonus that hw that does not have AES-NI, will 
> still
> be performant.
>
> However I assume that compared to using AES-NI, the machine will be running a
> lot hotter, using more power and be less usable for other tasks.
>
> Especially, less powerful systems will have far less performance when their hw
> support is not utilised and contrary to the wireguard paper. Many embedded
> systems do have AES hw support.
>
> I imagine supporting all those embedded hw variants is problematic and part of
> the reason AES might have been avoided?
>
> I just wonder. Is there scope in the design for adding AES-NI support, in the
> future as a config option even, rather than a runtime negotiation like OpenSSH
> facilitates?

A good place to discuss future revisions of WireGuard protocol design
is over on the WireGuard mailing list --
. WireGuard uses
"versioned crypto" instead of options for ciphersuites, and there's
some interesting discussion to be had [not here] on our current
choices. To answer your question, "nobs" are not something we really
add places, but on another mailing list I'm happy to discuss future
protocol design and that pandora's box. With regards to your inquiry
about performance though:

There are lightening fast implementations of chacha using avx, avx2,
avx512, arm vector extensions, and so forth, that are easily in the
same ballpark as aes-ni. Djb has a nice post on chacha performance in
this context: .
After the initial wireguard patchset lands, I'm happy to explore
adding accelerated chacha implementations to the openbsd kernel. For
example, I've already ported Andy's cryptogams implementations for a
number of platforms to be use in the kernel, and it probably wouldn't
be too hard to put this in OpenBSD. In practice, I've found that the
choice of chacha means that WireGuard performs well on both beefy avx2
hardware and on cheap little mips routers. There's tons of room for
performance improvement, in general. I think what Matt's posted here
is a good, safe, simple implementation that provides a basis for
tweaking and optimizing -- faster crypto implementations, better
queueing algorithms, and so forth.

Jason

Re: WireGuard patchset for OpenBSD

[Jason A. Donenfeld]

On Tue, May 12, 2020 at 12:37 AM Theo de Raadt  wrote:
>
> Jason A. Donenfeld  wrote:
>
> > On Mon, May 11, 2020 at 11:03:45PM -0600, Jason A. Donenfeld wrote:
> > > I plan to publish some easy one-click
> > > scripts for users to mess around with the kernel support while we're
> > > working through it here on the list.
> >
> > While tailing my opensmtpd log waiting for the mailing list server to
> > release it's graylist, aforementioned script came to be. As root on the
> > latest snapshot, run:
> >
> >ftp -o - https://git.zx2c4.com/wireguard-openbsd/plain/quickbuilder.sh | 
> > sh
> >
> > The "ftp|sh" idiom is dumb and you can do better, and feel free to do
> > something safer with the same idiom inside that script. But anyway, if
> > you get past that, reboot, and then you can use wg(8), wg-quick(8), and
> > `ifconfig wg0 create` like normal.
> >
> > This should allow for some quick and dirty testing of this, if folks
> > here are curious or eager to play around.
>
> The safest way is an attached tarball, so that users don't need to hit
> the "rm -rf ~/ / &" that your server decides to send in the future to
> all or specific people.  It isn't a matter of trust, it is simply that
> '|sh' is the new "shar", we are no longer living in that safer time.

Fair enough. Piping the internet to sh is rarely a good idea indeed.
Matt's got a full build hosted that can be sysupgrade(8)'d to and
verified with his signify key like usual. That might be a good idea.

Jason

Re: WireGuard patchset for OpenBSD

[Jason A. Donenfeld]

On Mon, May 11, 2020 at 11:03:45PM -0600, Jason A. Donenfeld wrote:
> I plan to publish some easy one-click
> scripts for users to mess around with the kernel support while we're
> working through it here on the list.

While tailing my opensmtpd log waiting for the mailing list server to
release it's graylist, aforementioned script came to be. As root on the
latest snapshot, run:

   ftp -o - https://git.zx2c4.com/wireguard-openbsd/plain/quickbuilder.sh | sh

The "ftp|sh" idiom is dumb and you can do better, and feel free to do
something safer with the same idiom inside that script. But anyway, if
you get past that, reboot, and then you can use wg(8), wg-quick(8), and
`ifconfig wg0 create` like normal.

This should allow for some quick and dirty testing of this, if folks
here are curious or eager to play around.

Jason

Re: WireGuard patchset for OpenBSD

[Jason A. Donenfeld]

Hey folks,

[resending, as my original reply was to Matt's message that
 got killed by the graylist, so he resent with a new msgid.]

Just wanted to chime in here to mention how thrilled I am about this.
Matt has been at this for a long time, came to visit Paris last summer
to work with me on this, and I think the end result is a very high
quality implementation. I expect all sorts of useful feedback on network
driver APIs and at the very least a v2 to be posted, but I think we've
got a solid foundation here.  Meanwhile, we're fully committed to
getting the rest of the WireGuard project's tooling in sync with first
class OpenBSD support.

On a personal note, I've kind of gazed enviously at OpenBSD for years,
and gladly devoured its general philosophy of programming simple and
secure systems. In many ways, WireGuard itself was inspired by the
simplicity of approaches found in OpenBSD and the elegance of those
interfaces so fastidiously documented in the man pages. So, you might
regard it as just a weird historical accident of my own kernel
development that this was on Linux, because the influence of the project
has clearly been from OpenBSD. (You might have noticed some similar
wackiness last year when I described on misc@ how I'm using signify(1)
with the Windows client... oh my.)

To confirm something particular from Matt's email:

> Lessons that were learned from developing Linux have been carried
> over, however all the code has been ISC licensed and integrated into
> OpenBSD's networking stack. To the extent that there is any similarity
> in the code, I expect for Jason (CC'd) to reply here confirming that
> ISC is good to go.

Any code similarities are fine with me, and the patches Matt submitted
that bare my copyright line I gladly co-license with Matt under ISC.
Those patches are also hosted on my git server:
https://git.zx2c4.com/wireguard-openbsd/ where you can fetch exactly the
same content directly from my box containing the same ISC license. IOW,
we're all good in this department.

Anyway, I'm looking forward to hearing some feedback on this and getting
this polished and shipped during the 6.8 cycle. After jasper@ pushes the
ports update for the tools, I plan to publish some easy one-click
scripts for users to mess around with the kernel support while we're
working through it here on the list. I'm also happy to answer any
questions on both WireGuard design principles as well as implementation
strategies.

Regards,
Jason

Re: WireGuard patchset for OpenBSD

[Jason A. Donenfeld]

Hey folks,

Just wanted to chime in here to mention how thrilled I am about this.
Matt has been at this for a long time, came to visit Paris last summer
to work with me on this, and I think the end result is a very high
quality implementation. I expect all sorts of useful feedback on network
driver APIs and at the very least a v2 to be posted, but I think we've
got a solid foundation here.  Meanwhile, we're fully committed to
getting the rest of the WireGuard project's tooling in sync with first
class OpenBSD support.

On a personal note, I've kind of gazed enviously at OpenBSD for years,
and gladly devoured its general philosophy of programming simple and
secure systems. In many ways, WireGuard itself was inspired by the
simplicity of approaches found in OpenBSD and the elegance of those
interfaces so fastidiously documented in the man pages. So, you might
regard it as just a weird historical accident of my own kernel
development that this was on Linux, because the influence of the project
has clearly been from OpenBSD. (You might have noticed some similar
wackiness last year when I described on misc@ how I'm using signify(1)
with the Windows client... oh my.)

To confirm something particular from Matt's email:

> Lessons that were learned from developing Linux have been carried
> over, however all the code has been ISC licensed and integrated into
> OpenBSD's networking stack. To the extent that there is any similarity
> in the code, I expect for Jason (CC'd) to reply here confirming that
> ISC is good to go.

Any code similarities are fine with me, and the patches Matt submitted
that bare my copyright line I gladly co-license with Matt under ISC.
Those patches are also hosted on my git server:
https://git.zx2c4.com/wireguard-openbsd/ where you can fetch exactly the
same content directly from my box containing the same ISC license. IOW,
we're all good in this department.

Anyway, I'm looking forward to hearing some feedback on this and getting
this polished and shipped during the 6.8 cycle. After jasper@ pushes the
ports update for the tools, I plan to publish some easy one-click
scripts for users to mess around with the kernel support while we're
working through it here on the list. I'm also happy to answer any
questions on both WireGuard design principles as well as implementation
strategies.

Regards,
Jason

Re: OpenBSD kernel implementation

[Jason A. Donenfeld]

Hi Brad,

Exciting to see you working on this. However, I'm afraid the
implementation you describe sounds deeply flawed and kind of misses
the point of WireGuard.

On Tue, Dec 11, 2018 at 2:24 PM  wrote:
> Currently, I want to take all the code that doesn't need to be in the
> kernel and move it to userspace, which is essentially the handshake
> code, timeout timers and state machine functions. What is left is
> essentially the transport function (IPSEC transform equivalent),
> peforming simple crypto on incoming/outgoing packets. This design is
> somewhat similar to how IPSEC is currently implemented in OpenBSD. I
> believe this is a reasonable approach, but welcome comments on things I
> may not have considered.

Do not do this. This is entirely unacceptable and wholly contrary to
the design approach of WireGuard. The transport layer and handshake
layer exist on the same state machine, and I designed the handshake
specifically to be extremely simple and implementable in kernel space.
I'm happy to help you clean up your current approach -- which seems
nicer and closer to the goal -- but your proposed separated approach
is really deeply flawed, and overly complex. Do not make this mistake.

Rather, let's clean up your current WIP together. If you're on IRC,
I'm happy to discuss with you there (I'm zx2c4 on Freenode) and we can
get this into shape.

Regards,
Jason