fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/43143?usp=email ) Change subject: llc: fix misaligned MetaInfo access in msgb headroom ...................................................................... llc: fix misaligned MetaInfo access in msgb headroom struct MetaInfo (two struct timespec) requires 8-byte alignment on some ABIs (e.g. 32-bit ARM with a 64-bit time_t), but msgb->head is only guaranteed byte alignment: struct msgb's own size before the flexible _data[] member is not a multiple of 8, so storing MetaInfo directly at msg->head can misalign it, triggering UBSan -fsanitize=alignment reports wherever recv_time/expire_time are subsequently dereferenced. Reserve extra headroom for worst-case alignment padding and store/retrieve MetaInfo via the next 8-byte-aligned address within that headroom instead of assuming msg->head itself is suitably aligned. Change-Id: I39abce68c5678704eea2ad1e330419f645aa4cfd Related: OS#6858 --- M src/llc.c 1 file changed, 34 insertions(+), 8 deletions(-) Approvals: osmith: Looks good to me, but someone else must approve pespin: Looks good to me, but someone else must approve fixeria: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/llc.c b/src/llc.c index 478d23d..8107c0c 100644 --- a/src/llc.c +++ b/src/llc.c @@ -17,6 +17,7 @@ #include <stdio.h> +#include <stdint.h> #include <osmocom/core/msgb.h> @@ -25,6 +26,31 @@ #include "pcu_utils.h" #include "llc.h" +/* struct msgb's flexible '_data' array is only guaranteed to start at an + * address suitable for 'unsigned char', not for arbitrary types: on some + * ABIs (e.g. 32-bit ARM with a 64-bit time_t) 'struct MetaInfo' requires + * stricter (8-byte) alignment than that, courtesy of the embedded + * 'struct timespec' members, so msgb->head itself cannot be assumed to be + * a valid storage location for it. Reserve extra headroom for worst-case + * padding and store/retrieve MetaInfo at the next aligned address instead + * of at msgb->head directly. */ +#define META_INFO_ALIGN _Alignof(struct MetaInfo) +#define META_INFO_HEADROOM (sizeof(struct MetaInfo) + META_INFO_ALIGN - 1) + +/* Allocate a msgb with enough headroom to hold a MetaInfo (plus worst-case + * alignment padding) followed by 'len' bytes of LLC PDU data. */ +static inline struct msgb *llc_msgb_meta_info_alloc(uint16_t len, const char *name) +{ + return msgb_alloc_headroom(META_INFO_HEADROOM + len, META_INFO_HEADROOM, name); +} + +static inline struct MetaInfo *llc_msgb_meta_info_get(const struct msgb *msg) +{ + uintptr_t p = (uintptr_t)msg->head; + p = (p + (META_INFO_ALIGN - 1)) & ~(uintptr_t)(META_INFO_ALIGN - 1); + return (struct MetaInfo *)p; +} + void llc_init(struct gprs_llc *llc) { llc_reset(llc); @@ -148,12 +174,12 @@ struct gprs_llc_hdr *llc_hdr; enum gprs_llc_queue_prio prio; - llc_msg = msgb_alloc_headroom(sizeof(*meta_storage) + len, sizeof(*meta_storage), "llc_pdu_queue"); + llc_msg = llc_msgb_meta_info_alloc(len, "llc_pdu_queue"); if (!llc_msg) return -ENOMEM; /* Fist set up MetaInfo in the msgb headroom: */ - meta_storage = (struct MetaInfo *)llc_msg->head; + meta_storage = llc_msgb_meta_info_get(llc_msg); osmo_clock_gettime(CLOCK_MONOTONIC, &meta_storage->recv_time); meta_storage->expire_time = *expire_time; @@ -215,8 +241,8 @@ msg = msg1; msg1 = NULL; } else { - const struct MetaInfo *mi1 = (struct MetaInfo *)msg1->head; - const struct MetaInfo *mi2 = (struct MetaInfo *)msg2->head; + const struct MetaInfo *mi1 = llc_msgb_meta_info_get(msg1); + const struct MetaInfo *mi2 = llc_msgb_meta_info_get(msg2); if (timespeccmp(&mi2->recv_time, &mi1->recv_time, >)) { msg = msg1; @@ -250,10 +276,10 @@ struct MetaInfo *meta_storage; unsigned int len = llc_frame_length(llc); - llc_msg = msgb_alloc_headroom(sizeof(*meta_storage) + len, sizeof(*meta_storage), "llc_pdu_queue"); + llc_msg = llc_msgb_meta_info_alloc(len, "llc_pdu_queue"); OSMO_ASSERT(llc_msg); - meta_storage = (struct MetaInfo *)llc_msg->head; + meta_storage = llc_msgb_meta_info_get(llc_msg); memcpy(meta_storage, &llc->meta_info, sizeof(struct MetaInfo)); memcpy(msgb_put(llc_msg, len), llc->frame, len); @@ -283,7 +309,7 @@ if (!msg) return NULL; - meta_storage = (struct MetaInfo *)msg->head; + meta_storage = llc_msgb_meta_info_get(msg); q->queue_size -= 1; q->queue_octets -= msgb_length(msg); @@ -316,7 +342,7 @@ timespecadd(&tv_now, &hyst_delta, &tv_now2); while ((msg = llc_queue_pick_msg(q, &prio))) { - info = (const struct MetaInfo *)msg->head; + info = llc_msgb_meta_info_get(msg); const struct timespec *tv_disc = &info->expire_time; const struct timespec *tv_recv = &info->recv_time; -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/43143?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I39abce68c5678704eea2ad1e330419f645aa4cfd Gerrit-Change-Number: 43143 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: Hoernchen <ewild@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: osmith <osmith@sysmocom.de> Gerrit-Reviewer: pespin <pespin@sysmocom.de>