Skip to contentSkip to conversation

net: drop skb on failure in ip_check_defrag()

PATCH In review Cong Wang 3 participants 3 messages Nov 2, 2018

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Cong Wang

author
#0 lore raw

PATCH 1/1 net: drop skb on failure in ip_check_defrag()

Most callers of pskb_trim_rcsum() simply drop the skb when it fails, however, ip_check_defrag() still continues to pass the skb up to stack. This is suspicious.

In ip_check_defrag(), after we learn the skb is an IP fragment, passing the skb to callers makes no sense, because callers expect fragments are defrag'ed on success. So, dropping the skb when we can't defrag it is reasonable.

Note, prior to commit 88078d98d1bb, this is not a big problem as checksum will be fixed up anyway. After it, the checksum is not correct on failure.

Found this during code review.

Fixes 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends")
Cc Eric Dumazet <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
 net/ipv4/ip_fragment.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
net/ipv4/ip_fragment.c +8 -4
index 9b0158fa431f..d6ee343fdb86 100644
@@ -722,10 +722,14 @@ struct sk_buff *ip_check_defrag(struct net *net, struct sk_buff *skb, u32 user)
 	if (ip_is_fragment(&iph)) {
 		skb = skb_share_check(skb, GFP_ATOMIC);
 		if (skb) {
-			if (!pskb_may_pull(skb, netoff + iph.ihl * 4))
-				return skb;
-			if (pskb_trim_rcsum(skb, netoff + len))
-				return skb;
+			if (!pskb_may_pull(skb, netoff + iph.ihl * 4)) {
+				kfree_skb(skb);
+				return NULL;
+			}
+			if (pskb_trim_rcsum(skb, netoff + len)) {
+				kfree_skb(skb);
+				return NULL;
+			}
 			memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
 			if (ip_defrag(net, skb, user))
 				return NULL;
Signature · 1 line
2.14.5
Reviewed-by on v1 1/1
#1 lore raw
Quoted 13 lines · Cong Wang

Most callers of pskb_trim_rcsum() simply drop the skb when it fails, however, ip_check_defrag() still continues to pass the skb up to stack. This is suspicious.

In ip_check_defrag(), after we learn the skb is an IP fragment, passing the skb to callers makes no sense, because callers expect fragments are defrag'ed on success. So, dropping the skb when we can't defrag it is reasonable.

Note, prior to commit 88078d98d1bb, this is not a big problem as checksum will be fixed up anyway. After it, the checksum is not correct on failure.

Quoted 5 lines

Found this during code review.

Fixes 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends")
Cc Eric Dumazet <[email protected]>
Signed-off-by Cong Wang <[email protected]>

Thanks Cong !

Reviewed-by Eric Dumazet <[email protected]>
maintainer
#2 lore raw
In reply to Cong Wang

From: Cong Wang <[email protected]>
Date: Thu, 1 Nov 2018 12:02:37 -0700

Quoted 13 lines

Most callers of pskb_trim_rcsum() simply drop the skb when it fails, however, ip_check_defrag() still continues to pass the skb up to stack. This is suspicious.

In ip_check_defrag(), after we learn the skb is an IP fragment, passing the skb to callers makes no sense, because callers expect fragments are defrag'ed on success. So, dropping the skb when we can't defrag it is reasonable.

Note, prior to commit 88078d98d1bb, this is not a big problem as checksum will be fixed up anyway. After it, the checksum is not correct on failure.

Quoted 5 lines

Found this during code review.

Fixes 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends")
Cc Eric Dumazet <[email protected]>
Signed-off-by Cong Wang <[email protected]>

Applied and queued up for -stable, thanks!