From 9d7b15df22d75e3664e4d33db6c6ae1e35773229 Mon Sep 17 00:00:00 2001 From: Vinicius Costa Gomes Date: Fri, 31 Mar 2023 17:31:23 -0700 Subject: [PATCH 1/3] Allow CFLAGS to be overwritten by the user Allow the user to control the value of CFLAGS without having to change the Makefile. Will allow easier experiments with different compilation flags/sanitizer options. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 0fb3e7d..e3d22f0 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ NAME = ntpperf CPPFLAGS = -D_GNU_SOURCE -CFLAGS = -O2 -Wall -g +CFLAGS ?= -O2 -Wall -g LDFLAGS = -lpcap -lm ifdef NTPPERF_NTS From 3e0c45fcf5270e458ea581b07a52cafd29c977ba Mon Sep 17 00:00:00 2001 From: Vinicius Costa Gomes Date: Fri, 31 Mar 2023 17:33:35 -0700 Subject: [PATCH 2/3] Add safe unaligned memory access routines These routines generate code known to be safe for unaligned memory access. Adapted/copied from: https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/shared/util.h#n53 Which is very similar to this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/asm-generic/unaligned.h#n12 --- sender.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/sender.h b/sender.h index 2b80caf..28bb613 100644 --- a/sender.h +++ b/sender.h @@ -59,4 +59,61 @@ int sender_start(struct sender_config *config); bool sender_send_requests(int sender_fd, struct sender_request *requests, int num); void sender_stop(int sender_fd); +#define GET_UNALIGNED(ptr) __extension__ \ +({ \ + struct __attribute__((packed)) { \ + __typeof__(*(ptr)) __v; \ + } *__p = (__typeof__(__p)) (ptr); \ + __p->__v; \ +}) + +#define PUT_UNALIGNED(val, ptr) \ +do { \ + struct __attribute__((packed)) { \ + __typeof__(*(ptr)) __v; \ + } *__p = (__typeof__(__p)) (ptr); \ + __p->__v = (val); \ +} while(0) + + +static inline uint8_t get_u8(const void *ptr) +{ + return *((const uint8_t *) ptr); +} + +static inline void put_u8(uint8_t val, void *ptr) +{ + *((uint8_t *) ptr) = val; +} + +static inline uint16_t get_u16(const void *ptr) +{ + return GET_UNALIGNED((const uint16_t *) ptr); +} + +static inline uint32_t get_u32(const void *ptr) +{ + return GET_UNALIGNED((const uint32_t *) ptr); +} + +static inline uint32_t get_u64(const void *ptr) +{ + return GET_UNALIGNED((const uint32_t *) ptr); +} + +static inline void put_u16(uint16_t val, void *ptr) +{ + PUT_UNALIGNED(val, (uint16_t *) ptr); +} + +static inline void put_u32(uint32_t val, void *ptr) +{ + PUT_UNALIGNED(val, (uint32_t *) ptr); +} + +static inline void put_u64(uint64_t val, void *ptr) +{ + PUT_UNALIGNED(val, (uint64_t *) ptr); +} + #endif From 65e4b26ab20fa78d791454cfb6953e3d8bc44285 Mon Sep 17 00:00:00 2001 From: Vinicius Costa Gomes Date: Fri, 31 Mar 2023 17:38:36 -0700 Subject: [PATCH 3/3] Replace raw memory access by their safer versions Solve some undefined memory access errors detected by the compiler. Changing the compilation flags to: CFLAGS = -O2 -Wall -g -fsanitize=undefined We get these kinds of errors during runtime: ender.c:150:26: runtime error: store to misaligned address 0x7ffca4a974fa for type 'uint32_t', which requires 4 byte alignment 0x7ffca4a974fa: note: pointer points here 40 11 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ sender.c:151:26: runtime error: store to misaligned address 0x7ffca4a974fe for type 'uint32_t', which requires 4 byte alignment 0x7ffca4a974fe: note: pointer points here ac 12 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ sender.c:173:27: runtime error: store to misaligned address 0x7ffca4a97522 for type 'uint64_t', which requires 8 byte alignment 0x7ffca4a97522: note: pointer points here 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ sender.c:174:27: runtime error: store to misaligned address 0x7ffca4a9752a for type 'uint64_t', which requires 8 byte alignment 0x7ffca4a9752a: note: pointer points here 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ sender.c:175:27: runtime error: store to misaligned address 0x7ffca4a97532 for type 'uint64_t', which requires 8 byte alignment 0x7ffca4a97532: note: pointer points here 36 00 dd 17 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ --- perf.c | 38 ++++++++++++++++---------------- sender.c | 67 ++++++++++++++++++++++++++++---------------------------- 2 files changed, 53 insertions(+), 52 deletions(-) diff --git a/perf.c b/perf.c index 3bbddf3..fee3e79 100644 --- a/perf.c +++ b/perf.c @@ -226,23 +226,23 @@ static bool process_response(struct pcap_pkthdr *header, const u_char *data, str if (memcmp(config->dst_mac, data + 6, 6)) return false; - if (ntohs(*(uint16_t *)(data + 12)) != 0x0800) + if (ntohs(get_u16(data + 12)) != 0x0800) return false; data += 14; if (data[0] >> 4 != 4 || (data[0] & 0xf) != 5 || data[9] != 17 || - ntohl(*(uint32_t *)(data + 12)) != config->dst_address) + ntohl(get_u32(data + 12)) != config->dst_address) return false; - dst_address = ntohl(*(uint32_t *)(data + 16)); - src_port = ntohs(*(uint16_t *)(data + 20)); - dst_port = ntohs(*(uint16_t *)(data + 22)); + dst_address = ntohl(get_u32(data + 16)); + src_port = ntohs(get_u16(data + 20)); + dst_port = ntohs(get_u16(data + 22)); data += 28; if (config->ptp_mcast) { if (dst_address != PTP_MCAST_ADDR || (data[0] & 0xf) != 9) return false; - dst_address = ntohl(*(uint32_t *)(data + 44)); + dst_address = ntohl(get_u32(data + 44)); } if ((dst_address ^ config->src_network) >> (32 - config->src_bits)) @@ -259,14 +259,14 @@ static bool process_response(struct pcap_pkthdr *header, const u_char *data, str return false; valid = header->caplen >= 90 && (data[0] & 0x7) == 0x4 && - (*(uint64_t *)(data + 24) & -2ULL) == (client->local_id & -2ULL) && + (get_u64(data + 24) & -2ULL) == (client->local_id & -2ULL) && (!config->nts.cookie || header->len > 90 + 4 + 32 + 4 + 16 + 16 + 4); if (valid) { if (config->mode == NTP_INTERLEAVED) - client->remote_id = *(uint64_t *)(data + 32); + client->remote_id = get_u64(data + 32); else - client->remote_id = *(uint64_t *)(data + 40); - client->remote_rx = convert_ntp_ts(*(uint64_t *)(data + 32)); + client->remote_id = get_u64(data + 40); + client->remote_rx = convert_ntp_ts(get_u64(data + 32)); client->local_rx = local_rx; } break; @@ -277,7 +277,7 @@ static bool process_response(struct pcap_pkthdr *header, const u_char *data, str ptp_type = data[0] & 0xf; valid = header->caplen >= 86 && data[1] == 2 && - *(uint16_t *)(data + 30) == (uint16_t)client->local_id && + get_u16(data + 30) == (uint16_t)client->local_id && ((ptp_type == 9 && dst_port == 320) || (config->mode == PTP_NSM && ((ptp_type == 0 && dst_port == 319) || @@ -290,9 +290,9 @@ static bool process_response(struct pcap_pkthdr *header, const u_char *data, str client->local_rx = local_rx; break; case 9: - client->remote_rx = convert_ptp_ts(*(uint16_t *)(data + 34), - *(uint32_t *)(data + 36), - *(uint32_t *)(data + 40)); + client->remote_rx = convert_ptp_ts(get_u16(data + 34), + get_u32(data + 36), + get_u32(data + 40)); /* Fall through */ case 8: memset(&client->local_rx, 0, sizeof client->local_rx); @@ -317,7 +317,7 @@ static bool process_response(struct pcap_pkthdr *header, const u_char *data, str switch (config->mode) { case NTP_BASIC: case NTP_INTERLEAVED: - if (*(uint64_t *)(data + 24) == client->local_id) { + if (get_u64(data + 24) == client->local_id) { stats->basic_responses++; if (config->mode != NTP_BASIC) return true; @@ -330,7 +330,7 @@ static bool process_response(struct pcap_pkthdr *header, const u_char *data, str local_rx = prev_local_rx; } - remote_tx = convert_ntp_ts(*(uint64_t *)(data + 40)); + remote_tx = convert_ntp_ts(get_u64(data + 40)); break; case PTP_DELAY: case PTP_NSM: @@ -346,9 +346,9 @@ static bool process_response(struct pcap_pkthdr *header, const u_char *data, str stats->sync_responses++; remote_rx = client->remote_rx; - remote_tx = convert_ptp_ts(*(uint16_t *)(data + 34), - *(uint32_t *)(data + 36), - *(uint32_t *)(data + 40)); + remote_tx = convert_ptp_ts(get_u16(data + 34), + get_u32(data + 36), + get_u32(data + 40)); break; case 9: stats->delay_responses++; diff --git a/sender.c b/sender.c index 50b43bc..d15a1b3 100644 --- a/sender.c +++ b/sender.c @@ -141,27 +141,28 @@ static int make_packet(struct sender_request *request, struct sender_config *con /* Ethernet header */ memcpy(buf + 0, config->dst_mac, 6); memcpy(buf + 6, config->src_mac, 6); - *(uint16_t *)(buf + 12) = htons(0x0800); + put_u16(htons(0x0800), buf + 12); buf += 14, len += 14; /* IP header */ memcpy(buf, "\x45\x00\x00\x00\xd7\xe9\x40\x00\x40\x11", 10); - *(uint16_t *)(buf + 2) = htons(20 + 8 + data_len); - *(uint32_t *)(buf + 12) = htonl(request->src_address); - *(uint32_t *)(buf + 16) = htonl(config->dst_address); + put_u16(htons(20 + 8 + data_len), buf + 2); + put_u32(htonl(request->src_address), buf + 12); + put_u32(htonl(config->dst_address), buf + 16); - for (i = 0; i < 10; i++) - sum += ((uint16_t *)buf)[i]; + for (i = 0; i < 20; i += 2) + sum += get_u16(buf + i); while ((carry = sum >> 16)) sum = (sum & 0xffff) + carry; - *(uint16_t *)(buf + 10) = ~sum; + put_u16(~sum, buf + 10); + buf += 20, len += 20; /* UDP header and data */ - *(uint16_t *)(buf + 0) = htons(src_port); - *(uint16_t *)(buf + 2) = htons(dst_port); - *(uint16_t *)(buf + 4) = htons(8 + data_len); + put_u16(htons(src_port), buf + 0); + put_u16(htons(dst_port), buf + 2); + put_u16(htons(8 + data_len), buf + 4); buf += 8, len += 8; assert(max_len >= len + data_len); @@ -170,21 +171,21 @@ static int make_packet(struct sender_request *request, struct sender_config *con case NTP_BASIC: case NTP_INTERLEAVED: buf[0] = 0xe3; - *(uint64_t *)(buf + 24) = request->remote_id; - *(uint64_t *)(buf + 32) = request->local_id ^ 1; - *(uint64_t *)(buf + 40) = request->local_id; + put_u64(request->remote_id, buf + 24); + put_u64(request->local_id ^ 1, buf + 32); + put_u64(request->local_id, buf + 40); auth = buf + 48; break; case PTP_NSM: - *(uint32_t *)(buf + 44) = htonl(0x21fe0000); + put_u32(htonl(0x21fe0000), buf + 44); /* Fall through */ case PTP_DELAY: - *(uint16_t *)(buf + 0) = htons(0x0102); - *(uint16_t *)(buf + 2) = htons(data_len); - *(uint8_t *)(buf + 4) = config->ptp_domain; + put_u16(htons(0x0102), buf + 0); + put_u16(htons(data_len), buf + 2); + put_u8(config->ptp_domain, buf + 4); buf[6] = config->ptp_mcast ? 0 : 0x4; - *(uint32_t *)(buf + 20) = htonl(request->src_address); - *(uint16_t *)(buf + 30) = request->local_id; + put_u32(htonl(request->src_address), buf + 20); + put_u16(request->local_id, buf + 30); buf[32] = 0x1; break; default: @@ -196,27 +197,27 @@ static int make_packet(struct sender_request *request, struct sender_config *con size_t clen; /* Unique Identifier */ - *(uint16_t *)(auth + 0) = htons(0x0104); - *(uint16_t *)(auth + 2) = htons(4 + 32); - *(uint32_t *)(auth + 4) = random(); - *(uint32_t *)(auth + 8) = random(); + put_u16(htons(0x0104), auth + 0); + put_u16(htons(4 + 32), auth + 2); + put_u32(random(), auth + 4); + put_u32(random(), auth + 8); auth += 4 + 32; /* Cookie */ - *(uint16_t *)(auth + 0) = htons(0x0204); - *(uint16_t *)(auth + 2) = htons(4 + nts->cookie_len); + put_u16(htons(0x0204), auth + 0); + put_u16(htons(4 + nts->cookie_len), auth + 2); memcpy(auth + 4, nts->cookie, nts->cookie_len); auth += 4 + nts->cookie_len; /* Authenticator */ - *(uint16_t *)(auth + 0) = htons(0x0404); - *(uint16_t *)(auth + 2) = htons(4 + 4 + 16 + 16); - *(uint16_t *)(auth + 4) = htons(16); - *(uint16_t *)(auth + 6) = htons(16); - *(uint32_t *)(auth + 8) = random(); - *(uint32_t *)(auth + 12) = random(); - *(uint32_t *)(auth + 16) = random(); - *(uint32_t *)(auth + 20) = random(); + put_u16(htons(0x0404), auth + 0); + put_u16(htons(4 + 4 + 16 + 16), auth + 2); + put_u16(htons(16), auth + 4); + put_u16(htons(16), auth + 6); + put_u32(random(), auth + 8); + put_u32(random(), auth + 12); + put_u32(random(), auth + 16); + put_u32(random(), auth + 20); clen = 16; if (gnutls_aead_cipher_encrypt(nts->cipher, auth + 4 + 4, 16, buf, auth - buf, 0,