Re: udp checksum problems with dhcpcd v8.1.0
Roy Marples
Tue Oct 15 14:19:50 2019
Hi Giorgio
On 15/10/2019 09:02, Giorgio Dal Molin wrote:
after updating dhcpcd from v8.0.6 to v8.1.0 I'm getting the following udp
checksum errors:
~ # dhcpcd -d eth1
dhcpcd-8.1.0 starting
eth1: executing `//lib/dhcpcd/dhcpcd-run-hooks' PREINIT
eth1: executing `//lib/dhcpcd/dhcpcd-run-hooks' CARRIER
DUID 00:04:03:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx:xx
eth1: IAID d6:14:c2:b9
eth1: adding address fe80::e30:7021:c623:ad0f
eth1: pltime infinity, vltime infinity
eth1: delaying IPv6 router solicitation for 0.1 seconds
eth1: delaying IPv4 for 0.2 seconds
eth1: soliciting an IPv6 router
eth1: delaying Router Solicitation for LL address
eth1: soliciting a DHCP lease
eth1: sending DISCOVER (xid 0x65e41544), next in 4.7 seconds
eth1: checksum failure from 172.18.4.226
eth1: sending Router Solicitation
eth1: sending DISCOVER (xid 0x65e41544), next in 8.1 seconds
eth1: checksum failure from 172.18.4.226
eth1: checksum failure from 172.18.4.226
eth1: probing for an IPv4LL address
eth1: probing for 169.254.78.153
eth1: ARP probing 169.254.78.153 (1 of 3), next in 1.4 seconds
eth1: sending Router Solicitation
eth1: checksum failure from 172.18.4.225
eth1: ARP probing 169.254.78.153 (2 of 3), next in 1.6 seconds
eth1: ARP probing 169.254.78.153 (3 of 3), next in 2.0 seconds
eth1: sending Router Solicitation
eth1: using IPv4LL address 169.254.78.153
eth1: adding IP address 169.254.78.153/16 broadcast 169.254.255.255
eth1: adding route to 169.254.0.0/16
eth1: adding default route
eth1: ARP announcing 169.254.78.153 (1 of 2), next in 2.0 seconds
eth1: executing `//lib/dhcpcd/dhcpcd-run-hooks' IPV4LL
forking to background
forked to background, child pid 22198
I've bisected this on the git repo to the following commit:
f198ce2b2b1df1febe0cb64ecd023a5ecb1c0bbf is the first bad commit
commit f198ce2b2b1df1febe0cb64ecd023a5ecb1c0bbf
Author: Roy Marples <roy@xxxxxxxxxxxx>
Date: Fri Oct 11 11:24:38 2019 +0100
BPF: Move validation logic from BPF to consumers
Even though we program the BPF filter should we trust it?
On Linux at least there is a window between opening the socket,
binding the interface and setting the filter where we receive data.
This data is NOT checked OR flushed and IS returned when reading.
We have no way of flushing it other than reading these packets!
But we don't know if they passed the filter or not ..... so we need
to validate each and every packet that comes through ourselves as well.
Even if Linux does fix this sorry state, who is to say other kernels
don't have bugs causing a similar effect?
As such, let's strive to keep the filters just for pattern matching
to avoid waking dhcpcd up.
src/arp.c | 53 ++++++++++++++++++----------
src/bpf.c | 81 ++++---------------------------------------
src/bpf.h | 19 ++++++++++
src/dhcp.c | 108 ++++++++++++++++++++++++++++-----------------------------
src/if-linux.c | 49 +++++++++++++++-----------
5 files changed, 142 insertions(+), 168 deletions(-)
Inspecting the changes to the function 'checksums_valid()' in 'src/dhcp.c' they
look a bit messed up:
/* Lengths have already been checked. */
static bool
checksums_valid(void *packet,
struct in_addr *from, unsigned int flags)
{
struct ip *ip = packet;
struct ip pseudo_ip = {
.ip_p = IPPROTO_UDP,
.ip_src = ip->ip_src,
.ip_dst = ip->ip_dst
};
size_t ip_hlen;
uint16_t udp_len, uh_sum;
struct udphdr *udp;
uint32_t csum;
if (from != NULL)
from->s_addr = ip->ip_src.s_addr;
ip_hlen = (size_t)ip->ip_hl * 4;
if (in_cksum(ip, ip_hlen, NULL) != 0)
return false;
if (flags & BPF_PARTIALCSUM)
return 0;
udp = (struct udphdr *)(void *)((char *)ip + ip_hlen);
if (udp->uh_sum == 0)
return 0;
/* UDP checksum is based on a pseudo IP header alongside
* the UDP header and payload. */
udp_len = ntohs(udp->uh_ulen);
uh_sum = udp->uh_sum;
udp->uh_sum = 0;
pseudo_ip.ip_len = udp->uh_ulen;
csum = 0;
in_cksum(&pseudo_ip, sizeof(pseudo_ip), &csum);
csum = in_cksum(udp, udp_len, &csum);
return csum == uh_sum;
}
you changed it to return a bool but a couple of return 0; were forgotten,
a very 'unfortunate circumstance'.
Yes, I've already pushed some commits, but this patch is the
combination. Does it fix it for you?
Roy
diff --git a/src/dhcp.c b/src/dhcp.c
index ab0ae90b..c989843b 100644
--- a/src/dhcp.c
+++ b/src/dhcp.c
@@ -164,8 +164,6 @@ dhcp_printoptions(const struct dhcpcd_ctx *ctx,
}
}
-#define get_option_raw(ctx, bootp, bootp_len, opt) \
- get_option((ctx), (bootp), (bootp_len), NULL)
static const uint8_t *
get_option(struct dhcpcd_ctx *ctx,
const struct bootp *bootp, size_t bootp_len,
@@ -3275,26 +3273,35 @@ is_packet_udp_bootp(void *packet, size_t plen)
{
struct ip *ip = packet;
size_t ip_hlen;
- struct udphdr *udp;
+ struct udphdr udp;
- if (sizeof(*ip) > plen)
+ if (plen < sizeof(*ip))
return false;
if (ip->ip_v != IPVERSION || ip->ip_p != IPPROTO_UDP)
return false;
/* Sanity. */
- if (ntohs(ip->ip_len) != plen)
+ if (ntohs(ip->ip_len) > plen)
return false;
ip_hlen = (size_t)ip->ip_hl * 4;
+ if (ip_hlen < sizeof(*ip))
+ return false;
+
/* Check we have a UDP header and BOOTP. */
- if (ip_hlen + sizeof(*udp) + offsetof(struct bootp, vend) > plen)
+ if (ip_hlen + sizeof(udp) + offsetof(struct bootp, vend) > plen)
+ return false;
+
+ /* Sanity. */
+ memcpy(&udp, (char *)ip + ip_hlen, sizeof(udp));
+ if (ntohs(udp.uh_ulen) < sizeof(udp))
+ return false;
+ if (ip_hlen + ntohs(udp.uh_ulen) > plen)
return false;
/* Check it's to and from the right ports. */
- udp = (struct udphdr *)(void *)((char *)ip + ip_hlen);
- if (udp->uh_dport != htons(BOOTPC) || udp->uh_sport != htons(BOOTPS))
+ if (udp.uh_dport != htons(BOOTPC) || udp.uh_sport != htons(BOOTPS))
return false;
return true;
@@ -3313,7 +3320,8 @@ checksums_valid(void *packet,
};
size_t ip_hlen;
uint16_t udp_len, uh_sum;
- struct udphdr *udp;
+ struct udphdr udp;
+ char *udpp, *uh_sump;
uint32_t csum;
if (from != NULL)
@@ -3324,21 +3332,30 @@ checksums_valid(void *packet,
return false;
if (flags & BPF_PARTIALCSUM)
- return 0;
+ return true;
- udp = (struct udphdr *)(void *)((char *)ip + ip_hlen);
- if (udp->uh_sum == 0)
- return 0;
+ udpp = (char *)ip + ip_hlen;
+ memcpy(&udp, udpp, sizeof(udp));
+ if (udp.uh_sum == 0)
+ return true;
/* UDP checksum is based on a pseudo IP header alongside
* the UDP header and payload. */
- udp_len = ntohs(udp->uh_ulen);
- uh_sum = udp->uh_sum;
- udp->uh_sum = 0;
- pseudo_ip.ip_len = udp->uh_ulen;
+ udp_len = ntohs(udp.uh_ulen);
+ uh_sum = udp.uh_sum;
+
+ /* Need to zero the UDP sum in the packet for the checksum to work. */
+ uh_sump = udpp + offsetof(struct udphdr, uh_sum);
+ memset(uh_sump, 0, sizeof(udp.uh_sum));
+
+ pseudo_ip.ip_len = udp.uh_ulen;
csum = 0;
in_cksum(&pseudo_ip, sizeof(pseudo_ip), &csum);
- csum = in_cksum(udp, udp_len, &csum);
+ csum = in_cksum(udpp, udp_len, &csum);
+
+ /* Put the checksum back. */
+ memcpy(uh_sump, &uh_sum, sizeof(udp.uh_sum));
+
return csum == uh_sum;
}
Archive administrator: postmaster@marples.name