From 835a2d8a27f43c01521101458fde8c2ea3d88b3a Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Wed, 25 Nov 2015 23:33:28 -0500 Subject: [PATCH 1/3] Add inet_csum_slice() to fix checksum for a sliced layer 4 payload Padding for an odd number of bytes was not calculated properly. --- sys/include/net/inet_csum.h | 39 +++++++++++++++++++++--- sys/net/crosslayer/inet_csum/inet_csum.c | 17 ++++++++--- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/sys/include/net/inet_csum.h b/sys/include/net/inet_csum.h index 53134053f6..6356e96998 100644 --- a/sys/include/net/inet_csum.h +++ b/sys/include/net/inet_csum.h @@ -21,13 +21,15 @@ #define INET_CSUM_H_ #include +#include #ifdef __cplusplus extern "C" { #endif /** - * @brief Calculates the unnormalized Internet Checksum of @p buf. + * @brief Calculates the unnormalized Internet Checksum of @p buf, where the + * buffer provides a slice of the full checksum domain, calculated in order. * * @see * RFC 1071 @@ -35,14 +37,41 @@ extern "C" { * * @details The Internet Checksum is not normalized (i. e. its 1's complement * was not taken of the result) to use it for further calculation. + * This function handles padding an odd number of bytes across the full domain. * - * @param[in] sum An initial value for the checksum. - * @param[in] buf A buffer. - * @param[in] len Length of @p buf in byte. + * @param[in] sum An initial value for the checksum. + * @param[in] buf A buffer. + * @param[in] len Length of @p buf in byte. + * @param[in] accum_len Accumulated length of checksum domain that has already + * been checksummed. * * @return The unnormalized Internet Checksum of @p buf. */ -uint16_t inet_csum(uint16_t sum, const uint8_t *buf, uint16_t len); +uint16_t inet_csum_slice(uint16_t sum, const uint8_t *buf, uint16_t len, size_t accum_len); + +/** + * @brief Calculates the unnormalized Internet Checksum of @p buf, where the + * buffer provides a standalone domain for the checksum. + * + * @see + * RFC 1071 + * + * + * @details The Internet Checksum is not normalized (i. e. its 1's complement + * was not taken of the result) to use it for further calculation. + * This function, rather than inet_csum_slice(), has been used historically + * when we are not concerned with padding for an odd number of bytes. + * + * + * @param[in] sum An initial value for the checksum. + * @param[in] buf A buffer. + * @param[in] len Length of @p buf in byte. + * + * @return The unnormalized Internet Checksum of @p buf. + */ +static inline uint16_t inet_csum(uint16_t sum, const uint8_t *buf, uint16_t len) { + return inet_csum_slice(sum, buf, len, 0); +} #ifdef __cplusplus } diff --git a/sys/net/crosslayer/inet_csum/inet_csum.c b/sys/net/crosslayer/inet_csum/inet_csum.c index c53c025d60..4a125fab7f 100644 --- a/sys/net/crosslayer/inet_csum/inet_csum.c +++ b/sys/net/crosslayer/inet_csum/inet_csum.c @@ -20,7 +20,7 @@ #define ENABLE_DEBUG (0) #include "debug.h" -uint16_t inet_csum(uint16_t sum, const uint8_t *buf, uint16_t len) +uint16_t inet_csum_slice(uint16_t sum, const uint8_t *buf, uint16_t len, size_t accum_len) { uint32_t csum = sum; @@ -34,14 +34,23 @@ uint16_t inet_csum(uint16_t sum, const uint8_t *buf, uint16_t len) #endif #endif + if (len == 0) + return csum; + + if (accum_len & 1) { /* if accumulated length is odd */ + csum += *buf; /* add first byte as bottom half of 16-byte word */ + buf++; + len--; + accum_len++; + } + for (int i = 0; i < (len >> 1); buf += 2, i++) { csum += (*buf << 8) + *(buf + 1); /* group bytes by 16-byte words * and add them*/ } - if (len & 1) { /* if len is odd */ - csum += (*buf << 8); /* add last byte as top half of 16-byte word */ - } + if ((accum_len + len) & 1) /* if accumulated length is odd */ + csum += (*buf << 8); /* add last byte as top half of 16-byte word */ while (csum >> 16) { uint16_t carry = csum >> 16; From 0cd5bf9b8f0b13ec7e454f58ca4bcffe4592dd3f Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Fri, 27 Nov 2015 23:50:09 -0500 Subject: [PATCH 2/3] Add unit tests for inet_csum_slice() --- .../tests-inet_csum/tests-inet_csum.c | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/unittests/tests-inet_csum/tests-inet_csum.c b/tests/unittests/tests-inet_csum/tests-inet_csum.c index 1af7299ca0..ebb40e6e04 100644 --- a/tests/unittests/tests-inet_csum/tests-inet_csum.c +++ b/tests/unittests/tests-inet_csum/tests-inet_csum.c @@ -121,6 +121,50 @@ static void test_inet_csum__odd_len(void) TEST_ASSERT_EQUAL_INT(0xffff, inet_csum(17 + 39, data, sizeof(data))); } +static void test_inet_csum__two_app_snips(void) +{ + /* CoAP header with Uri-Path and Content-Format options; odd length */ + uint8_t data_hdr[] = { + 0x50, 0x02, 0x00, 0x01, 0xb4, 0x74, 0x65, 0x73, + 0x74, 0x10, 0xff, + }; + /* Single character payload, 'a' */ + uint8_t data_pyld[] = { + 0x61, + }; + uint16_t hdr_sum, pyld_sum, hdr_expected = 0xdcfc; + + /* result unnormalized: + * initial sum (0) is arbitrary, and incoming length (0) must be even; + * we expect last byte is shifted left for this odd-sized header */ + hdr_sum = inet_csum_slice(0, data_hdr, sizeof(data_hdr), 0); + TEST_ASSERT_EQUAL_INT(hdr_expected, hdr_sum); + + /* Since header was odd length, we expect the single byte in the payload + * snip is not shifted left */ + pyld_sum = inet_csum_slice(hdr_expected, data_pyld, sizeof(data_pyld), sizeof(data_hdr)); + TEST_ASSERT_EQUAL_INT(hdr_expected + 0x61, pyld_sum); +} + +static void test_inet_csum__empty_app_buffer(void) +{ + /* CoAP header with Uri-Path and Content-Format options; odd length */ + uint8_t data_hdr[] = { + 0x50, 0x02, 0x00, 0x01, 0xb4, 0x74, 0x65, 0x73, + 0x74, 0x10, 0xff, + }; + uint16_t hdr_sum, pyld_sum, hdr_expected = 0xdcfc; + + /* result unnormalized: + * explictly using an odd-sized header for the first slice, to setup corner case */ + hdr_sum = inet_csum_slice(0, data_hdr, sizeof(data_hdr), 0); + TEST_ASSERT_EQUAL_INT(hdr_expected, hdr_sum); + + /* expect an empty buffer simply to reflect the incoming checksum */ + pyld_sum = inet_csum_slice(hdr_expected, NULL, 0, sizeof(data_hdr)); + TEST_ASSERT_EQUAL_INT(hdr_expected, pyld_sum); +} + Test *tests_inet_csum_tests(void) { EMB_UNIT_TESTFIXTURES(fixtures) { @@ -130,6 +174,8 @@ Test *tests_inet_csum_tests(void) new_TestFixture(test_inet_csum__wraps_more_than_once), new_TestFixture(test_inet_csum__calculate_csum), new_TestFixture(test_inet_csum__odd_len), + new_TestFixture(test_inet_csum__two_app_snips), + new_TestFixture(test_inet_csum__empty_app_buffer), }; EMB_UNIT_TESTCALLER(inet_csum_tests, NULL, NULL, fixtures); From 1abffc84e1f1b7accaa7c58361201717dc5b2629 Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Sat, 28 Nov 2015 16:11:07 -0500 Subject: [PATCH 3/3] Update layer 4 files to fix checksum with inet_csum_slice(). --- sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c | 2 +- sys/net/gnrc/transport_layer/udp/gnrc_udp.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c b/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c index b85e7166db..c2d70fc021 100644 --- a/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c +++ b/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c @@ -42,7 +42,7 @@ static inline uint16_t _calc_csum(gnrc_pktsnip_t *hdr, uint16_t len = (uint16_t)hdr->size; while (payload && (payload != hdr)) { - csum = inet_csum(csum, payload->data, payload->size); + csum = inet_csum_slice(csum, payload->data, payload->size, len); len += (uint16_t)payload->size; payload = payload->next; } diff --git a/sys/net/gnrc/transport_layer/udp/gnrc_udp.c b/sys/net/gnrc/transport_layer/udp/gnrc_udp.c index e630d3d088..28034a5380 100644 --- a/sys/net/gnrc/transport_layer/udp/gnrc_udp.c +++ b/sys/net/gnrc/transport_layer/udp/gnrc_udp.c @@ -69,7 +69,7 @@ static uint16_t _calc_csum(gnrc_pktsnip_t *hdr, gnrc_pktsnip_t *pseudo_hdr, /* process the payload */ while (payload && payload != hdr) { - csum = inet_csum(csum, (uint8_t *)(payload->data), payload->size); + csum = inet_csum_slice(csum, (uint8_t *)(payload->data), payload->size, len); len += (uint16_t)payload->size; payload = payload->next; }