dhcpv4: make the cookie explicit in struct dhcpv4_message

This makes the struct more explicit while allowing a number of magic offsets to
be removed.

Note that adding the packed attribute to struct dhcpv4_message is necessary
because the incoming data is cast directly to the struct. Adding the attribute
will give the struct an alignment of 1 instead of 4 (the normal alignment of
struct in_addr and uint32_t).

In the beginning of dhcpv4_handle_msg(), we cast "void *data" to struct
dhcpv4_message, but we can't know the alignment of "*data" (meaning it has
alignment 1). And by casting "*data" to a struct, we're actually promising the
compiler that "*data" has the same alignment as the struct.

Later, the address of members of struct dhcpv4_message are passed to other
functions, but since struct dhcpv4_message can be at any memory location, it's
not a given that the pointers are aligned. This issue only becomes visible once
the packed attribute is added, which is why the temporary (aligned) struct
in_addr variables are added in the WITH_UBUS blocks.

Signed-off-by: David Härdeman <[email protected]>
Link: https://github.com/openwrt/odhcpd/pull/266
Signed-off-by: Álvaro Fernández Rojas <[email protected]>

diff --git a/src/dhcpv4.c b/src/dhcpv4.c
index f1e910617f6715562586aea90c44f156de019dc1..4304d94bdad0232e89719af3df1c80bbc78fa39c 100644 (file)
--- a/src/dhcpv4.c
+++ b/src/dhcpv4.c
@@ -170,7 +170,7 @@ static void dhcpv4_fr_send(struct dhcp_assignment *a)
                .chaddr = { 0 },
                .sname = { 0 },
                .file = { 0 },
-               .options = { DHCPV4_MAGIC_COOKIE },
+               .cookie = htonl(DHCPV4_MAGIC_COOKIE),
        };
        struct dhcpv4_auth_forcerenew *auth_o, auth = {
                .protocol = DHCPV4_AUTH_PROTO_RKAP,
@@ -181,7 +181,7 @@ static void dhcpv4_fr_send(struct dhcp_assignment *a)
                .key = { 0 },
        };
        struct interface *iface = a->iface;
-       uint8_t *cursor = &fr_msg.options[DHCPV4_MAGIC_COOKIE_LEN];
+       uint8_t *cursor = fr_msg.options;
        uint8_t msg = DHCPV4_MSG_FORCERENEW;
        struct sockaddr_in dest = {
                .sin_family = AF_INET,
@@ -564,8 +564,10 @@ void dhcpv4_handle_msg(void *addr, void *data, size_t len,
        if (iface->dhcpv4 == MODE_DISABLED)
                return;
 
-       if (len < offsetof(struct dhcpv4_message, options) + 4 ||
-                       req->op != DHCPV4_OP_BOOTREQUEST || req->hlen != ETH_ALEN)
+       /* FIXME: would checking the magic cookie value here break any clients? */
+
+       if (len < offsetof(struct dhcpv4_message, options) ||
+           req->op != DHCPV4_OP_BOOTREQUEST || req->hlen != ETH_ALEN)
                return;
 
        syslog(LOG_DEBUG, "Got DHCPv4 request on %s", iface->name);
@@ -588,11 +590,11 @@ void dhcpv4_handle_msg(void *addr, void *data, size_t len,
                .ciaddr = { INADDR_ANY },
                .giaddr = req->giaddr,
                .siaddr = iface->dhcpv4_local,
-               .options = { DHCPV4_MAGIC_COOKIE },
+               .cookie = htonl(DHCPV4_MAGIC_COOKIE),
        };
        memcpy(reply.chaddr, req->chaddr, sizeof(reply.chaddr));
 
-       uint8_t *cursor = &reply.options[DHCPV4_MAGIC_COOKIE_LEN];
+       uint8_t *cursor = reply.options;
        uint8_t reqmsg = DHCPV4_MSG_REQUEST;
        uint8_t msg = DHCPV4_MSG_ACK;
 
@@ -605,9 +607,10 @@ void dhcpv4_handle_msg(void *addr, void *data, size_t len,
        bool accept_fr_nonce = false;
        bool incl_fr_opt = false;
 
-       uint8_t *start = &req->options[4];
-       uint8_t *end = ((uint8_t*)data) + len;
+       uint8_t *start = req->options;
+       uint8_t *end = ((uint8_t *)data) + len;
        struct dhcpv4_option *opt;
+
        dhcpv4_for_each_option(start, end, opt) {
                if (opt->type == DHCPV4_OPT_MESSAGE && opt->len == 1)
                        reqmsg = opt->data[0];
@@ -701,10 +704,13 @@ void dhcpv4_handle_msg(void *addr, void *data, size_t len,
                        odhcpd_print_mac(req->chaddr, req->hlen), iface->name);
 
 #ifdef WITH_UBUS
-       if (reqmsg == DHCPV4_MSG_RELEASE)
+       if (reqmsg == DHCPV4_MSG_RELEASE) {
+               struct in_addr ciaddr = req->ciaddr; // ensure pointer alignment
                ubus_bcast_dhcp_event("dhcp.release", req->chaddr, req->hlen,
-                                       &req->ciaddr, a ? a->hostname : NULL, iface->ifname);
+                                       &ciaddr, a ? a->hostname : NULL, iface->ifname);
+       }
 #endif
+
        if (reqmsg == DHCPV4_MSG_DECLINE || reqmsg == DHCPV4_MSG_RELEASE)
                return;
 
@@ -927,11 +933,12 @@ void dhcpv4_handle_msg(void *addr, void *data, size_t len,
                        "ff:ff:ff:ff:ff:ff": odhcpd_print_mac(req->chaddr, req->hlen),
                        inet_ntoa(dest.sin_addr));
 
-
 #ifdef WITH_UBUS
-       if (msg == DHCPV4_MSG_ACK)
-               ubus_bcast_dhcp_event("dhcp.ack", req->chaddr, req->hlen, &reply.yiaddr,
+       if (msg == DHCPV4_MSG_ACK) {
+               struct in_addr yiaddr = reply.yiaddr; // ensure pointer alignment
+               ubus_bcast_dhcp_event("dhcp.ack", req->chaddr, req->hlen, &yiaddr,
                                        a ? a->hostname : NULL, iface->ifname);
+       }
 #endif
 }
 

diff --git a/src/dhcpv4.h b/src/dhcpv4.h
index 36fc1a79a936339294ad326e2e2045575268bb7a..e7e485e7abcce20fa9abcece66ef3073fb56b627 100644 (file)
--- a/src/dhcpv4.h
+++ b/src/dhcpv4.h
@@ -92,12 +92,12 @@ struct dhcpv4_message {
        uint8_t chaddr[16];
        char sname[64];
        char file[128];
-       uint8_t options[312];
-};
+       uint32_t cookie;
+       uint8_t options[308];
+} _packed;
 
 // RFC2131, §3
-#define DHCPV4_MAGIC_COOKIE 99, 130, 83, 99
-#define DHCPV4_MAGIC_COOKIE_LEN 4
+#define DHCPV4_MAGIC_COOKIE 0x63825363
 
 // RFC3203, §6; RFC3118, §2; RFC6704, §3.1.2
 struct dhcpv4_auth_forcerenew {