From ce48e5a8da49d25f5490cc1be6a54f8d9db5f520 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20=C4=8Cun=C3=A1t?= Date: Tue, 6 Apr 2021 17:28:52 +0200 Subject: [PATCH] treewide: fix unaligned access Some less common HW (not x86, usually ARM) doesn't tolerate unaligned access to memory and it's breakage of C as well. It's easiest to check by meson's -Db_sanitize=undefined (on any HW). I pushed millions of real-life QNAME+QTYPE queries over UDP in default mode and the sanitizer seems clear now. --- lib/cache/impl.h | 16 +++++++++++----- lib/cache/nsec1.c | 14 ++++++++++---- lib/layer/iterate.c | 4 +++- lib/selection.c | 4 ++-- 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/lib/cache/impl.h b/lib/cache/impl.h index b884c361..fca8653c 100644 --- a/lib/cache/impl.h +++ b/lib/cache/impl.h @@ -45,7 +45,10 @@ struct entry_h { bool has_optout : 1; /**< Only for packets; persisted DNSSEC_OPTOUT. */ uint8_t _pad; /**< We need even alignment for data now. */ uint8_t data[]; -}; +/* Well, we don't really need packing or alignment changes, + * but due to LMDB the whole structure may not be stored at an aligned address, + * and we need compilers (for non-x86) to know it to avoid SIGBUS (test: UBSAN). */ +} __attribute__ ((packed,aligned(1))); struct entry_apex; /** Check basic consistency of entry_h for 'E' entries, not looking into ->data. @@ -303,10 +306,13 @@ static inline int rdataset_dematerialized_size(const uint8_t *data, uint16_t *rd assert(sizeof(count) == KR_CACHE_RR_COUNT_SIZE); memcpy(&count, data, sizeof(count)); const uint8_t *rdata = data + sizeof(count); - if (rdataset_count) - *rdataset_count = count; - for (int i = 0; i < count; ++i) - rdata += knot_rdata_size(((knot_rdata_t *)rdata)->len); + if (rdataset_count) // memcpy is safe for unaligned case (on non-x86) + memcpy(rdataset_count, &count, sizeof(count)); + for (int i = 0; i < count; ++i) { + __typeof__(((knot_rdata_t *)NULL)->len) len; // memcpy as above + memcpy(&len, rdata + offsetof(knot_rdata_t, len), sizeof(len)); + rdata += knot_rdata_size(len); + } return rdata - (data + sizeof(count)); } diff --git a/lib/cache/nsec1.c b/lib/cache/nsec1.c index 3985ca3b..ed242ca8 100644 --- a/lib/cache/nsec1.c +++ b/lib/cache/nsec1.c @@ -197,8 +197,14 @@ static const char * find_leq_NSEC1(struct kr_cache *cache, const struct kr_query /* We know it starts before sname, so let's check the other end. * 1. construct the key for the next name - kwz_hi. */ /* it's *full* name ATM */ - const knot_rdata_t *next = (const knot_rdata_t *) - (eh->data + KR_CACHE_RR_COUNT_SIZE); + /* Technical complication: memcpy is safe for unaligned case (on non-x86) */ + __typeof__(((knot_rdata_t *)NULL)->len) next_len; + const uint8_t *next_data; + { /* next points to knot_rdata_t but possibly unaligned */ + const uint8_t *next = eh->data + KR_CACHE_RR_COUNT_SIZE; + memcpy(&next_len, next + offsetof(knot_rdata_t, len), sizeof(next_len)); + next_data = next + offsetof(knot_rdata_t, data); + } if (KR_CACHE_RR_COUNT_SIZE != 2 || get_uint16(eh->data) == 0) { assert(false); return "ERROR"; @@ -220,8 +226,8 @@ static const char * find_leq_NSEC1(struct kr_cache *cache, const struct kr_query /* Lower-case chs; see also RFC 6840 5.1. * LATER(optim.): we do lots of copying etc. */ knot_dname_t lower_buf[KNOT_DNAME_MAXLEN]; - ret = knot_dname_to_wire(lower_buf, next->data, - MIN(next->len, KNOT_DNAME_MAXLEN)); + ret = knot_dname_to_wire(lower_buf, next_data, + MIN(next_len, KNOT_DNAME_MAXLEN)); if (ret < 0) { /* _ESPACE */ return "range search found record with incorrect contents"; } diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c index 94342cfb..8fa8010e 100644 --- a/lib/layer/iterate.c +++ b/lib/layer/iterate.c @@ -132,7 +132,9 @@ static bool is_valid_addr(const uint8_t *addr, size_t len) { if (len == sizeof(struct in_addr)) { /* Filter ANY and 127.0.0.0/8 */ - uint32_t ip_host = ntohl(*(const uint32_t *)(addr)); + uint32_t ip_host; /* Memcpy is safe for unaligned case (on non-x86) */ + memcpy(&ip_host, addr, sizeof(ip_host)); + ip_host = ntohl(ip_host); if (ip_host == 0 || (ip_host & 0xff000000) == 0x7f000000) { return false; } diff --git a/lib/selection.c b/lib/selection.c index 56530cba..4b516bb8 100644 --- a/lib/selection.c +++ b/lib/selection.c @@ -155,8 +155,8 @@ struct rtt_state get_rtt_state(const uint8_t *ip, size_t len, } else if (value.len != sizeof(struct rtt_state)) { assert(false); // shouldn't happen but let's be more robust state = default_rtt_state; - } else { - state = *(struct rtt_state *)value.data; + } else { // memcpy is safe for unaligned case (on non-x86) + memcpy(&state, value.data, sizeof(state)); } free(key.data); -- 2.31.0