netdev - Re: [RFC][BUG-FIX] the problem of checksum checking in UDP protocol

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1277757512.4235.750.camel@edumazet-laptop>
Date:	Mon, 28 Jun 2010 22:38:32 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Shan Wei <shanwei@...fujitsu.com>
Cc:	David Miller <davem@...emloft.net>,
	"Ronciak, John" <john.ronciak@...el.com>, netdev@...r.kernel.org
Subject: Re: [RFC][BUG-FIX] the problem of checksum checking in UDP protocol

Le lundi 28 juin 2010 à 18:49 +0800, Shan Wei a écrit :
> Eric Dumazet wrote, at 06/26/2010 01:28 PM:
> >> (This patch is not complete, it's just for my idea.)
> >> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> >> index 1dd1aff..47f7e86 100644
> >> --- a/net/ipv6/udp.c
> >> +++ b/net/ipv6/udp.c
> >> @@ -723,6 +723,10 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> >>                 if (ulen < skb->len) {
> >>                         if (pskb_trim_rcsum(skb, ulen))
> >>                                 goto short_packet;
> >> +
> >> +                       if (skb_csum_unnecessary(skb))
> >> +                               skb->ip_summed = CHECKSUM_NONE;
> >> +
> >>                         saddr = &ipv6_hdr(skb)->saddr;
> >>                         daddr = &ipv6_hdr(skb)->daddr;
> >>                         uh = udp_hdr(skb);
> >>
> > 
> > I really dont know if this fix is the right one.
> > 
> > pskb_trim_rcsum() already contains a check. Should this check be changed
> > to include yours ?
> 
> Oh..... I don't think so.
> pskb_trim_rcsum() is also used when IPv4/IPv6 protocol receiving packets
> and reassembling fragments. IP protocol does the right check and should
> trust CHECKSUM_UNNECESSARY flag that drivers set, So we no need to 
> change IP protocol.
> If we add the skb_csum_unnecessary(skb) check into pskb_trim_rcsum() and
> reset ip_summed with CHECKSUM_NONE, the checksum check that NIC hardward 
> has done is wasted.
> 
> Only for UDP protocol over IPv4/IPv6, and length parameter is lower than
> skb->len, We reset ip_summed with CHECKSUM_NONE.
> 
> 

I read RFC 768 again, and cannot tell if your patch is ever needed.

NIC validated the UDP checksum including the whole IP data length, not
the udp length.

Linux kernel computes checksum using udp.length, not ip.length, because
only udp.length is delivered to application anyway. Extra padding is
meaningless.

RFC 768 author didnt asserted ip.length = udp.length + 8

Both implementations are RFC compliant, but may have different results
with special hand crafted packets.

You add a test for a non issue, my analysis is we should not care at
all, unless you have another valid point (security ???)

If a change is needed, I would vote for a change in NIC firmware,
because RFC 768 gives following pseudo header :

0      7 8     15 16    23 24    31 
+--------+--------+--------+--------+
|          source address           |
+--------+--------+--------+--------+
|        destination address        |
+--------+--------+--------+--------+
|  zero  |protocol|   UDP length    |
+--------+--------+--------+--------+

Note it mentions UDP length, not IP length.

If NIC reports UDP check sum was verified, it should have used UDP length as well.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html