From: David Härdeman Date: Thu, 25 Sep 2025 13:33:31 +0000 (+0200) Subject: dhcpv4: make the cookie explicit in struct dhcpv4_message X-Git-Url: http://git.openwrt.org/?a=commitdiff_plain;h=f26abfdd27c0550ea233c4987c19d50abbc3dcf1;p=project%2Fodhcpd.git 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 Link: https://github.com/openwrt/odhcpd/pull/266 Signed-off-by: Álvaro Fernández Rojas --- diff --git a/src/dhcpv4.c b/src/dhcpv4.c index f1e9106..4304d94 100644 --- 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 36fc1a7..e7e485e 100644 --- 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 {