l2tp: read and write AVP values through unaligned-safe accessors

AVPs are packed back to back in the receive buffer, so the offset of any
given AVP is the sum of the lengths of all the AVPs before it, i.e. a
value the peer picks. Casting avp->val to uint16_t/uint32_t/uint64_t
therefore dereferences a pointer with an arbitrary alignment, which is
undefined behaviour, is caught by -fsanitize=alignment, and is only
harmless on x86 by accident. The same applies to the send path, where
the AVPs being built are laid out the same way.

Introduce unaligned_{ntohs,ntohl,be64toh}() and their store
counterparts, all memcpy() based, and use them for every multi-byte
field read out of or written into an AVP body. Accesses to the members
of struct l2tp_avp_t itself are fine as it is declared packed.

memxor() had the same problem, in a worse form: it cast both of its
uint8_t pointers to uintmax_t and walked them word by word. Since it is
only ever called on MD5 sized chunks, replace it with a plain byte loop
which compilers vectorize just as well, and which no longer breaks
strict aliasing either.

No functional change intended, this is a portability and UB fix; it also
removes a source of noise for the s390x (big endian) CI job.

1 files changed, 62 insertions, 34 deletions

diff --git a/accel-pppd/ctrl/l2tp/packet.c b/accel-pppd/ctrl/l2tp/packet.c
index 1e1488b5..68f36ece 100644
--- a/accel-pppd/ctrl/l2tp/packet.c
+++ b/accel-pppd/ctrl/l2tp/packet.c
@@ -111,36 +111,64 @@ void l2tp_packet_free(struct l2tp_packet_t *pack)
 	mempool_free(pack);
 }
 
+/*
+ * AVPs are not aligned in any way inside the packet buffer: their offset
+ * depends on the length of every preceding AVP, which is peer chosen.
+ * Always go through memcpy() to read multi-byte fields out of them, both to
+ * stay portable on strict alignment architectures and to avoid tripping
+ * -fsanitize=alignment.
+ */
+static uint16_t unaligned_ntohs(const void *ptr)
+{
+	uint16_t val;
+
+	memcpy(&val, ptr, sizeof(val));
+
+	return ntohs(val);
+}
+
+static uint32_t unaligned_ntohl(const void *ptr)
+{
+	uint32_t val;
+
+	memcpy(&val, ptr, sizeof(val));
+
+	return ntohl(val);
+}
+
+static uint64_t unaligned_be64toh(const void *ptr)
+{
+	uint64_t val;
+
+	memcpy(&val, ptr, sizeof(val));
+
+	return be64toh(val);
+}
+
+static void unaligned_htons(void *ptr, uint16_t val)
+{
+	val = htons(val);
+	memcpy(ptr, &val, sizeof(val));
+}
+
+static void unaligned_htonl(void *ptr, uint32_t val)
+{
+	val = htonl(val);
+	memcpy(ptr, &val, sizeof(val));
+}
+
+static void unaligned_htobe64(void *ptr, uint64_t val)
+{
+	val = htobe64(val);
+	memcpy(ptr, &val, sizeof(val));
+}
+
 static void memxor(uint8_t *dst, const uint8_t *src, size_t sz)
 {
-	const uintmax_t *umax_src = (const uintmax_t *)src;
-	uintmax_t *umax_dst = (uintmax_t *)dst;
-	size_t left = sz % sizeof(uintmax_t);
 	size_t indx;
 
-	for (indx = 0; indx < sz / sizeof(uintmax_t); ++indx)
-		umax_dst[indx] ^= umax_src[indx];
-
-	src += sz - left;
-	dst += sz - left;
-	while (left) {
-		if (left >= sizeof(uint32_t)) {
-			*(uint32_t *)dst ^= *(uint32_t *)src;
-			src += sizeof(uint32_t);
-			dst += sizeof(uint32_t);
-			left -= sizeof(uint32_t);
-		} else if (left >= sizeof(uint16_t)) {
-			*(uint16_t *)dst ^= *(uint16_t *)src;
-			src += sizeof(uint16_t);
-			dst += sizeof(uint16_t);
-			left -= sizeof(uint16_t);
-		} else {
-			*dst ^= *src;
-			src += sizeof(uint8_t);
-			dst += sizeof(uint8_t);
-			left -= sizeof(uint8_t);
-		}
-	}
+	for (indx = 0; indx < sz; ++indx)
+		dst[indx] ^= src[indx];
 }
 
 /*
@@ -185,7 +213,7 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV,
 	}
 
 	memxor(p1, avp->val, MD5_DIGEST_LENGTH);
-	orig_attr_len = ntohs(*(uint16_t *)p1);
+	orig_attr_len = unaligned_ntohs(p1);
 
 	if (orig_attr_len <= MD5_DIGEST_LENGTH - 2) {
 		/* Enough bytes decoded already, no need to decode padding */
@@ -423,7 +451,7 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info,
 				if (decode_avp(avp, RV, secret, secret_len) < 0)
 					goto out_err;
 
-				orig_avp_len = ntohs(*(uint16_t *)avp->val) + sizeof(*avp);
+				orig_avp_len = unaligned_ntohs(avp->val) + sizeof(*avp);
 				orig_avp_val = avp->val + sizeof(uint16_t);
 			} else {
 				orig_avp_len = avp_len;
@@ -445,17 +473,17 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info,
 				case ATTR_TYPE_INT16:
 					if (orig_avp_len != sizeof(*avp) + 2)
 						goto out_err_len;
-					attr->val.uint16 = ntohs(*(uint16_t *)orig_avp_val);
+					attr->val.uint16 = unaligned_ntohs(orig_avp_val);
 					break;
 				case ATTR_TYPE_INT32:
 					if (orig_avp_len != sizeof(*avp) + 4)
 						goto out_err_len;
-					attr->val.uint32 = ntohl(*(uint32_t *)orig_avp_val);
+					attr->val.uint32 = unaligned_ntohl(orig_avp_val);
 					break;
 				case ATTR_TYPE_INT64:
 					if (orig_avp_len != sizeof(*avp) + 8)
 						goto out_err_len;
-					attr->val.uint64 = be64toh(*(uint64_t *)orig_avp_val);
+					attr->val.uint64 = unaligned_be64toh(orig_avp_val);
 					break;
 				case ATTR_TYPE_OCTETS:
 					attr->val.octets = _malloc(attr->length);
@@ -532,13 +560,13 @@ int l2tp_packet_send(int sock, struct l2tp_packet_t *pack)
 		else
 			switch (attr->attr->type) {
 			case ATTR_TYPE_INT16:
-				*(int16_t *)avp->val = htons(attr->val.int16);
+				unaligned_htons(avp->val, attr->val.int16);
 				break;
 			case ATTR_TYPE_INT32:
-				*(int32_t *)avp->val = htonl(attr->val.int32);
+				unaligned_htonl(avp->val, attr->val.int32);
 				break;
 			case ATTR_TYPE_INT64:
-				*(uint64_t *)avp->val = htobe64(attr->val.uint64);
+				unaligned_htobe64(avp->val, attr->val.uint64);
 				break;
 			case ATTR_TYPE_STRING:
 			case ATTR_TYPE_OCTETS: