Discussion:
Wrong size of L4 header + payload in in{,6}_undefer_cksum()
(too old to reply)
Rin Okuyama
2018-09-18 02:10:19 UTC
Permalink
I found bugs in in{,6}_undefer_cksum() while working on kern/53562:

Index: sys/netinet/in_offload.c
===================================================================
RCS file: /home/netbsd/src/sys/netinet/in_offload.c,v
retrieving revision 1.11
diff -p -u -r1.11 in_offload.c
--- sys/netinet/in_offload.c 11 Jul 2018 06:25:05 -0000 1.11
+++ sys/netinet/in_offload.c 16 Sep 2018 14:00:52 -0000
@@ -213,7 +226,7 @@ in_undefer_cksum(struct mbuf *m, size_t
if (csum_flags & (M_CSUM_UDPv4|M_CSUM_TCPv4)) {
size_t l4offset = hdrlen + iphdrlen;

- csum = in4_cksum(m, 0, l4offset, ip_len - l4offset - hdrlen);
+ csum = in4_cksum(m, 0, l4offset, ip_len - iphdrlen);
if (csum == 0 && (csum_flags & M_CSUM_UDPv4) != 0)
csum = 0xffff;

Index: sys/netinet6/in6_offload.c
===================================================================
RCS file: /home/netbsd/src/sys/netinet6/in6_offload.c,v
retrieving revision 1.10
diff -p -u -r1.10 in6_offload.c
--- sys/netinet6/in6_offload.c 10 Aug 2018 06:55:04 -0000 1.10
+++ sys/netinet6/in6_offload.c 18 Sep 2018 01:33:59 -0000
@@ -193,7 +205,8 @@ in6_undefer_cksum(struct mbuf *m, size_t

l4hdroff = M_CSUM_DATA_IPv6_IPHL(m->m_pkthdr.csum_data);
l4offset = hdrlen + l4hdroff;
- csum = in6_cksum(m, 0, l4offset, plen - l4hdroff);
+ csum = in6_cksum(m, 0, l4offset,
+ plen - (l4hdroff - sizeof(struct ip6_hdr)));

if (csum == 0 && (csum_flags & M_CSUM_UDPv6) != 0)
csum = 0xffff;

The 4th argument for in[46]_cksum() is size of L4 header + L4 payload.

- For IPv4, the current version is wrong when hdrlen != 0.
- For IPv6, the current version is always wrong.

These functions are used only in net/if_loop.c and
arch/powerpc/booke/dev/pq3etsec.c under some special circumferences.
I guess this is why these bugs has not been found until today.

Can I commit the fix?

Thanks,
rin

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Maxime Villard
2018-09-19 07:45:29 UTC
Permalink
Post by Rin Okuyama
Index: sys/netinet/in_offload.c
===================================================================
RCS file: /home/netbsd/src/sys/netinet/in_offload.c,v
retrieving revision 1.11
diff -p -u -r1.11 in_offload.c
--- sys/netinet/in_offload.c 11 Jul 2018 06:25:05 -0000 1.11
+++ sys/netinet/in_offload.c 16 Sep 2018 14:00:52 -0000
@@ -213,7 +226,7 @@ in_undefer_cksum(struct mbuf *m, size_t
if (csum_flags & (M_CSUM_UDPv4|M_CSUM_TCPv4)) {
size_t l4offset = hdrlen + iphdrlen;
- csum = in4_cksum(m, 0, l4offset, ip_len - l4offset - hdrlen);
+ csum = in4_cksum(m, 0, l4offset, ip_len - iphdrlen);
if (csum == 0 && (csum_flags & M_CSUM_UDPv4) != 0)
csum = 0xffff;
Index: sys/netinet6/in6_offload.c
===================================================================
RCS file: /home/netbsd/src/sys/netinet6/in6_offload.c,v
retrieving revision 1.10
diff -p -u -r1.10 in6_offload.c
--- sys/netinet6/in6_offload.c 10 Aug 2018 06:55:04 -0000 1.10
+++ sys/netinet6/in6_offload.c 18 Sep 2018 01:33:59 -0000
@@ -193,7 +205,8 @@ in6_undefer_cksum(struct mbuf *m, size_t
l4hdroff = M_CSUM_DATA_IPv6_IPHL(m->m_pkthdr.csum_data);
l4offset = hdrlen + l4hdroff;
- csum = in6_cksum(m, 0, l4offset, plen - l4hdroff);
+ csum = in6_cksum(m, 0, l4offset,
+ plen - (l4hdroff - sizeof(struct ip6_hdr)));
if (csum == 0 && (csum_flags & M_CSUM_UDPv6) != 0)
csum = 0xffff;
The 4th argument for in[46]_cksum() is size of L4 header + L4 payload.
- For IPv4, the current version is wrong when hdrlen != 0.
- For IPv6, the current version is always wrong.
These functions are used only in net/if_loop.c and
arch/powerpc/booke/dev/pq3etsec.c under some special circumferences.
I guess this is why these bugs has not been found until today.
Can I commit the fix?
Yes, please commit.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Rin Okuyama
2018-09-19 07:55:45 UTC
Permalink
...
Post by Maxime Villard
Post by Rin Okuyama
Can I commit the fix?
Yes, please commit.
Committed. Thank you for your review!

rin

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Loading...