Index | Thread | Search

From:
Jan Klemkow <jan@openbsd.org>
Subject:
Re: dhcp daemon checksum fix
To:
Alexander Bluhm <bluhm@openbsd.org>
Cc:
tech@openbsd.org, Markus Friedl <markus@openbsd.org>
Date:
Fri, 7 Feb 2025 22:53:59 +0100

Download raw body.

Thread
On Tue, Feb 04, 2025 at 10:32:12PM GMT, Alexander Bluhm wrote:
> 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?

I tested and reviewed the dhcrelay part. Looks correct and works
in my test setup.

ok jan@

> Index: usr.sbin/dhcrelay/bpf.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/dhcrelay/bpf.c,v
> diff -u -p -r1.19 bpf.c
> --- usr.sbin/dhcrelay/bpf.c	19 Apr 2017 05:36:12 -0000	1.19
> +++ usr.sbin/dhcrelay/bpf.c	16 Jan 2025 22:03:10 -0000
> @@ -453,7 +453,7 @@ receive_packet(struct interface_info *in
>  
>  		/* Decode the IP and UDP headers... */
>  		offset = decode_udp_ip_header(interface->rbuf,
> -		    interface->rbuf_len, offset, pc);
> +		    interface->rbuf_len, offset, pc, hdr.bh_csumflags);
>  
>  		/* If the IP or UDP checksum was bad, skip the packet... */
>  		if (offset < 0) {
> Index: usr.sbin/dhcrelay/dhcpd.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/dhcrelay/dhcpd.h,v
> diff -u -p -r1.24 dhcpd.h
> --- usr.sbin/dhcrelay/dhcpd.h	21 May 2024 05:00:48 -0000	1.24
> +++ usr.sbin/dhcrelay/dhcpd.h	16 Jan 2025 22:01:55 -0000
> @@ -169,7 +169,7 @@ ssize_t assemble_udp_ip_header(unsigned 
>  ssize_t decode_hw_header(unsigned char *, size_t, size_t, struct packet_ctx *,
>      unsigned int);
>  ssize_t decode_udp_ip_header(unsigned char *, size_t, size_t,
> -    struct packet_ctx *);
> +    struct packet_ctx *, u_int16_t);
>  
>  /* dhcrelay.c */
>  extern int server_fd;
> Index: usr.sbin/dhcrelay/packet.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/dhcrelay/packet.c,v
> diff -u -p -r1.14 packet.c
> --- usr.sbin/dhcrelay/packet.c	5 Apr 2017 14:40:56 -0000	1.14
> +++ usr.sbin/dhcrelay/packet.c	16 Jan 2025 22:03:52 -0000
> @@ -42,6 +42,7 @@
>  
>  #include <sys/types.h>
>  #include <sys/socket.h>
> +#include <sys/mbuf.h>
>  
>  #include <arpa/inet.h>
>  
> @@ -214,13 +215,12 @@ decode_hw_header(unsigned char *buf, siz
>  
>  ssize_t
>  decode_udp_ip_header(unsigned char *buf, size_t buflen,
> -    size_t offset, struct packet_ctx *pc)
> +    size_t offset, struct packet_ctx *pc, 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;
> @@ -240,7 +240,8 @@ decode_udp_ip_header(unsigned char *buf,
>  	ip_packets_seen++;
>  
>  	/* Check the IP header checksum - it should be zero. */
> -	if (wrapsum(checksum(buf + offset, ip_len, 0)) != 0) {
> +	if ((csumflags & M_IPV4_CSUM_IN_OK) == 0 &&
> +	    wrapsum(checksum(buf + offset, 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) {
> @@ -306,24 +307,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);
>  	}
>  
>  	ss2sin(&pc->pc_src)->sin_port = udp->uh_sport;