BUG/MEDIUM: quic: don't blindly rely on unaligned accesses

There are several places where the QUIC low-level code performs unaligned
accesses by casting unaligned char* pointers to uint32_t, but this is
totally forbidden as it only works on machines that support unaligned
accesses, and either crashes on other ones (SPARC, MIPS), can result in
reading garbage (ARMv5) or be very slow due to the access being emulated
(RISC-V). We do have functions for this, such as read_u32() and write_u32()
that rely on the compiler's knowledge of the machine's capabilities to
either perform an unaligned access or do it one byte at a time.

This must be backported at least as far as 2.6. Some of the code moved a
few times since, so in order to figure the points that need to be fixed,
one may look for a forced pointer cast without having verified that either
the machine is compatible or that the pointer is aligned using this:

  $ git grep 'uint[36][24]_t \*)'

Or build and run the code on a MIPS or SPARC and perform requests using
curl to see if they work or crash with a bus error. All the places fixed
in this commit were found thanks to an immediate crash on the first
request.

This was tagged medium because the affected archs are not the most common
ones where QUIC will be found these days.

diff --git a/src/quic_retry.c b/src/quic_retry.c
index 1c58e5e835c6f8712efadfb6da7cbde196d2e46a..f1d55b8781dcadeaeccc88cc67e64915c6a19e1a 100644 (file)
--- a/src/quic_retry.c
+++ b/src/quic_retry.c
@@ -60,7 +60,7 @@ static int quic_generate_retry_token_aad(unsigned char *aad,
        unsigned char *p;
 
        p = aad;
-       *(uint32_t *)p = htonl(version);
+       write_u32(p, htonl(version));
        p += sizeof version;
        p += quic_saddr_cpy(p, addr);
        memcpy(p, cid->data, cid->len);

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 8612c3f00707974658e35b90a151b4b9e31a2993..3645f1673d43f7295aca182e6dc41eb26b4c71fd 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -1462,7 +1462,7 @@ static inline int quic_read_uint32(uint32_t *val,
        if (end - *buf < sizeof *val)
                return 0;
 
-       *val = ntohl(*(uint32_t *)*buf);
+       *val = ntohl(read_u32(*buf));
        *buf += sizeof *val;
 
        return 1;

diff --git a/src/quic_tp.c b/src/quic_tp.c
index d13401478cdbe3edee438947c9e68caa90459316..08d24b28f4701c863bf7adcf021c41c7e520c112 100644 (file)
--- a/src/quic_tp.c
+++ b/src/quic_tp.c
@@ -171,23 +171,23 @@ static int quic_transport_param_dec_version_info(struct tp_version_information *
                                                  const unsigned char *end, int server)
 {
        size_t tp_len = end - *buf;
-       const uint32_t *ver, *others;
+       const unsigned char *ver, *others;
 
        /* <tp_len> must be a multiple of sizeof(uint32_t) */
        if (tp_len < sizeof tp->chosen || (tp_len & 0x3))
                return 0;
 
-       tp->chosen = ntohl(*(uint32_t *)*buf);
+       tp->chosen = ntohl(read_u32(*buf));
        /* Must not be null */
        if (!tp->chosen)
                return 0;
 
        *buf += sizeof tp->chosen;
-       others = (const uint32_t *)*buf;
+       others = *buf;
 
        /* Others versions must not be null */
-       for (ver = others; ver < (const uint32_t *)end; ver++) {
-               if (!*ver)
+       for (ver = others; ver < end; ver += 4) {
+               if (!read_u32(ver))
                        return 0;
        }
 
@@ -195,19 +195,19 @@ static int quic_transport_param_dec_version_info(struct tp_version_information *
                /* TODO: not supported */
                return 0;
 
-       for (ver = others; ver < (const uint32_t *)end; ver++) {
+       for (ver = others; ver < end; ver += 4) {
                if (!tp->negotiated_version) {
                        int i;
 
                        for (i = 0; i < quic_versions_nb; i++) {
-                               if (ntohl(*ver) == quic_versions[i].num) {
+                               if (ntohl(read_u32(ver)) == quic_versions[i].num) {
                                        tp->negotiated_version = &quic_versions[i];
                                        break;
                                }
                        }
                }
 
-               if (preferred_version && ntohl(*ver) == preferred_version->num) {
+               if (preferred_version && ntohl(read_u32(ver)) == preferred_version->num) {
                        tp->negotiated_version = preferred_version;
                        goto out;
                }

diff --git a/src/quic_tx.c b/src/quic_tx.c
index f9f021cfceeac2abc10bdcb58bb3652c3d530573..94646a68d22ce1cb6d243a5f96f3666b8803e42b 100644 (file)
--- a/src/quic_tx.c
+++ b/src/quic_tx.c
@@ -1323,7 +1323,7 @@ static inline int quic_write_uint32(unsigned char **buf,
        if (end - *buf < sizeof val)
                return 0;
 
-       *(uint32_t *)*buf = htonl(val);
+       write_u32(*buf, htonl(val));
        *buf += sizeof val;
 
        return 1;