net: udp: fix handling of CHECKSUM_COMPLETE packets
Subsystems (4)
PATCH v2 1/1 net: udp: fix handling of CHECKSUM_COMPLETE packets
Current handling of CHECKSUM_COMPLETE packets by the UDP stack is incorrect for any packet that has an incorrect checksum value.
udp4/6_csum_init() will both make a call to __skb_checksum_validate_complete() to initialize/validate the csum field when receiving a CHECKSUM_COMPLETE packet. When this packet fails validation, skb->csum will be overwritten with the pseudoheader checksum so the packet can be fully validated by software, but the skb->ip_summed value will be left as CHECKSUM_COMPLETE so that way the stack can later warn the user about their hardware spewing bad checksums. Unfortunately, leaving the SKB in this state can cause problems later on in the checksum calculation.
Since the the packet is still marked as CHECKSUM_COMPLETE, udp_csum_pull_header() will SUBTRACT the checksum of the UDP header from skb->csum instead of adding it, leaving us with a garbage value in that field. Once we try to copy the packet to userspace in the udp4/6_recvmsg(), we'll make a call to skb_copy_and_csum_datagram_msg() to checksum the packet data and add it in the garbage skb->csum value to perform our final validation check.
Since the value we're validating is not the proper checksum, it's possible that the folded value could come out to 0, causing us not to drop the packet. Instead, we believe that the packet was checksummed incorrectly by hardware since skb->ip_summed is still CHECKSUM_COMPLETE, and we attempt to warn the user with netdev_rx_csum_fault(skb->dev);
Unfortunately, since this is the UDP path, skb->dev has been overwritten by skb->dev_scratch and is no longer a valid pointer, so we end up reading invalid memory.
This patch addresses this problem in two ways:
1) Do not use the dev pointer when calling netdev_rx_csum_fault() from skb_copy_and_csum_datagram_msg(). Since this gets called from the UDP path where skb->dev has been overwritten, we have no way of knowing if the pointer is still valid. Also for the sake of consistency with the other uses of netdev_rx_csum_fault(), don't attempt to call it if the packet was checksummed by software.
2) Add better CHECKSUM_COMPLETE handling to udp4/6_csum_init(). If we receive a packet that's CHECKSUM_COMPLETE that fails verification (i.e. skb->csum_valid == 0), check who performed the calculation. It's possible that the checksum was done in software by the network stack earlier (such as Netfilter's CONNTRACK module), and if that says the checksum is bad, we can drop the packet immediately instead of waiting until we try and copy it to userspace. Otherwise, we need to mark the SKB as CHECKSUM_NONE, since the skb->csum field no longer contains the full packet checksum after the call to __skb_checksum_validate_complete().
--- net/core/datagram.c | 5 +++-- net/ipv4/udp.c | 20 ++++++++++++++++++-- net/ipv6/ip6_checksum.c | 20 ++++++++++++++++++-- 3 files changed, 39 insertions(+), 6 deletions(-)
net/core/datagram.c +3 -2
@@ -808,8 +808,9 @@ int skb_copy_and_csum_datagram_msg(struct sk_buff *skb,return -EINVAL ; } -if (unlikely (skb->ip_summed ==CHECKSUM_COMPLETE )) -netdev_rx_csum_fault (skb->dev); +if (unlikely (skb->ip_summed ==CHECKSUM_COMPLETE ) && + !skb->csum_complete_sw) +netdev_rx_csum_fault (NULL ); }return 0 ; fault:
net/ipv4/udp.c +18 -2
@@ -2120,8 +2120,24 @@ static inline int udp4_csum_init(struct sk_buff *skb, struct udphdr *uh,/* Note, we are only interested in != 0 or == 0, thus the * force to int. */ -return (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, - inet_compute_pseudo); + err = (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, + inet_compute_pseudo); +if (err) +return err; + +if (skb->ip_summed ==CHECKSUM_COMPLETE && !skb->csum_valid) { +/* If SW calculated the value, we know it's bad */ +if (skb->csum_complete_sw) +return 1 ; + +/* HW says the value is bad. Let's validate that. +* skb->csum is no longer the full packet checksum, +* so don't treat it as such. +*/ +skb_checksum_complete_unset (skb); + } + +return 0 ; }/* wrapper for udp_queue_rcv_skb tacking care of csum conversion and
net/ipv6/ip6_checksum.c +18 -2
@@ -88,8 +88,24 @@ int udp6_csum_init(struct sk_buff *skb, struct udphdr *uh, int proto)* Note, we are only interested in != 0 or == 0, thus the * force to int. */ -return (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, - ip6_compute_pseudo); + err = (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, + ip6_compute_pseudo); +if (err) +return err; + +if (skb->ip_summed ==CHECKSUM_COMPLETE && !skb->csum_valid) { +/* If SW calculated the value, we know it's bad */ +if (skb->csum_complete_sw) +return 1 ; + +/* HW says the value is bad. Let's validate that. +* skb->csum is no longer the full packet checksum, +* so don't treat is as such. +*/ +skb_checksum_complete_unset (skb); + } + +return 0 ; }EXPORT_SYMBOL (udp6_csum_init);
Signature · 1 line
1.9.1
David Miller
maintainerFrom: Sean Tranchetti <[email protected]>
Date: Tue, 23 Oct 2018 16:04:31 -0600
Quoted 27 lines
Current handling of
CHECKSUM_COMPLETEpackets by the UDP stack is incorrect for any packet that has an incorrect checksum value.udp4/6_csum_init() will both make a call to
__skb_checksum_validate_complete()to initialize/validate the csum field when receiving aCHECKSUM_COMPLETEpacket. When this packet fails validation,skb->csumwill be overwritten with the pseudoheader checksum so the packet can be fully validated by software, but theskb->ip_summedvalue will be left asCHECKSUM_COMPLETEso that way the stack can later warn the user about their hardware spewing bad checksums. Unfortunately, leaving the SKB in this state can cause problems later on in the checksum calculation.Since the the packet is still marked as
CHECKSUM_COMPLETE,udp_csum_pull_header()will SUBTRACT the checksum of the UDP header fromskb->csuminstead of adding it, leaving us with a garbage value in that field. Once we try to copy the packet to userspace in the udp4/6_recvmsg(), we'll make a call toskb_copy_and_csum_datagram_msg()to checksum the packet data and add it in the garbageskb->csumvalue to perform our final validation check.Since the value we're validating is not the proper checksum, it's possible that the folded value could come out to 0, causing us not to drop the packet. Instead, we believe that the packet was checksummed incorrectly by hardware since
skb->ip_summedis stillCHECKSUM_COMPLETE, and we attempt to warn the user withnetdev_rx_csum_fault(skb->dev);
Quoted 3 lines
Unfortunately, since this is the UDP path,
skb->devhas been overwritten byskb->dev_scratchand is no longer a valid pointer, so we end up reading invalid memory.
Just want to say that it has always been complicated in this area due to the fact that we do this deferral of final checksum validation to when we copy the packet into userspace. For example, poll() needs to do special things, etc.
Because we have to make it seem as if we dropped the packet with a bad checksum from the point of view of what the user sees during recvmsg() and poll() calls. But until we do that checksum validation, we don't know exactly what the situation is.
Quoted 21 lines
This patch addresses this problem in two ways:
1) Do not use the dev pointer when callingnetdev_rx_csum_fault()fromskb_copy_and_csum_datagram_msg(). Since this gets called from the UDP path whereskb->devhas been overwritten, we have no way of knowing if the pointer is still valid. Also for the sake of consistency with the other uses ofnetdev_rx_csum_fault(), don't attempt to call it if the packet was checksummed by software.2) Add better
CHECKSUM_COMPLETEhandling to udp4/6_csum_init(). If we receive a packet that'sCHECKSUM_COMPLETEthat fails verification (i.e.skb->csum_valid== 0), check who performed the calculation. It's possible that the checksum was done in software by the network stack earlier (such as Netfilter's CONNTRACK module), and if that says the checksum is bad, we can drop the packet immediately instead of waiting until we try and copy it to userspace. Otherwise, we need to mark the SKB asCHECKSUM_NONE, since theskb->csumfield no longer contains the full packet checksum after the call to__skb_checksum_validate_complete().
Quoted 1 line
Fixes e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")
Can't count on my hands how many regressions are a result of that change and it's subtle side effects. :-/
Quoted 4 lines
Fixes c84d949057ca ("udp: copy skb->truesize in the first cache line")Cc Sam Kumar <[email protected]>Cc Eric Dumazet <[email protected]>Signed-off-by Sean Tranchetti <[email protected]>
Applied and queued up for -stable, thank you.
Possibly worth mentioning that without this fix you can also end up with valid udp packets being dropped (ie. the reverse of the commit description which talks about receiving invalid ones).
The (approximate?) requirement is:
(a) nic generates CHECKSUM_COMPLETE, but gets the actual checksum wrong
(b) udp packet is longer then 76 bytes L2 (CHECKSUM_BREAK), so we don't calculate checksum inline prior to pulling udp header
(c) udp socket has no bpf filter, so we delay checksum until we do copy to userspace from recvmsg()
76 bytes is 76 - 14 ether - 20 ipv4 - 8 udp header = 34 udp ipv4 payload bytes
Quoted 77 lines · David Miller
From: Sean Tranchetti <[email protected]>
Date: Tue, 23 Oct 2018 16:04:31 -0600Quoted 27 lines
Current handling of
CHECKSUM_COMPLETEpackets by the UDP stack is incorrect for any packet that has an incorrect checksum value.udp4/6_csum_init() will both make a call to
__skb_checksum_validate_complete()to initialize/validate the csum field when receiving aCHECKSUM_COMPLETEpacket. When this packet fails validation,skb->csumwill be overwritten with the pseudoheader checksum so the packet can be fully validated by software, but theskb->ip_summedvalue will be left asCHECKSUM_COMPLETEso that way the stack can later warn the user about their hardware spewing bad checksums. Unfortunately, leaving the SKB in this state can cause problems later on in the checksum calculation.Since the the packet is still marked as
CHECKSUM_COMPLETE,udp_csum_pull_header()will SUBTRACT the checksum of the UDP header fromskb->csuminstead of adding it, leaving us with a garbage value in that field. Once we try to copy the packet to userspace in the udp4/6_recvmsg(), we'll make a call toskb_copy_and_csum_datagram_msg()to checksum the packet data and add it in the garbageskb->csumvalue to perform our final validation check.Since the value we're validating is not the proper checksum, it's possible that the folded value could come out to 0, causing us not to drop the packet. Instead, we believe that the packet was checksummed incorrectly by hardware since
skb->ip_summedis stillCHECKSUM_COMPLETE, and we attempt to warn the user withnetdev_rx_csum_fault(skb->dev);Quoted 3 lines
Unfortunately, since this is the UDP path,
skb->devhas been overwritten byskb->dev_scratchand is no longer a valid pointer, so we end up reading invalid memory.Just want to say that it has always been complicated in this area due to the fact that we do this deferral of final checksum validation to when we copy the packet into userspace. For example,
poll()needs to do special things, etc.Because we have to make it seem as if we dropped the packet with a bad checksum from the point of view of what the user sees during
recvmsg()andpoll()calls. But until we do that checksum validation, we don't know exactly what the situation is.Quoted 21 lines
This patch addresses this problem in two ways:
1) Do not use the dev pointer when callingnetdev_rx_csum_fault()fromskb_copy_and_csum_datagram_msg(). Since this gets called from the UDP path whereskb->devhas been overwritten, we have no way of knowing if the pointer is still valid. Also for the sake of consistency with the other uses ofnetdev_rx_csum_fault(), don't attempt to call it if the packet was checksummed by software.2) Add better
CHECKSUM_COMPLETEhandling to udp4/6_csum_init(). If we receive a packet that'sCHECKSUM_COMPLETEthat fails verification (i.e.skb->csum_valid== 0), check who performed the calculation. It's possible that the checksum was done in software by the network stack earlier (such as Netfilter's CONNTRACK module), and if that says the checksum is bad, we can drop the packet immediately instead of waiting until we try and copy it to userspace. Otherwise, we need to mark the SKB asCHECKSUM_NONE, since theskb->csumfield no longer contains the full packet checksum after the call to__skb_checksum_validate_complete().Quoted 1 line
Fixes e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")Can't count on my hands how many regressions are a result of that change and it's subtle side effects. :-/
Quoted 4 lines
Fixes c84d949057ca ("udp: copy skb->truesize in the first cache line")Cc Sam Kumar <[email protected]>Cc Eric Dumazet <[email protected]>Signed-off-by Sean Tranchetti <[email protected]>Applied and queued up for -stable, thank you.
Hi Sean,
I was analyzing this code and have some concerns. Please see the comments below.
Quoted 69 lines · Sean Tranchetti
Current handling of
CHECKSUM_COMPLETEpackets by the UDP stack is incorrect for any packet that has an incorrect checksum value.udp4/6_csum_init() will both make a call to
__skb_checksum_validate_complete()to initialize/validate the csum field when receiving aCHECKSUM_COMPLETEpacket. When this packet fails validation,skb->csumwill be overwritten with the pseudoheader checksum so the packet can be fully validated by software, but theskb->ip_summedvalue will be left asCHECKSUM_COMPLETEso that way the stack can later warn the user about their hardware spewing bad checksums. Unfortunately, leaving the SKB in this state can cause problems later on in the checksum calculation.Since the the packet is still marked as
CHECKSUM_COMPLETE,udp_csum_pull_header()will SUBTRACT the checksum of the UDP header fromskb->csuminstead of adding it, leaving us with a garbage value in that field. Once we try to copy the packet to userspace in the udp4/6_recvmsg(), we'll make a call toskb_copy_and_csum_datagram_msg()to checksum the packet data and add it in the garbageskb->csumvalue to perform our final validation check.Since the value we're validating is not the proper checksum, it's possible that the folded value could come out to 0, causing us not to drop the packet. Instead, we believe that the packet was checksummed incorrectly by hardware since
skb->ip_summedis stillCHECKSUM_COMPLETE, and we attempt to warn the user withnetdev_rx_csum_fault(skb->dev);Unfortunately, since this is the UDP path,
skb->devhas been overwritten byskb->dev_scratchand is no longer a valid pointer, so we end up reading invalid memory.This patch addresses this problem in two ways:
1) Do not use the dev pointer when callingnetdev_rx_csum_fault()fromskb_copy_and_csum_datagram_msg(). Since this gets called from the UDP path whereskb->devhas been overwritten, we have no way of knowing if the pointer is still valid. Also for the sake of consistency with the other uses ofnetdev_rx_csum_fault(), don't attempt to call it if the packet was checksummed by software.2) Add better
CHECKSUM_COMPLETEhandling to udp4/6_csum_init(). If we receive a packet that'sCHECKSUM_COMPLETEthat fails verification (i.e.skb->csum_valid== 0), check who performed the calculation. It's possible that the checksum was done in software by the network stack earlier (such as Netfilter's CONNTRACK module), and if that says the checksum is bad, we can drop the packet immediately instead of waiting until we try and copy it to userspace. Otherwise, we need to mark the SKB asCHECKSUM_NONE, since theskb->csumfield no longer contains the full packet checksum after the call to__skb_checksum_validate_complete().Fixes e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")Fixes c84d949057ca ("udp: copy skb->truesize in the first cache line")Cc Sam Kumar <[email protected]>Cc Eric Dumazet <[email protected]>Signed-off-by Sean Tranchetti <[email protected]>--- net/core/datagram.c | 5 +++-- net/ipv4/udp.c | 20 ++++++++++++++++++-- net/ipv6/ip6_checksum.c | 20 ++++++++++++++++++-- 3 files changed, 39 insertions(+), 6 deletions(-)net/core/datagram.c +0
-0
net/core/datagram.c int skb_copy_and_csum_datagram_msg(struct sk_buff *skb, · line 813
Quoted 8 lines
net/core/datagram.c +3
-2@@ -808,8 +808,9 @@ int skb_copy_and_csum_datagram_msg(struct sk_buff *skb,return -EINVAL ; } -if (unlikely (skb->ip_summed ==CHECKSUM_COMPLETE )) -netdev_rx_csum_fault (skb->dev); +if (unlikely (skb->ip_summed ==CHECKSUM_COMPLETE ) && + !skb->csum_complete_sw) +netdev_rx_csum_fault (NULL );
Here you are fixing skb_copy_and_csum_datagram_msg, but this is not the only flow that leads to netdev_rx_csum_fault. This function is also called from __skb_checksum_complete (also after using the screwed up skb->csum), and there are two ways we can get to __skb_checksum_complete:
1. Directly from skb_copy_and_csum_datagram_msg.
2. udpv6_recvmsg -> __udp_lib_checksum_complete -> __skb_checksum_complete:
if (copied < ulen || peeking ||
(is_udplite && UDP_SKB_CB(skb)->partial_cov)) {
checksum_valid = udp_skb_csum_unnecessary(skb) ||
!__udp_lib_checksum_complete(skb);
if (!checksum_valid)
goto csum_copy_err;
}It looks to me, as if these flows may also lead to parsing skb->dev_scratch as skb->dev.
Quoted 49 lines
net/core/datagram.c +0
-0}return 0 ; fault:net/ipv4/udp.c +18
-2@@ -2120,8 +2120,24 @@ static inline int udp4_csum_init(struct sk_buff *skb, struct udphdr *uh,/* Note, we are only interested in != 0 or == 0, thus the * force to int. */ -return (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, - inet_compute_pseudo); + err = (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, + inet_compute_pseudo); +if (err) +return err; + +if (skb->ip_summed ==CHECKSUM_COMPLETE && !skb->csum_valid) { +/* If SW calculated the value, we know it's bad */ +if (skb->csum_complete_sw) +return 1 ; + +/* HW says the value is bad. Let's validate that. +* skb->csum is no longer the full packet checksum, +* so don't treat it as such. +*/ +skb_checksum_complete_unset (skb); + } + +return 0 ; }/* wrapper for udp_queue_rcv_skb tacking care of csum conversion and net/ipv6/ip6_checksum.c +5
-2@@ -88,8 +88,24 @@ int udp6_csum_init(struct sk_buff *skb, struct udphdr *uh, int proto)* Note, we are only interested in != 0 or == 0, thus the * force to int. */ -return (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, - ip6_compute_pseudo); + err = (__force int )skb_checksum_init_zero_check (skb, proto, uh->check, + ip6_compute_pseudo); +if (err) +return err; +
net/ipv6/ip6_checksum.c int udp6_csum_init(struct sk_buff *skb, struct udphdr *uh, int proto) · line 105
Quoted 10 lines
net/ipv6/ip6_checksum.c +10
-0+if (skb->ip_summed ==CHECKSUM_COMPLETE && !skb->csum_valid) { +/* If SW calculated the value, we know it's bad */ +if (skb->csum_complete_sw) +return 1 ; + +/* HW says the value is bad. Let's validate that. +* skb->csum is no longer the full packet checksum, +* so don't treat is as such. +*/ +skb_checksum_complete_unset (skb);
If I understand it correctly, we'll have ip_summed == CHECKSUM_NONE after this point, so __skb_checksum_complete won't call netdev_rx_csum_fault anymore in case of bad hardware that calculates checksums incorrectly.
net/ipv6/ip6_checksum.c int udp6_csum_init(struct sk_buff *skb, struct udphdr *uh, int proto) · line 111
Quoted 6 lines
net/ipv6/ip6_checksum.c +3
-0+ } + +return 0 ; }EXPORT_SYMBOL (udp6_csum_init);
Are my concerns real, or am I missing anything? If my points are valid, do you have ideas how it could be fixed?
Thanks and looking forward to your reply.