From: Florian Obser Subject: slaacd(8): do not peek inside of struct imsg To: tech Date: Fri, 23 Aug 2024 17:36:59 +0200 Two commits on top of each other, first introduce i2s to simplify imsg type logging (inspired by smtpd(8)). The second diff switches to imsg_get_data to fetch data out of the imsg and validate sizes. I always wanted to simplify the imsg logging but I was worried about the churn. Now is a good time since I have to touch everything anyway. OK? commit 0d133cb516f2323c3ee0e025acc133be7f9ae8bc Author: Florian Obser Date: Fri Aug 23 17:07:59 2024 +0200 Helper function for logging imsg type names. diff --git slaacd.c slaacd.c index 05af06e68de..c6c84d25e2c 100644 --- slaacd.c +++ slaacd.c @@ -899,3 +899,54 @@ open_icmp6sock(int rdomain) main_imsg_compose_frontend(IMSG_ICMP6SOCK, icmp6sock, &rdomain, sizeof(rdomain)); } + +#ifndef SMALL + +#define I2S(x) case x: return #x + +const char* +i2s(uint32_t type) +{ + static char unknown[sizeof("IMSG_4294967295")]; + + switch (type) { + I2S(IMSG_NONE); + I2S(IMSG_CTL_LOG_VERBOSE); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_RA); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_RA_PREFIX); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_RA_RDNS); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_ADDR_PROPOSALS); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_ADDR_PROPOSAL); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_DFR_PROPOSALS); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_DFR_PROPOSAL); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_RDNS_PROPOSALS); + I2S(IMSG_CTL_SHOW_INTERFACE_INFO_RDNS_PROPOSAL); + I2S(IMSG_CTL_END); + I2S(IMSG_PROPOSE_RDNS); + I2S(IMSG_REPROPOSE_RDNS); + I2S(IMSG_CTL_SEND_SOLICITATION); + I2S(IMSG_SOCKET_IPC); + I2S(IMSG_OPEN_ICMP6SOCK); + I2S(IMSG_ICMP6SOCK); + I2S(IMSG_ROUTESOCK); + I2S(IMSG_CONTROLFD); + I2S(IMSG_STARTUP); + I2S(IMSG_UPDATE_IF); + I2S(IMSG_REMOVE_IF); + I2S(IMSG_RA); + I2S(IMSG_CONFIGURE_ADDRESS); + I2S(IMSG_WITHDRAW_ADDRESS); + I2S(IMSG_DEL_ADDRESS); + I2S(IMSG_DEL_ROUTE); + I2S(IMSG_CONFIGURE_DFR); + I2S(IMSG_WITHDRAW_DFR); + I2S(IMSG_DUP_ADDRESS); + default: + snprintf(unknown, sizeof(unknown), "IMSG_%u", type); + return unknown; + } +} +#undef I2S + +#endif /* SMALL */ diff --git slaacd.h slaacd.h index 2844f4a63b7..910392bfaf2 100644 --- slaacd.h +++ slaacd.h @@ -206,6 +206,8 @@ int imsg_compose_event(struct imsgev *, uint16_t, uint32_t, pid_t, int, void *, uint16_t); #ifndef SMALL const char *sin6_to_str(struct sockaddr_in6 *); +const char *i2s(uint32_t); #else #define sin6_to_str(x...) "" +#define i2s(x...) "" #endif /* SMALL */ commit ad9dff9d8cdbad3151933969eb155e0b0d61384d Author: Florian Obser Date: Fri Aug 23 17:22:42 2024 +0200 Do not peek inside of struct imsg. While here use i2s helper function for error logging. diff --git control.c control.c index bd3f2290f95..11a4f2ee608 100644 --- control.c +++ control.c @@ -225,6 +225,8 @@ control_dispatch_imsg(int fd, short event, void *bula) struct imsg imsg; ssize_t n; int verbose; + uint32_t if_index, type; + pid_t pid; if ((c = control_connbyfd(fd)) == NULL) { log_warnx("%s: fd %d: not found", __func__, fd); @@ -253,37 +255,34 @@ control_dispatch_imsg(int fd, short event, void *bula) if (n == 0) break; - switch (imsg.hdr.type) { + type = imsg_get_type(&imsg); + pid = imsg_get_pid(&imsg); + switch (type) { case IMSG_CTL_LOG_VERBOSE: - if (IMSG_DATA_SIZE(imsg) != sizeof(verbose)) + if (imsg_get_data(&imsg, &verbose, + sizeof(verbose)) == -1) break; /* Forward to all other processes. */ - frontend_imsg_compose_main(imsg.hdr.type, imsg.hdr.pid, - imsg.data, IMSG_DATA_SIZE(imsg)); - frontend_imsg_compose_engine(imsg.hdr.type, 0, - imsg.hdr.pid, imsg.data, IMSG_DATA_SIZE(imsg)); + frontend_imsg_compose_main(type, pid, &verbose, + sizeof(verbose)); + frontend_imsg_compose_engine(type, 0, pid, &verbose, + sizeof(verbose)); - memcpy(&verbose, imsg.data, sizeof(verbose)); log_setverbose(verbose); break; case IMSG_CTL_SHOW_INTERFACE_INFO: - if (IMSG_DATA_SIZE(imsg) != sizeof(uint32_t)) - break; - c->iev.ibuf.pid = imsg.hdr.pid; - frontend_imsg_compose_engine(imsg.hdr.type, 0, - imsg.hdr.pid, imsg.data, IMSG_DATA_SIZE(imsg)); - break; case IMSG_CTL_SEND_SOLICITATION: - if (IMSG_DATA_SIZE(imsg) != sizeof(uint32_t)) + if (imsg_get_data(&imsg, &if_index, + sizeof(if_index)) == -1) break; - c->iev.ibuf.pid = imsg.hdr.pid; - frontend_imsg_compose_engine(imsg.hdr.type, 0, - imsg.hdr.pid, imsg.data, IMSG_DATA_SIZE(imsg)); + + c->iev.ibuf.pid = pid; + frontend_imsg_compose_engine(type, 0, pid, &if_index, + sizeof(if_index)); break; default: - log_debug("%s: error handling imsg %d", __func__, - imsg.hdr.type); + log_debug("%s: error handling imsg %d", __func__, type); break; } imsg_free(&imsg); diff --git engine.c engine.c index f50fd6a821e..818c2d75bf4 100644 --- engine.c +++ engine.c @@ -462,7 +462,7 @@ engine_dispatch_frontend(int fd, short event, void *bula) #ifndef SMALL int verbose; #endif /* SMALL */ - uint32_t if_index; + uint32_t if_index, type; if (event & EV_READ) { if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN) @@ -483,36 +483,36 @@ engine_dispatch_frontend(int fd, short event, void *bula) if (n == 0) /* No more messages. */ break; - switch (imsg.hdr.type) { + type = imsg_get_type(&imsg); + + switch (type) { #ifndef SMALL case IMSG_CTL_LOG_VERBOSE: - if (IMSG_DATA_SIZE(imsg) != sizeof(verbose)) - fatalx("%s: IMSG_CTL_LOG_VERBOSE wrong length: " - "%lu", __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&verbose, imsg.data, sizeof(verbose)); + if (imsg_get_data(&imsg, &verbose, + sizeof(verbose)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + log_setverbose(verbose); break; case IMSG_CTL_SHOW_INTERFACE_INFO: - if (IMSG_DATA_SIZE(imsg) != sizeof(if_index)) - fatalx("%s: IMSG_CTL_SHOW_INTERFACE_INFO wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - memcpy(&if_index, imsg.data, sizeof(if_index)); + if (imsg_get_data(&imsg, &if_index, + sizeof(if_index)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + engine_showinfo_ctl(&imsg, if_index); break; #endif /* SMALL */ case IMSG_REMOVE_IF: - if (IMSG_DATA_SIZE(imsg) != sizeof(if_index)) - fatalx("%s: IMSG_REMOVE_IF wrong length: %lu", - __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&if_index, imsg.data, sizeof(if_index)); + if (imsg_get_data(&imsg, &if_index, + sizeof(if_index)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + remove_slaacd_iface(if_index); break; case IMSG_RA: - if (IMSG_DATA_SIZE(imsg) != sizeof(ra)) - fatalx("%s: IMSG_RA wrong length: %lu", - __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&ra, imsg.data, sizeof(ra)); + if (imsg_get_data(&imsg, &ra, sizeof(ra)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + iface = get_slaacd_iface_by_id(ra.if_index); /* @@ -524,11 +524,10 @@ engine_dispatch_frontend(int fd, short event, void *bula) parse_ra(iface, &ra); break; case IMSG_CTL_SEND_SOLICITATION: - if (IMSG_DATA_SIZE(imsg) != sizeof(if_index)) - fatalx("%s: IMSG_CTL_SEND_SOLICITATION wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - memcpy(&if_index, imsg.data, sizeof(if_index)); + if (imsg_get_data(&imsg, &if_index, + sizeof(if_index)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + iface = get_slaacd_iface_by_id(if_index); if (iface == NULL) log_warnx("requested to send solicitation on " @@ -539,10 +538,10 @@ engine_dispatch_frontend(int fd, short event, void *bula) } break; case IMSG_DEL_ADDRESS: - if (IMSG_DATA_SIZE(imsg) != sizeof(del_addr)) - fatalx("%s: IMSG_DEL_ADDRESS wrong length: %lu", - __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&del_addr, imsg.data, sizeof(del_addr)); + if (imsg_get_data(&imsg, &del_addr, + sizeof(del_addr)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + iface = get_slaacd_iface_by_id(del_addr.if_index); if (iface == NULL) { log_debug("IMSG_DEL_ADDRESS: unknown interface" @@ -562,10 +561,10 @@ engine_dispatch_frontend(int fd, short event, void *bula) free_address_proposal(addr_proposal); break; case IMSG_DEL_ROUTE: - if (IMSG_DATA_SIZE(imsg) != sizeof(del_route)) - fatalx("%s: IMSG_DEL_ROUTE wrong length: %lu", - __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&del_route, imsg.data, sizeof(del_route)); + if (imsg_get_data(&imsg, &del_route, + sizeof(del_route)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + iface = get_slaacd_iface_by_id(del_route.if_index); if (iface == NULL) { log_debug("IMSG_DEL_ROUTE: unknown interface" @@ -582,10 +581,10 @@ engine_dispatch_frontend(int fd, short event, void *bula) } break; case IMSG_DUP_ADDRESS: - if (IMSG_DATA_SIZE(imsg) != sizeof(dup_addr)) - fatalx("%s: IMSG_DUP_ADDRESS wrong length: %lu", - __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&dup_addr, imsg.data, sizeof(dup_addr)); + if (imsg_get_data(&imsg, &dup_addr, + sizeof(dup_addr)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + iface = get_slaacd_iface_by_id(dup_addr.if_index); if (iface == NULL) { log_debug("IMSG_DUP_ADDRESS: unknown interface" @@ -606,8 +605,7 @@ engine_dispatch_frontend(int fd, short event, void *bula) iface->rdomain); break; default: - log_debug("%s: unexpected imsg %d", __func__, - imsg.hdr.type); + log_debug("%s: unexpected imsg %d", __func__, type); break; } imsg_free(&imsg); @@ -629,6 +627,7 @@ engine_dispatch_main(int fd, short event, void *bula) struct imsgbuf *ibuf = &iev->ibuf; struct imsg_ifinfo imsg_ifinfo; ssize_t n; + uint32_t type; int shut = 0; if (event & EV_READ) { @@ -650,7 +649,9 @@ engine_dispatch_main(int fd, short event, void *bula) if (n == 0) /* No more messages. */ break; - switch (imsg.hdr.type) { + type = imsg_get_type(&imsg); + + switch (type) { case IMSG_SOCKET_IPC: /* * Setup pipe and event handler to the frontend @@ -681,15 +682,14 @@ engine_dispatch_main(int fd, short event, void *bula) fatal("pledge"); break; case IMSG_UPDATE_IF: - if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_ifinfo)) - fatalx("%s: IMSG_UPDATE_IF wrong length: %lu", - __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&imsg_ifinfo, imsg.data, sizeof(imsg_ifinfo)); + if (imsg_get_data(&imsg, &imsg_ifinfo, + sizeof(imsg_ifinfo)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + engine_update_iface(&imsg_ifinfo); break; default: - log_debug("%s: unexpected imsg %d", __func__, - imsg.hdr.type); + log_debug("%s: unexpected imsg %d", __func__, type); break; } imsg_free(&imsg); diff --git frontend.c frontend.c index 7e5024a5c84..0b7659bac1d 100644 --- frontend.c +++ frontend.c @@ -282,6 +282,7 @@ frontend_dispatch_main(int fd, short event, void *bula) struct imsgev *iev = bula; struct imsgbuf *ibuf = &iev->ibuf; ssize_t n; + uint32_t type; int shut = 0, icmp6sock, rdomain; if (event & EV_READ) { @@ -303,7 +304,9 @@ frontend_dispatch_main(int fd, short event, void *bula) if (n == 0) /* No more messages. */ break; - switch (imsg.hdr.type) { + type = imsg_get_type(&imsg); + + switch (type) { case IMSG_SOCKET_IPC: /* * Setup pipe and event handler to the engine @@ -335,10 +338,10 @@ frontend_dispatch_main(int fd, short event, void *bula) fatalx("%s: expected to receive imsg " "ICMPv6 fd but didn't receive any", __func__); - if (IMSG_DATA_SIZE(imsg) != sizeof(rdomain)) - fatalx("%s: IMSG_ICMP6SOCK wrong length: " - "%lu", __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&rdomain, imsg.data, sizeof(rdomain)); + if (imsg_get_data(&imsg, &rdomain, + sizeof(rdomain)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + set_icmp6sock(icmp6sock, rdomain); break; case IMSG_ROUTESOCK: @@ -346,6 +349,7 @@ frontend_dispatch_main(int fd, short event, void *bula) fatalx("%s: expected to receive imsg " "routesocket fd but didn't receive any", __func__); + event_set(&ev_route, fd, EV_READ | EV_PERSIST, route_receive, NULL); break; @@ -358,6 +362,7 @@ frontend_dispatch_main(int fd, short event, void *bula) fatalx("%s: expected to receive imsg " "control fd but didn't receive any", __func__); + /* Listen on control socket. */ control_listen(fd); break; @@ -366,8 +371,7 @@ frontend_dispatch_main(int fd, short event, void *bula) break; #endif /* SMALL */ default: - log_debug("%s: error handling imsg %d", __func__, - imsg.hdr.type); + log_debug("%s: error handling imsg %d", __func__, type); break; } imsg_free(&imsg); @@ -389,7 +393,7 @@ frontend_dispatch_engine(int fd, short event, void *bula) struct imsg imsg; ssize_t n; int shut = 0; - uint32_t if_index; + uint32_t if_index, type; if (event & EV_READ) { if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN) @@ -410,7 +414,9 @@ frontend_dispatch_engine(int fd, short event, void *bula) if (n == 0) /* No more messages. */ break; - switch (imsg.hdr.type) { + type = imsg_get_type(&imsg); + + switch (type) { #ifndef SMALL case IMSG_CTL_END: case IMSG_CTL_SHOW_INTERFACE_INFO: @@ -427,16 +433,14 @@ frontend_dispatch_engine(int fd, short event, void *bula) break; #endif /* SMALL */ case IMSG_CTL_SEND_SOLICITATION: - if (IMSG_DATA_SIZE(imsg) != sizeof(if_index)) - fatalx("%s: IMSG_CTL_SEND_SOLICITATION wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - if_index = *((uint32_t *)imsg.data); + if (imsg_get_data(&imsg, &if_index, + sizeof(if_index)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + send_solicitation(if_index); break; default: - log_debug("%s: error handling imsg %d", __func__, - imsg.hdr.type); + log_debug("%s: error handling imsg %d", __func__, type); break; } imsg_free(&imsg); diff --git slaacd.c slaacd.c index c6c84d25e2c..43245529543 100644 --- slaacd.c +++ slaacd.c @@ -381,6 +381,7 @@ main_dispatch_frontend(int fd, short event, void *bula) struct imsg imsg; struct imsg_ifinfo imsg_ifinfo; ssize_t n; + uint32_t type; int shut = 0; int rdomain; #ifndef SMALL @@ -408,29 +409,30 @@ main_dispatch_frontend(int fd, short event, void *bula) if (n == 0) /* No more messages. */ break; - switch (imsg.hdr.type) { + type = imsg_get_type(&imsg); + + switch (type) { case IMSG_OPEN_ICMP6SOCK: - log_debug("IMSG_OPEN_ICMP6SOCK"); - if (IMSG_DATA_SIZE(imsg) != sizeof(rdomain)) - fatalx("%s: IMSG_OPEN_ICMP6SOCK wrong length: " - "%lu", __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&rdomain, imsg.data, sizeof(rdomain)); + if (imsg_get_data(&imsg, &rdomain, + sizeof(rdomain)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + open_icmp6sock(rdomain); break; #ifndef SMALL case IMSG_CTL_LOG_VERBOSE: - if (IMSG_DATA_SIZE(imsg) != sizeof(verbose)) - fatalx("%s: IMSG_CTL_LOG_VERBOSE wrong length: " - "%lu", __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&verbose, imsg.data, sizeof(verbose)); + if (imsg_get_data(&imsg, &verbose, + sizeof(verbose)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + log_setverbose(verbose); break; #endif /* SMALL */ case IMSG_UPDATE_IF: - if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_ifinfo)) - fatalx("%s: IMSG_UPDATE_IF wrong length: %lu", - __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&imsg_ifinfo, imsg.data, sizeof(imsg_ifinfo)); + if (imsg_get_data(&imsg, &imsg_ifinfo, + sizeof(imsg_ifinfo)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + if (get_soiikey(imsg_ifinfo.soiikey) == -1) log_warn("get_soiikey"); else @@ -438,8 +440,7 @@ main_dispatch_frontend(int fd, short event, void *bula) &imsg_ifinfo, sizeof(imsg_ifinfo)); break; default: - log_debug("%s: error handling imsg %d", __func__, - imsg.hdr.type); + log_debug("%s: error handling imsg %d", __func__, type); break; } imsg_free(&imsg); @@ -463,6 +464,7 @@ main_dispatch_engine(int fd, short event, void *bula) struct imsg_configure_dfr dfr; struct imsg_propose_rdns rdns; ssize_t n; + uint32_t type; int shut = 0; ibuf = &iev->ibuf; @@ -486,54 +488,47 @@ main_dispatch_engine(int fd, short event, void *bula) if (n == 0) /* No more messages. */ break; - switch (imsg.hdr.type) { + type = imsg_get_type(&imsg); + + switch (type) { case IMSG_CONFIGURE_ADDRESS: - if (IMSG_DATA_SIZE(imsg) != sizeof(address)) - fatalx("%s: IMSG_CONFIGURE_ADDRESS wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - memcpy(&address, imsg.data, sizeof(address)); + if (imsg_get_data(&imsg, &address, + sizeof(address)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + configure_interface(&address); break; case IMSG_WITHDRAW_ADDRESS: - if (IMSG_DATA_SIZE(imsg) != sizeof(address)) - fatalx("%s: IMSG_WITHDRAW_ADDRESS wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - memcpy(&address, imsg.data, sizeof(address)); + if (imsg_get_data(&imsg, &address, + sizeof(address)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + delete_address(&address); break; case IMSG_CONFIGURE_DFR: - if (IMSG_DATA_SIZE(imsg) != sizeof(dfr)) - fatalx("%s: IMSG_CONFIGURE_DFR wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - memcpy(&dfr, imsg.data, sizeof(dfr)); + if (imsg_get_data(&imsg, &dfr, sizeof(dfr)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + add_gateway(&dfr); break; case IMSG_WITHDRAW_DFR: - if (IMSG_DATA_SIZE(imsg) != sizeof(dfr)) - fatalx("%s: IMSG_WITHDRAW_DFR wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - memcpy(&dfr, imsg.data, sizeof(dfr)); + if (imsg_get_data(&imsg, &dfr, sizeof(dfr)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); + delete_gateway(&dfr); break; case IMSG_PROPOSE_RDNS: - if (IMSG_DATA_SIZE(imsg) != sizeof(rdns)) - fatalx("%s: IMSG_PROPOSE_RDNS wrong " - "length: %lu", __func__, - IMSG_DATA_SIZE(imsg)); - memcpy(&rdns, imsg.data, sizeof(rdns)); + if (imsg_get_data(&imsg, &rdns, sizeof(rdns)) == -1) + fatalx("%s: invalid %s", __func__, i2s(type)); if ((2 + rdns.rdns_count * sizeof(struct in6_addr)) > sizeof(struct sockaddr_rtdns)) fatalx("%s: rdns_count too big: %d", __func__, rdns.rdns_count); + send_rdns_proposal(&rdns); break; default: - log_debug("%s: error handling imsg %d", __func__, - imsg.hdr.type); + log_debug("%s: error handling imsg %d", __func__, type); break; } imsg_free(&imsg); -- In my defence, I have been left unsupervised.