From d79be7dd5e10fae87cca200b7950f8c9c3b52096 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Tue, 26 Jun 2018 11:18:43 +0200 Subject: [PATCH 1/2] Fix socket cmsg buffer usage --- CHANGES | 3 ++ lib/isc/unix/socket.c | 69 +++++++++++++------------------------------ 2 files changed, 23 insertions(+), 49 deletions(-) diff --git a/CHANGES b/CHANGES index f8f12d5353..01c60916e1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +4980. [bug] Fix race in cmsg buffer usage in socket code. + [GL #180] + 4979. [bug] Named-checkconf failed to detect bad in-view targets. [GL #288] diff --git a/lib/isc/unix/socket.c b/lib/isc/unix/socket.c index ae1b7498d6..028c28035f 100644 --- a/lib/isc/unix/socket.c +++ b/lib/isc/unix/socket.c @@ -375,9 +375,7 @@ struct isc__socket { unsigned char overflow; /* used for MSG_TRUNC fake */ #endif - char *recvcmsgbuf; ISC_SOCKADDR_LEN_T recvcmsgbuflen; - char *sendcmsgbuf; ISC_SOCKADDR_LEN_T sendcmsgbuflen; void *fdwatcharg; @@ -487,9 +485,9 @@ static void internal_send(isc_task_t *, isc_event_t *); static void internal_fdwatch_write(isc_task_t *, isc_event_t *); static void internal_fdwatch_read(isc_task_t *, isc_event_t *); static void process_cmsg(isc__socket_t *, struct msghdr *, isc_socketevent_t *); -static void build_msghdr_send(isc__socket_t *, isc_socketevent_t *, +static void build_msghdr_send(isc__socket_t *, char *, isc_socketevent_t *, struct msghdr *, struct iovec *, size_t *); -static void build_msghdr_recv(isc__socket_t *, isc_socketevent_t *, +static void build_msghdr_recv(isc__socket_t *, char *, isc_socketevent_t *, struct msghdr *, struct iovec *, size_t *); #ifdef USE_WATCHER_THREAD static isc_boolean_t process_ctlfd(isc__socketmgr_t *manager); @@ -1443,7 +1441,7 @@ process_cmsg(isc__socket_t *sock, struct msghdr *msg, isc_socketevent_t *dev) { * this transaction can send. */ static void -build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, +build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, struct msghdr *msg, struct iovec *iov, size_t *write_countp) { unsigned int iovcount; @@ -1456,8 +1454,9 @@ build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, #endif memset(msg, 0, sizeof(*msg)); + if (sock->sendcmsgbuflen != 0U) { - memset(sock->sendcmsgbuf, 0, sock->sendcmsgbuflen); + memset(cmsgbuf, 0, sock->sendcmsgbuflen); } if (!sock->connected) { @@ -1536,11 +1535,11 @@ build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, "sendto pktinfo data, ifindex %u", dev->pktinfo.ipi6_ifindex); - msg->msg_control = (void *)sock->sendcmsgbuf; + msg->msg_control = (void *)cmsgbuf; msg->msg_controllen = cmsg_space(sizeof(struct in6_pktinfo)); INSIST(msg->msg_controllen <= sock->sendcmsgbuflen); - cmsgp = (struct cmsghdr *)sock->sendcmsgbuf; + cmsgp = (struct cmsghdr *)cmsgbuf; cmsgp->cmsg_level = IPPROTO_IPV6; cmsgp->cmsg_type = IPV6_PKTINFO; cmsgp->cmsg_len = cmsg_len(sizeof(struct in6_pktinfo)); @@ -1555,7 +1554,7 @@ build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, { int use_min_mtu = 1; /* -1, 0, 1 */ - cmsgp = (struct cmsghdr *)(sock->sendcmsgbuf + + cmsgp = (struct cmsghdr *)(cmsgbuf + msg->msg_controllen); msg->msg_control = (void *)sock->sendcmsgbuf; msg->msg_controllen += cmsg_space(sizeof(use_min_mtu)); @@ -1585,9 +1584,9 @@ build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, #ifdef IP_TOS if (sock->pf == AF_INET && sock->pktdscp) { - cmsgp = (struct cmsghdr *)(sock->sendcmsgbuf + + cmsgp = (struct cmsghdr *)(cmsgbuf + msg->msg_controllen); - msg->msg_control = (void *)sock->sendcmsgbuf; + msg->msg_control = (void *)cmsgbuf; msg->msg_controllen += cmsg_space(sizeof(dscp)); INSIST(msg->msg_controllen <= sock->sendcmsgbuflen); @@ -1616,9 +1615,9 @@ build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, #endif #if defined(IPPROTO_IPV6) && defined(IPV6_TCLASS) if (sock->pf == AF_INET6 && sock->pktdscp) { - cmsgp = (struct cmsghdr *)(sock->sendcmsgbuf + + cmsgp = (struct cmsghdr *)(cmsgbuf + msg->msg_controllen); - msg->msg_control = (void *)sock->sendcmsgbuf; + msg->msg_control = (void *)cmsgbuf; msg->msg_controllen += cmsg_space(sizeof(dscp)); INSIST(msg->msg_controllen <= sock->sendcmsgbuflen); @@ -1647,7 +1646,7 @@ build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, if (msg->msg_controllen != 0 && msg->msg_controllen < sock->sendcmsgbuflen) { - memset(sock->sendcmsgbuf + msg->msg_controllen, 0, + memset(cmsgbuf + msg->msg_controllen, 0, sock->sendcmsgbuflen - msg->msg_controllen); } } @@ -1675,7 +1674,7 @@ build_msghdr_send(isc__socket_t *sock, isc_socketevent_t *dev, * this transaction can receive. */ static void -build_msghdr_recv(isc__socket_t *sock, isc_socketevent_t *dev, +build_msghdr_recv(isc__socket_t *sock, char *cmsgbuf, isc_socketevent_t *dev, struct msghdr *msg, struct iovec *iov, size_t *read_countp) { unsigned int iovcount; @@ -1773,7 +1772,7 @@ build_msghdr_recv(isc__socket_t *sock, isc_socketevent_t *dev, #ifdef ISC_NET_BSD44MSGHDR #if defined(USE_CMSG) - msg->msg_control = sock->recvcmsgbuf; + msg->msg_control = cmsgbuf; msg->msg_controllen = sock->recvcmsgbuflen; #else msg->msg_control = NULL; @@ -1877,8 +1876,9 @@ doio_recv(isc__socket_t *sock, isc_socketevent_t *dev) { isc_buffer_t *buffer; int recv_errno; char strbuf[ISC_STRERRORSIZE]; + char cmsgbuf[sock->recvcmsgbuflen]; - build_msghdr_recv(sock, dev, &msghdr, iov, &read_count); + build_msghdr_recv(sock, cmsgbuf, dev, &msghdr, iov, &read_count); #if defined(ISC_SOCKET_DEBUG) dump_msg(&msghdr); @@ -2072,8 +2072,9 @@ doio_send(isc__socket_t *sock, isc_socketevent_t *dev) { int attempts = 0; int send_errno; char strbuf[ISC_STRERRORSIZE]; + char cmsgbuf[sock->sendcmsgbuflen]; - build_msghdr_send(sock, dev, &msghdr, iov, &write_count); + build_msghdr_send(sock, cmsgbuf, dev, &msghdr, iov, &write_count); resend: if (sock->type == isc_sockettype_udp && @@ -2312,11 +2313,8 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, ISC_LINK_INIT(sock, link); - sock->recvcmsgbuf = NULL; - sock->sendcmsgbuf = NULL; - /* - * Set up cmsg buffers. + * Set up cmsg buffer lengths. */ cmsgbuflen = 0; #if defined(USE_CMSG) && defined(ISC_PLATFORM_HAVEIN6PKTINFO) @@ -2329,13 +2327,6 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, cmsgbuflen += cmsg_space(sizeof(int)); #endif sock->recvcmsgbuflen = cmsgbuflen; - if (sock->recvcmsgbuflen != 0U) { - sock->recvcmsgbuf = isc_mem_get(manager->mctx, cmsgbuflen); - if (sock->recvcmsgbuf == NULL) { - result = ISC_R_NOMEMORY; - goto error; - } - } cmsgbuflen = 0; #if defined(USE_CMSG) && defined(ISC_PLATFORM_HAVEIN6PKTINFO) @@ -2352,13 +2343,6 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, cmsgbuflen += cmsg_space(sizeof(int)); #endif sock->sendcmsgbuflen = cmsgbuflen; - if (sock->sendcmsgbuflen != 0U) { - sock->sendcmsgbuf = isc_mem_get(manager->mctx, cmsgbuflen); - if (sock->sendcmsgbuf == NULL) { - result = ISC_R_NOMEMORY; - goto error; - } - } memset(sock->name, 0, sizeof(sock->name)); sock->tag = NULL; @@ -2406,12 +2390,6 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, return (ISC_R_SUCCESS); error: - if (sock->recvcmsgbuf != NULL) - isc_mem_put(manager->mctx, sock->recvcmsgbuf, - sock->recvcmsgbuflen); - if (sock->sendcmsgbuf != NULL) - isc_mem_put(manager->mctx, sock->sendcmsgbuf, - sock->sendcmsgbuflen); isc_mem_put(manager->mctx, sock, sizeof(*sock)); return (result); @@ -2440,13 +2418,6 @@ free_socket(isc__socket_t **socketp) { INSIST(ISC_LIST_EMPTY(sock->connect_list)); INSIST(!ISC_LINK_LINKED(sock, link)); - if (sock->recvcmsgbuf != NULL) - isc_mem_put(sock->manager->mctx, sock->recvcmsgbuf, - sock->recvcmsgbuflen); - if (sock->sendcmsgbuf != NULL) - isc_mem_put(sock->manager->mctx, sock->sendcmsgbuf, - sock->sendcmsgbuflen); - sock->common.magic = 0; sock->common.impmagic = 0; From 49f90025a07be39d1236504d46db3ce73f1578ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Tue, 26 Jun 2018 15:11:28 +0200 Subject: [PATCH 2/2] Use completely static-sized buffers --- lib/isc/unix/socket.c | 96 ++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 57 deletions(-) diff --git a/lib/isc/unix/socket.c b/lib/isc/unix/socket.c index 028c28035f..b14f23c05a 100644 --- a/lib/isc/unix/socket.c +++ b/lib/isc/unix/socket.c @@ -319,6 +319,35 @@ typedef isc_event_t intev_t; #define RCVBUFSIZE (32*1024) #endif /* TUNE_LARGE */ +/*% + * Instead of calculating the cmsgbuf lengths every time we take + * a rule of thumb approach - sizes are taken from x86_64 linux, + * multiplied by 2, everything should fit. Those sizes are not + * large enough to cause any concern. + */ +#if defined(USE_CMSG) && defined(ISC_PLATFORM_HAVEIN6PKTINFO) +#define CMSG_SP_IN6PKT 40 +#else +#define CMSG_SP_IN6PKT 0 +#endif + +#if defined(USE_CMSG) && defined(SO_TIMESTAMP) +#define CMSG_SP_TIMESTAMP 32 +#else +#define CMSG_SP_TIMESTAMP 0 +#endif + +#if defined(USE_CMSG) && (defined(IPV6_TCLASS) || defined(IP_TOS)) +#define CMSG_SP_TCTOS 24 +#else +#define CMSG_SP_TCTOS 0 +#endif + +#define CMSG_SP_INT 24 + +#define RECVCMSGBUFLEN (2*(CMSG_SP_IN6PKT + CMSG_SP_TIMESTAMP + CMSG_SP_TCTOS)+1) +#define SENDCMSGBUFLEN (2*(CMSG_SP_IN6PKT + CMSG_SP_INT + CMSG_SP_TCTOS)+1) + /*% * The number of times a send operation is repeated if the result is EINTR. */ @@ -375,9 +404,6 @@ struct isc__socket { unsigned char overflow; /* used for MSG_TRUNC fake */ #endif - ISC_SOCKADDR_LEN_T recvcmsgbuflen; - ISC_SOCKADDR_LEN_T sendcmsgbuflen; - void *fdwatcharg; isc_sockfdwatch_t fdwatchcb; int fdwatchflags; @@ -1455,10 +1481,6 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, memset(msg, 0, sizeof(*msg)); - if (sock->sendcmsgbuflen != 0U) { - memset(cmsgbuf, 0, sock->sendcmsgbuflen); - } - if (!sock->connected) { msg->msg_name = (void *)&dev->address.type.sa; msg->msg_namelen = dev->address.length; @@ -1537,7 +1559,7 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, msg->msg_control = (void *)cmsgbuf; msg->msg_controllen = cmsg_space(sizeof(struct in6_pktinfo)); - INSIST(msg->msg_controllen <= sock->sendcmsgbuflen); + INSIST(msg->msg_controllen <= SENDCMSGBUFLEN); cmsgp = (struct cmsghdr *)cmsgbuf; cmsgp->cmsg_level = IPPROTO_IPV6; @@ -1556,9 +1578,9 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, cmsgp = (struct cmsghdr *)(cmsgbuf + msg->msg_controllen); - msg->msg_control = (void *)sock->sendcmsgbuf; + msg->msg_control = (void *)cmsgbuf; msg->msg_controllen += cmsg_space(sizeof(use_min_mtu)); - INSIST(msg->msg_controllen <= sock->sendcmsgbuflen); + INSIST(msg->msg_controllen <= SENDCMSGBUFLEN); cmsgp->cmsg_level = IPPROTO_IPV6; cmsgp->cmsg_type = IPV6_USE_MIN_MTU; @@ -1588,7 +1610,7 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, msg->msg_controllen); msg->msg_control = (void *)cmsgbuf; msg->msg_controllen += cmsg_space(sizeof(dscp)); - INSIST(msg->msg_controllen <= sock->sendcmsgbuflen); + INSIST(msg->msg_controllen <= SENDCMSGBUFLEN); cmsgp->cmsg_level = IPPROTO_IP; cmsgp->cmsg_type = IP_TOS; @@ -1619,7 +1641,7 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, msg->msg_controllen); msg->msg_control = (void *)cmsgbuf; msg->msg_controllen += cmsg_space(sizeof(dscp)); - INSIST(msg->msg_controllen <= sock->sendcmsgbuflen); + INSIST(msg->msg_controllen <= SENDCMSGBUFLEN); cmsgp->cmsg_level = IPPROTO_IPV6; cmsgp->cmsg_type = IPV6_TCLASS; @@ -1644,10 +1666,10 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, } #endif if (msg->msg_controllen != 0 && - msg->msg_controllen < sock->sendcmsgbuflen) + msg->msg_controllen < SENDCMSGBUFLEN) { memset(cmsgbuf + msg->msg_controllen, 0, - sock->sendcmsgbuflen - msg->msg_controllen); + SENDCMSGBUFLEN - msg->msg_controllen); } } #endif @@ -1773,7 +1795,7 @@ build_msghdr_recv(isc__socket_t *sock, char *cmsgbuf, isc_socketevent_t *dev, #ifdef ISC_NET_BSD44MSGHDR #if defined(USE_CMSG) msg->msg_control = cmsgbuf; - msg->msg_controllen = sock->recvcmsgbuflen; + msg->msg_controllen = RECVCMSGBUFLEN; #else msg->msg_control = NULL; msg->msg_controllen = 0; @@ -1876,7 +1898,7 @@ doio_recv(isc__socket_t *sock, isc_socketevent_t *dev) { isc_buffer_t *buffer; int recv_errno; char strbuf[ISC_STRERRORSIZE]; - char cmsgbuf[sock->recvcmsgbuflen]; + char cmsgbuf[RECVCMSGBUFLEN] = {0}; build_msghdr_recv(sock, cmsgbuf, dev, &msghdr, iov, &read_count); @@ -2072,7 +2094,7 @@ doio_send(isc__socket_t *sock, isc_socketevent_t *dev) { int attempts = 0; int send_errno; char strbuf[ISC_STRERRORSIZE]; - char cmsgbuf[sock->sendcmsgbuflen]; + char cmsgbuf[SENDCMSGBUFLEN] = {0}; build_msghdr_send(sock, cmsgbuf, dev, &msghdr, iov, &write_count); @@ -2292,7 +2314,6 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, { isc__socket_t *sock; isc_result_t result; - ISC_SOCKADDR_LEN_T cmsgbuflen; sock = isc_mem_get(manager->mctx, sizeof(*sock)); @@ -2313,36 +2334,6 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, ISC_LINK_INIT(sock, link); - /* - * Set up cmsg buffer lengths. - */ - cmsgbuflen = 0; -#if defined(USE_CMSG) && defined(ISC_PLATFORM_HAVEIN6PKTINFO) - cmsgbuflen += cmsg_space(sizeof(struct in6_pktinfo)); -#endif -#if defined(USE_CMSG) && defined(SO_TIMESTAMP) - cmsgbuflen += cmsg_space(sizeof(struct timeval)); -#endif -#if defined(USE_CMSG) && (defined(IPV6_TCLASS) || defined(IP_TOS)) - cmsgbuflen += cmsg_space(sizeof(int)); -#endif - sock->recvcmsgbuflen = cmsgbuflen; - - cmsgbuflen = 0; -#if defined(USE_CMSG) && defined(ISC_PLATFORM_HAVEIN6PKTINFO) - cmsgbuflen += cmsg_space(sizeof(struct in6_pktinfo)); -#if defined(IPV6_USE_MIN_MTU) - /* - * Provide space for working around FreeBSD's broken IPV6_USE_MIN_MTU - * support. - */ - cmsgbuflen += cmsg_space(sizeof(int)); -#endif -#endif -#if defined(USE_CMSG) && (defined(IP_TOS) || defined(IPV6_TCLASS)) - cmsgbuflen += cmsg_space(sizeof(int)); -#endif - sock->sendcmsgbuflen = cmsgbuflen; memset(sock->name, 0, sizeof(sock->name)); sock->tag = NULL; @@ -2788,15 +2779,6 @@ opensocket(isc__socketmgr_t *manager, isc__socket_t *sock, #endif /* SO_TIMESTAMP */ #if defined(ISC_PLATFORM_HAVEIPV6) - if (sock->pf == AF_INET6 && sock->recvcmsgbuflen == 0U) { - /* - * Warn explicitly because this anomaly can be hidden - * in usual operation (and unexpectedly appear later). - */ - UNEXPECTED_ERROR(__FILE__, __LINE__, - "No buffer available to receive " - "IPv6 destination"); - } #ifdef ISC_PLATFORM_HAVEIN6PKTINFO #ifdef IPV6_RECVPKTINFO /* RFC 3542 */