In tcpdump, we have generally used the "extract" approach rather
than __packed, for various reasons including potential encapculation
cases where compiler generated code has been insufficiently cynical --
meaning, __packed doesn't mean "must access everything as bytes".
Visa Hankala <v...@hankala.org> wrote:
> On Tue, May 04, 2021 at 07:29:20AM +0200, Peter J. Philipp wrote:
> > > Index: print-wg.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/tcpdump/print-wg.c,v
> > > retrieving revision 1.6
> > > diff -u -p -u -r1.6 print-wg.c
> > > --- print-wg.c 14 Apr 2021 19:34:56 -0000 1.6
> > > +++ print-wg.c 3 May 2021 16:29:29 -0000
> > > @@ -21,6 +21,7 @@
> > >
> > > #include <stdio.h>
> > > #include <stddef.h>
> > > +#include <string.h>
> > >
> > > #include "interface.h"
> > > #include "extract.h"
> > > @@ -104,6 +105,9 @@ wg_print(const u_char *bp, u_int length)
> > > struct wg_data *data = (void *)bp;
> > > u_int caplen;
> > >
> > > + uint32_t receiver;
> > > + uint64_t nonce;
> > > +
> > > caplen = snapend - bp;
> > > if (caplen < sizeof(type))
> > > goto trunc;
> > > @@ -142,8 +146,12 @@ wg_print(const u_char *bp, u_int length)
> > > printf("[wg] keepalive ");
> > > if (caplen < offsetof(struct wg_data, mac))
> > > goto trunc;
> > > +
> > > + memcpy((void *)&receiver, (void *)&data->receiver,
> > > sizeof(receiver));
> > > + memcpy((void *)&nonce, (void *)&data->nonce, sizeof(nonce));
> > > +
> > > printf("to 0x%08x nonce %llu",
> > > - letoh32(data->receiver), letoh64(data->nonce));
> > > + letoh32(receiver), letoh64(nonce));
> > > break;
> > > }
> > > return;
> > >
> > >
> > > There may be other variables that need the same treatment... if that
> > > looks ok
> > > for you I'll work on that and submit the patch formally.
> >
> > I dumped with this patch for about an hour, and even restarted the wg tunnel
> > on the device behind vlan6 so that other types get used (initialization) and
> > I found that there was no more SIGBUS's for tcpdump on octeon, so I am
> > unsure
> > if other variables actually do need same treatment still.
>
> data->nonce is the (most) offending variable because it needs 8-byte
> alignment.
>
> An alternative to memcpy() is to use tcpdump's EXTRACT_* macros that
> handle unaligned data.
>
> Yet another option is to declare the structs with the "packed" type
> attribute. This makes the compiler emit machine code that is safe with
> unaligned data. This also enables type checking that is better than with
> EXTRACT_* because types are not overridden with explicit type casts.
> However, "packed" makes it non obvious that unaligned accesses
> may happen.
>
> Index: print-wg.c
> ===================================================================
> RCS file: src/usr.sbin/tcpdump/print-wg.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 print-wg.c
> --- print-wg.c 14 Apr 2021 19:34:56 -0000 1.6
> +++ print-wg.c 14 Sep 2021 12:42:32 -0000
> @@ -34,20 +34,20 @@ struct wg_initiation {
> uint32_t type;
> uint32_t sender;
> uint8_t fill[140]; /* Includes ephemeral + MAC */
> -};
> +} __packed;
>
> struct wg_response {
> uint32_t type;
> uint32_t sender;
> uint32_t receiver;
> uint8_t fill[80]; /* Includes ephemeral + MAC */
> -};
> +} __packed;
>
> struct wg_cookie {
> uint32_t type;
> uint32_t receiver;
> uint8_t fill[56]; /* Includes nonce + encrypted cookie */
> -};
> +} __packed;
>
> struct wg_data {
> uint32_t type;
> @@ -55,7 +55,7 @@ struct wg_data {
> uint64_t nonce;
> /* uint8_t data[variable]; - Variable length data */
> uint8_t mac[16];
> -};
> +} __packed;
>
> /*
> * Check if packet is a WireGuard packet, as WireGuard may run on any port.
>
