From: Jan Klemkow Subject: Re: dhcp daemon checksum fix To: Alexander Bluhm Cc: tech@openbsd.org, Markus Friedl Date: Fri, 7 Feb 2025 22:30:04 +0100 On Tue, Feb 04, 2025 at 10:32:12PM GMT, Alexander Bluhm wrote: > Hi, > > When sending traffic via vio(4) between guests living within a > single KVM host, the packet checksum is never calculated. A flag > is set to assume the checksum is correct. Our IP stack handles > that correctly but when using bpf(4) userland has to care. > > jan@ has fixed it for dhcpleased a while ago. > > ---------------------------- > revision 1.42 > date: 2024/01/26 21:14:08; author: jan; state: Exp; lines: +17 -12; commitid: nvgEHwLOyfqqyGwh; > Put checksum flags in bpf_hdr to use them in userland dhcpleased. > > Thus, dhcpleased accept non-calculated checksums which were verified by > hardware/hypervisor. > > With tweaks from dlg@ > > ok bluhm@ > mkay tobhe@ > ---------------------------- > > But our other dhcp daemons suffer from the same problem. Below is > the diff for dhcpd(8), dhcrelay(8), and dhcrelay6(8). > > While there, I discovered that checksum calculation is suboptimal. > To verify, calculate the checksum of the received packet and it has > to be 0. That is what RFC 1071 says and our kernel does. > > I did run dhcpleased on a KVM guest and markus@ tested dhcpd. > > If you run any of these daemons on KVM guest, please test. > > ok? > > bluhm I reviewed and tested the dhcpd part. Looks correct and works in offloaded and non-offloaded cases. ok jan@ > Index: usr.sbin/dhcpd/bpf.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/dhcpd/bpf.c,v > diff -u -p -r1.20 bpf.c > --- usr.sbin/dhcpd/bpf.c 18 Mar 2019 00:00:59 -0000 1.20 > +++ usr.sbin/dhcpd/bpf.c 16 Jan 2025 20:10:42 -0000 > @@ -347,7 +347,8 @@ receive_packet(struct interface_info *in > > /* Decode the IP and UDP headers... */ > offset = decode_udp_ip_header(interface->rbuf + > - interface->rbuf_offset, hdr.bh_caplen, from); > + interface->rbuf_offset, hdr.bh_caplen, from, > + hdr.bh_csumflags); > > /* If the IP or UDP checksum was bad, skip the packet... */ > if (offset < 0) { > Index: usr.sbin/dhcpd/dhcpd.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/dhcpd/dhcpd.h,v > diff -u -p -r1.69 dhcpd.h > --- usr.sbin/dhcpd/dhcpd.h 21 May 2024 05:00:48 -0000 1.69 > +++ usr.sbin/dhcpd/dhcpd.h 16 Jan 2025 20:11:40 -0000 > @@ -494,7 +494,8 @@ void assemble_hw_header(struct interface > void assemble_udp_ip_header(struct interface_info *, unsigned char *, > int *, u_int32_t, u_int32_t, unsigned int, unsigned char *, int); > ssize_t decode_hw_header(unsigned char *, u_int32_t, struct hardware *); > -ssize_t decode_udp_ip_header(unsigned char *, u_int32_t, struct sockaddr_in *); > +ssize_t decode_udp_ip_header(unsigned char *, u_int32_t, struct sockaddr_in *, > + u_int16_t); > u_int32_t checksum(unsigned char *, u_int32_t, u_int32_t); > u_int32_t wrapsum(u_int32_t); > > Index: usr.sbin/dhcpd/packet.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/dhcpd/packet.c,v > diff -u -p -r1.14 packet.c > --- usr.sbin/dhcpd/packet.c 18 Apr 2017 13:59:09 -0000 1.14 > +++ usr.sbin/dhcpd/packet.c 16 Jan 2025 20:14:23 -0000 > @@ -42,6 +42,7 @@ > > #include > #include > +#include > > #include > > @@ -168,13 +169,12 @@ decode_hw_header(unsigned char *buf, u_i > > ssize_t > decode_udp_ip_header(unsigned char *buf, u_int32_t buflen, > - struct sockaddr_in *from) > + struct sockaddr_in *from, u_int16_t csumflags) > { > struct ip *ip; > struct udphdr *udp; > unsigned char *data; > u_int32_t ip_len; > - u_int32_t sum, usum; > static unsigned int ip_packets_seen; > static unsigned int ip_packets_bad_checksum; > static unsigned int udp_packets_seen; > @@ -194,7 +194,8 @@ decode_udp_ip_header(unsigned char *buf, > ip_packets_seen++; > > /* Check the IP header checksum - it should be zero. */ > - if (wrapsum(checksum((unsigned char *)ip, ip_len, 0)) != 0) { > + if ((csumflags & M_IPV4_CSUM_IN_OK) == 0 && > + wrapsum(checksum((unsigned char *)ip, ip_len, 0)) != 0) { > ip_packets_bad_checksum++; > if (ip_packets_seen > 4 && ip_packets_bad_checksum != 0 && > (ip_packets_seen / ip_packets_bad_checksum) < 2) { > @@ -253,24 +254,26 @@ decode_udp_ip_header(unsigned char *buf, > if (len + data != buf + buflen) > log_debug("accepting packet with data after udp payload."); > > - usum = udp->uh_sum; > - udp->uh_sum = 0; > - > - sum = wrapsum(checksum((unsigned char *)udp, sizeof(*udp), > - checksum(data, len, checksum((unsigned char *)&ip->ip_src, > - 2 * sizeof(ip->ip_src), > - IPPROTO_UDP + (u_int32_t)ntohs(udp->uh_ulen))))); > - > udp_packets_seen++; > - if (usum && usum != sum) { > - udp_packets_bad_checksum++; > - if (udp_packets_seen > 4 && udp_packets_bad_checksum != 0 && > - (udp_packets_seen / udp_packets_bad_checksum) < 2) { > - log_info("%u bad udp checksums in %u packets", > - udp_packets_bad_checksum, udp_packets_seen); > - udp_packets_seen = udp_packets_bad_checksum = 0; > + > + if ((csumflags & M_UDP_CSUM_IN_OK) == 0 && > + udp->uh_sum != 0) { > + udp->uh_sum = wrapsum(checksum((uint8_t *)udp, sizeof(*udp), > + checksum(data, len, > + checksum((uint8_t *)&ip->ip_src, 2 * sizeof(ip->ip_src), > + IPPROTO_UDP + ntohs(udp->uh_ulen))))); > + > + if (udp->uh_sum != 0) { > + udp_packets_bad_checksum++; > + if (udp_packets_seen > 4 && > + udp_packets_bad_checksum != 0 && > + (udp_packets_seen / udp_packets_bad_checksum) < 2) { > + log_info("%u bad udp checksums in %u packets", > + udp_packets_bad_checksum, udp_packets_seen); > + udp_packets_seen = udp_packets_bad_checksum = 0; > + } > + return (-1); > } > - return (-1); > } > > memcpy(&from->sin_port, &udp->uh_sport, sizeof(udp->uh_sport));