We have a winner here. I tested from INIT through to REBINDING and the
behavior matches what's in the RFC now. ok joel@
One cosmetic thing I noticed this time around: log_dhcp_hdr() in
engine.c should be printing dhcp_hdr->xid in host byte order.
.joel
On Fri, Dec 3, 2021 at 5:21 AM Florian Obser <flor...@openbsd.org> wrote:
>
> Last one, Joel spotted one more bug in the previous one, I was missing
> an assignment to dhcp_server in the ACK case.
>
> This is a rewrite of the package construction logic to send the correct
> information in the right states:
>
> RFC 2131 4.3.6, Table 4
> ---------------------------------------------------------------------
> | |REBOOTING |REQUESTING |RENEWING |REBINDING |
> ---------------------------------------------------------------------
> |broad/unicast |broadcast |broadcast |unicast |broadcast |
> |server-ip |MUST NOT |MUST |MUST NOT |MUST NOT |
> |requested-ip |MUST |MUST |MUST NOT |MUST NOT |
> |ciaddr |zero |zero |IP address |IP address|
> ---------------------------------------------------------------------
>
> Previous this logic was all over the place and difficult to check.
> This diff moves it all into the engine.c functions
> request_dhcp_discover() and request_dhcp_request().
>
> The frontend then leaves out dhcp options that are set to INADDR_ARPA,
> but it does not need to know in which state the engine is.
>
> OK?
>
>
> diff --git dhcpleased.h dhcpleased.h
> index b3b4938ad3c..6df38be6f5b 100644
> --- dhcpleased.h
> +++ dhcpleased.h
> @@ -287,15 +287,10 @@ struct imsg_dhcp {
> uint8_t packet[1500];
> };
>
> -
> -struct imsg_req_discover {
> - uint32_t if_index;
> - uint32_t xid;
> -};
> -
> -struct imsg_req_request {
> +struct imsg_req_dhcp {
> uint32_t if_index;
> uint32_t xid;
> + struct in_addr ciaddr;
> struct in_addr requested_ip;
> struct in_addr server_identifier;
> struct in_addr dhcp_server;
> diff --git engine.c engine.c
> index 17e65fbe789..3169bd80d40 100644
> --- engine.c
> +++ engine.c
> @@ -1170,6 +1170,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct
> imsg_dhcp *dhcp)
> return;
> }
> iface->server_identifier.s_addr = server_identifier.s_addr;
> + iface->dhcp_server.s_addr = server_identifier.s_addr;
> iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
> state_transition(iface, IF_REQUESTING);
> break;
> @@ -1228,6 +1229,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct
> imsg_dhcp *dhcp)
>
> clock_gettime(CLOCK_MONOTONIC, &iface->request_time);
> iface->server_identifier.s_addr = server_identifier.s_addr;
> + iface->dhcp_server.s_addr = server_identifier.s_addr;
> iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
> iface->mask.s_addr = subnet_mask.s_addr;
> #ifndef SMALL
> @@ -1354,11 +1356,8 @@ state_transition(struct dhcpleased_iface *iface, enum
> if_state new_state)
> case IF_REBOOTING:
> if (old_state == IF_REBOOTING)
> iface->timo.tv_sec *= 2;
> - else {
> - /* make sure we send broadcast */
> - iface->dhcp_server.s_addr = INADDR_ANY;
> + else
> iface->timo.tv_sec = START_EXP_BACKOFF;
> - }
> request_dhcp_request(iface);
> break;
> case IF_REQUESTING:
> @@ -1377,10 +1376,6 @@ state_transition(struct dhcpleased_iface *iface, enum
> if_state new_state)
> break;
> case IF_RENEWING:
> if (old_state == IF_BOUND) {
> - iface->dhcp_server.s_addr =
> - iface->server_identifier.s_addr;
> - iface->server_identifier.s_addr = INADDR_ANY;
> -
> iface->timo.tv_sec = (iface->rebinding_time -
> iface->renewal_time) / 2; /* RFC 2131 4.4.5 */
> } else
> @@ -1392,7 +1387,6 @@ state_transition(struct dhcpleased_iface *iface, enum
> if_state new_state)
> break;
> case IF_REBINDING:
> if (old_state == IF_RENEWING) {
> - iface->dhcp_server.s_addr = INADDR_ANY;
> iface->timo.tv_sec = (iface->lease_time -
> iface->rebinding_time) / 2; /* RFC 2131 4.4.5 */
> } else
> @@ -1470,28 +1464,90 @@ iface_timeout(int fd, short events, void *arg)
> void
> request_dhcp_discover(struct dhcpleased_iface *iface)
> {
> - struct imsg_req_discover imsg_req_discover;
> + struct imsg_req_dhcp imsg;
>
> - imsg_req_discover.if_index = iface->if_index;
> - imsg_req_discover.xid = iface->xid = arc4random();
> - engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0,
> &imsg_req_discover,
> - sizeof(imsg_req_discover));
> + memset(&imsg, 0, sizeof(imsg));
> +
> + iface->xid = arc4random();
> + imsg.if_index = iface->if_index;
> + imsg.xid = iface->xid;
> +
> + /*
> + * similar to RFC 2131 4.3.6, Table 4 for DHCPDISCOVER
> + * ------------------------------
> + * | | INIT |
> + * ------------------------------
> + * |broad/unicast | broadcast |
> + * |server-ip | MUST NOT |
> + * |requested-ip | MAY |
> + * |ciaddr | zero |
> + * ------------------------------
> + *
> + * Leaving everything at 0 from the memset results in this table with
> + * requested-ip not set.
> + */
> +
> + engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0, &imsg,
> sizeof(imsg));
> }
>
> void
> request_dhcp_request(struct dhcpleased_iface *iface)
> {
> - struct imsg_req_request imsg_req_request;
> + struct imsg_req_dhcp imsg;
>
> iface->xid = arc4random();
> - imsg_req_request.if_index = iface->if_index;
> - imsg_req_request.xid = iface->xid;
> - imsg_req_request.server_identifier.s_addr =
> - iface->server_identifier.s_addr;
> - imsg_req_request.requested_ip.s_addr = iface->requested_ip.s_addr;
> - imsg_req_request.dhcp_server.s_addr = iface->dhcp_server.s_addr;
> - engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg_req_request,
> - sizeof(imsg_req_request));
> + imsg.if_index = iface->if_index;
> + imsg.xid = iface->xid;
> +
> + /*
> + * RFC 2131 4.3.6, Table 4
> + *
> ---------------------------------------------------------------------
> + * | |REBOOTING |REQUESTING |RENEWING
> |REBINDING |
> + *
> ---------------------------------------------------------------------
> + * |broad/unicast |broadcast |broadcast |unicast
> |broadcast |
> + * |server-ip |MUST NOT |MUST |MUST NOT |MUST NOT
> |
> + * |requested-ip |MUST |MUST |MUST NOT |MUST NOT
> |
> + * |ciaddr |zero |zero |IP address |IP
> address|
> + *
> ---------------------------------------------------------------------
> + */
> + switch (iface->state) {
> + case IF_DOWN:
> + fatalx("invalid state IF_DOWN in %s", __func__);
> + break;
> + case IF_INIT:
> + fatalx("invalid state IF_INIT in %s", __func__);
> + break;
> + case IF_BOUND:
> + fatalx("invalid state IF_BOUND in %s", __func__);
> + break;
> + case IF_REBOOTING:
> + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast
> */
> + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT */
> + imsg.requested_ip = iface->requested_ip; /* MUST */
> + imsg.ciaddr.s_addr = INADDR_ANY; /* zero */
> + break;
> + case IF_REQUESTING:
> + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast
> */
> + imsg.server_identifier =
> + iface->server_identifier; /* MUST */
> + imsg.requested_ip = iface->requested_ip; /* MUST */
> + imsg.ciaddr.s_addr = INADDR_ANY; /* zero */
> + break;
> + case IF_RENEWING:
> + imsg.dhcp_server = iface->dhcp_server; /* unicast */
> + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT */
> + imsg.requested_ip.s_addr = INADDR_ANY; /* MUST NOT */
> + imsg.ciaddr = iface->requested_ip; /* IP address
> */
> + break;
> + case IF_REBINDING:
> + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast
> */
> + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT */
> + imsg.requested_ip.s_addr = INADDR_ANY; /* MUST NOT */
> + imsg.ciaddr = iface->requested_ip; /* IP address
> */
> + break;
> + }
> +
> + engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg,
> sizeof(imsg));
> }
>
> void
> diff --git frontend.c frontend.c
> index bd1e37fe952..e2c626ce5d2 100644
> --- frontend.c
> +++ frontend.c
> @@ -70,6 +70,7 @@ struct iface {
> struct imsg_ifinfo ifinfo;
> int send_discover;
> uint32_t xid;
> + struct in_addr ciaddr;
> struct in_addr requested_ip;
> struct in_addr server_identifier;
> struct in_addr dhcp_server;
> @@ -90,10 +91,10 @@ int get_xflags(char *);
> struct iface *get_iface_by_id(uint32_t);
> void remove_iface(uint32_t);
> void set_bpfsock(int, uint32_t);
> +void iface_data_from_imsg(struct iface*, struct imsg_req_dhcp *);
> ssize_t build_packet(uint8_t, char *, uint32_t, struct
> ether_addr *,
> - struct in_addr *, struct in_addr *);
> -void send_discover(struct iface *);
> -void send_request(struct iface *);
> + struct in_addr *, struct in_addr *, struct in_addr *);
> +void send_packet(uint8_t, struct iface *);
> void bpf_send_packet(struct iface *, uint8_t *, ssize_t);
> int udp_send_packet(struct iface *, uint8_t *, ssize_t);
> #ifndef SMALL
> @@ -480,39 +481,39 @@ frontend_dispatch_engine(int fd, short event, void
> *bula)
> break;
> #endif /* SMALL */
> case IMSG_SEND_DISCOVER: {
> - struct imsg_req_discover imsg_req_discover;
> - if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_discover))
> + struct imsg_req_dhcp imsg_req_dhcp;
> + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
> fatalx("%s: IMSG_SEND_DISCOVER wrong "
> "length: %lu", __func__,
> IMSG_DATA_SIZE(imsg));
> - memcpy(&imsg_req_discover, imsg.data,
> - sizeof(imsg_req_discover));
> - iface = get_iface_by_id(imsg_req_discover.if_index);
> - if (iface != NULL) {
> - iface->xid = imsg_req_discover.xid;
> - send_discover(iface);
> - }
> + memcpy(&imsg_req_dhcp, imsg.data,
> + sizeof(imsg_req_dhcp));
> +
> + iface = get_iface_by_id(imsg_req_dhcp.if_index);
> +
> + if (iface == NULL)
> + break;
> +
> + iface_data_from_imsg(iface, &imsg_req_dhcp);
> + send_packet(DHCPDISCOVER, iface);
> break;
> }
> case IMSG_SEND_REQUEST: {
> - struct imsg_req_request imsg_req_request;
> - if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_request))
> + struct imsg_req_dhcp imsg_req_dhcp;
> + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
> fatalx("%s: IMSG_SEND_REQUEST wrong "
> "length: %lu", __func__,
> IMSG_DATA_SIZE(imsg));
> - memcpy(&imsg_req_request, imsg.data,
> - sizeof(imsg_req_request));
> - iface = get_iface_by_id(imsg_req_request.if_index);
> - if (iface != NULL) {
> - iface->xid = imsg_req_request.xid;
> - iface->requested_ip.s_addr =
> - imsg_req_request.requested_ip.s_addr;
> - iface->server_identifier.s_addr =
> - imsg_req_request.server_identifier.s_addr;
> - iface->dhcp_server.s_addr =
> - imsg_req_request.dhcp_server.s_addr;
> - send_request(iface);
> - }
> + memcpy(&imsg_req_dhcp, imsg.data,
> + sizeof(imsg_req_dhcp));
> +
> + iface = get_iface_by_id(imsg_req_dhcp.if_index);
> +
> + if (iface == NULL)
> + break;
> +
> + iface_data_from_imsg(iface, &imsg_req_dhcp);
> + send_packet(DHCPREQUEST, iface);
> break;
> }
> default:
> @@ -885,10 +886,20 @@ bpf_receive(int fd, short events, void *arg)
> }
> }
>
> +void
> +iface_data_from_imsg(struct iface* iface, struct imsg_req_dhcp *imsg)
> +{
> + iface->xid = imsg->xid;
> + iface->ciaddr.s_addr = imsg->ciaddr.s_addr;
> + iface->requested_ip.s_addr = imsg->requested_ip.s_addr;
> + iface->server_identifier.s_addr = imsg->server_identifier.s_addr;
> + iface->dhcp_server.s_addr = imsg->dhcp_server.s_addr;
> +}
> +
> ssize_t
> build_packet(uint8_t message_type, char *if_name, uint32_t xid,
> - struct ether_addr *hw_address, struct in_addr *requested_ip,
> - struct in_addr *server_identifier)
> + struct ether_addr *hw_address, struct in_addr *ciaddr, struct in_addr
> + *requested_ip, struct in_addr *server_identifier)
> {
> static uint8_t dhcp_cookie[] = DHCP_COOKIE;
> static uint8_t dhcp_message_type[] = {DHO_DHCP_MESSAGE_TYPE, 1,
> @@ -926,6 +937,7 @@ build_packet(uint8_t message_type, char *if_name,
> uint32_t xid,
> hdr->hops = 0;
> hdr->xid = xid;
> hdr->secs = 0;
> + hdr->ciaddr.s_addr = ciaddr->s_addr;
> memcpy(hdr->chaddr, hw_address, sizeof(*hw_address));
> p += sizeof(struct dhcp_hdr);
> memcpy(p, dhcp_cookie, sizeof(dhcp_cookie));
> @@ -966,20 +978,20 @@ build_packet(uint8_t message_type, char *if_name,
> uint32_t xid,
> memcpy(p, dhcp_req_list, sizeof(dhcp_req_list));
> p += sizeof(dhcp_req_list);
>
> - if (message_type == DHCPREQUEST) {
> + if (requested_ip->s_addr != INADDR_ANY) {
> memcpy(dhcp_requested_address + 2, requested_ip,
> sizeof(*requested_ip));
> memcpy(p, dhcp_requested_address,
> sizeof(dhcp_requested_address));
> p += sizeof(dhcp_requested_address);
> + }
>
> - if (server_identifier->s_addr != INADDR_ANY) {
> - memcpy(dhcp_server_identifier + 2, server_identifier,
> - sizeof(*server_identifier));
> - memcpy(p, dhcp_server_identifier,
> - sizeof(dhcp_server_identifier));
> - p += sizeof(dhcp_server_identifier);
> - }
> + if (server_identifier->s_addr != INADDR_ANY) {
> + memcpy(dhcp_server_identifier + 2, server_identifier,
> + sizeof(*server_identifier));
> + memcpy(p, dhcp_server_identifier,
> + sizeof(dhcp_server_identifier));
> + p += sizeof(dhcp_server_identifier);
> }
>
> *p = DHO_END;
> @@ -995,7 +1007,7 @@ build_packet(uint8_t message_type, char *if_name,
> uint32_t xid,
> }
>
> void
> -send_discover(struct iface *iface)
> +send_packet(uint8_t message_type, struct iface *iface)
> {
> ssize_t pkt_len;
> char ifnamebuf[IF_NAMESIZE], *if_name;
> @@ -1004,27 +1016,17 @@ send_discover(struct iface *iface)
> iface->send_discover = 1;
> return;
> }
> - iface->send_discover = 0;
> -
> - if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
> - log_debug("DHCPDISCOVER on %s", if_name == NULL ? "?" : if_name);
>
> - pkt_len = build_packet(DHCPDISCOVER, if_name, iface->xid,
> - &iface->ifinfo.hw_address, &iface->requested_ip, NULL);
> - bpf_send_packet(iface, dhcp_packet, pkt_len);
> -}
> + iface->send_discover = 0;
>
> -void
> -send_request(struct iface *iface)
> -{
> - ssize_t pkt_len;
> - char ifnamebuf[IF_NAMESIZE], *if_name;
> + if ((if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf)) ==
> NULL)
> + return; /* iface went away, nothing to do */
>
> - if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
> - log_debug("DHCPREQUEST on %s", if_name == NULL ? "?" : if_name);
> + log_debug("%s on %s", message_type == DHCPDISCOVER ? "DHCPDISCOVER" :
> + "DHCPREQUEST", if_name);
>
> - pkt_len = build_packet(DHCPREQUEST, if_name, iface->xid,
> - &iface->ifinfo.hw_address, &iface->requested_ip,
> + pkt_len = build_packet(message_type, if_name, iface->xid,
> + &iface->ifinfo.hw_address, &iface->ciaddr, &iface->requested_ip,
> &iface->server_identifier);
> if (iface->dhcp_server.s_addr != INADDR_ANY) {
> if (udp_send_packet(iface, dhcp_packet, pkt_len) == -1)
> @@ -1162,7 +1164,7 @@ set_bpfsock(int bpfsock, uint32_t if_index)
> EV_PERSIST, bpf_receive, iface);
> event_add(&iface->bpfev.ev, NULL);
> if (iface->send_discover)
> - send_discover(iface);
> + send_packet(DHCPDISCOVER, iface);
> }
> }
>
>
>
> --
> I'm not entirely sure you are real.
>
