[Patch net] net: drop skb on failure in ip_check_defrag()

[Patch net] net: drop skb on failure in ip_check_defrag()

[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.

Found this during code review.

Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends")
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/ipv4/ip_fragment.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 9b0158fa431f..d6ee343fdb86 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -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;
-- 
2.14.5

Re: [Patch net] net: drop skb on failure in ip_check_defrag()

[Eric Dumazet]

On 11/01/2018 12:02 PM, Cong Wang wrote:
> 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 <edumazet@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Thanks Cong !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [Patch net] net: drop skb on failure in ip_check_defrag()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu,  1 Nov 2018 12:02:37 -0700

> 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 <edumazet@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable, thanks!