Skip to contentSkip to conversation

2.6.25-rc: Null dereference in ip_defrag

PATCH Phil Oester 4 participants 12 messages Mar 17, 2008 – Mar 20, 2008

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

Phil Oester

author
#0 lore raw

Been seeing occasional panics in my testing of 2.6.25-rc in ip_defrag. Offending line in ip_defrag is here:

	net = skb->dev->nd_net

where dev is NULL. Bisected the problem down to commit ac18e7509e7df327e30d6e073a787d922eaf211d ([NETNS][FRAGS]: Make the inet_frag_queue lookup work in namespaces).

To prevent panic, I added the below patch (whitespace damaged):

net/ipv4/ip_fragment.c +8 -0
@@ -568,6 +568,14 @@ int ip_defrag(struct sk_buff *skb, u32 user)
 
        IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
 
+       if (!skb->dev) {
+               printk("ip_defrag_bug: %u.%u.%u.%u -> %u.%u.%u.%u\n",
+                      NIPQUAD(ip_hdr(skb)->saddr), NIPQUAD(ip_hdr(skb)->daddr));
+               WARN_ON(1);
+               kfree_skb(skb);
+               return -ENOMEM;
+       }
+

And the packets causing the problem are all multicast fragments being generated by Quagga's OSPFD (see debug output below). Tried manually generating some multicast fragments with iperf, but couldn't reproduce it.

Any ideas?

Phil

ip_defrag_bug: 10.253.13.122 -> 224.0.0.5

 ------------[ cut here ]------------
 WARNING: at net/ipv4/ip_fragment.c:574 ip_defrag+0x9d/0xa0d()
 Pid: 1662, comm: ospfd Not tainted 2.6.25-rc4 #4
 
 Call Trace:
  [<ffffffff8022742a>] warn_on_slowpath+0x53/0x66
  [<ffffffff80228230>] ? printk+0x67/0x69
  [<ffffffff803b36f5>] ? skb_release_data+0xa8/0xad
  [<ffffffff803b35b3>] ? __kfree_skb+0x74/0x78
  [<ffffffff803d77c7>] ip_defrag+0x9d/0xa0d
  [<ffffffff803b15c6>] ? sock_def_write_space+0x18/0x89
  [<ffffffff80404324>] ipv4_conntrack_defrag+0x67/0x96
  [<ffffffff803caa7b>] nf_iterate+0x41/0x81
  [<ffffffff803f29b0>] ? dst_output+0x0/0x10
  [<ffffffff803cab19>] nf_hook_slow+0x5e/0xbe
  [<ffffffff803f29b0>] ? dst_output+0x0/0x10
  [<ffffffff803f3b6e>] raw_sendmsg+0x586/0x758
  [<ffffffff803fb169>] inet_sendmsg+0x46/0x53
  [<ffffffff803adef9>] sock_sendmsg+0xdf/0xf8
  [<ffffffff80421cd1>] ? _spin_lock_bh+0x11/0x29
  [<ffffffff803afbfc>] ? release_sock+0x9b/0xa3
  [<ffffffff80238b37>] ? autoremove_wake_function+0x0/0x38
  [<ffffffff803ad359>] ? move_addr_to_kernel+0x25/0x35
  [<ffffffff803c4c6f>] ? verify_compat_iovec+0x60/0x9e
  [<ffffffff803ae0f3>] sys_sendmsg+0x1e1/0x253
  [<ffffffff802332ee>] ? getrusage+0x1c9/0x1e6
  [<ffffffff804206d4>] ? thread_return+0x3d/0x9c
  [<ffffffff803c45d0>] compat_sys_sendmsg+0xf/0x11
  [<ffffffff803c4e82>] compat_sys_socketcall+0x13f/0x158
  [<ffffffff8021cc12>] sysenter_do_call+0x1b/0x66
 
 ---[ end trace 48218d00aa061d3c ]---
#1 lore raw
Quoted 4 lines · Phil Oester

Been seeing occasional panics in my testing of 2.6.25-rc in ip_defrag. Offending line in ip_defrag is here:

	net = skb->dev->nd_net

Yikes :(

Quoted 25 lines

where dev is NULL. Bisected the problem down to commit ac18e7509e7df327e30d6e073a787d922eaf211d ([NETNS][FRAGS]: Make the inet_frag_queue lookup work in namespaces).

To prevent panic, I added the below patch (whitespace damaged):

net/ipv4/ip_fragment.c +8 -0
@@ -568,6 +568,14 @@ int ip_defrag(struct sk_buff *skb, u32 user)
 
        IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
 
+       if (!skb->dev) {
+               printk("ip_defrag_bug: %u.%u.%u.%u -> %u.%u.%u.%u\n",
+                      NIPQUAD(ip_hdr(skb)->saddr), NIPQUAD(ip_hdr(skb)->daddr));
+               WARN_ON(1);
+               kfree_skb(skb);
+               return -ENOMEM;
+       }
+

And the packets causing the problem are all multicast fragments being generated by Quagga's OSPFD (see debug output below). Tried manually generating some multicast fragments with iperf, but couldn't reproduce it.

Quoted 1 line

Any ideas?

This is the same as the problem fixed here:

commit 4136cd523eb0c0bd53173e16fd7406d31d05824f [IPV4]: route: fix crash ip_route_input

The sk_buff does not have a valid dev sometimes in ip_defrag() :( and you have to setup conntrack rules to make packets go this way. But unlike the above problem we cannot even trust the skb->dst to be not NULL...

Can you check with this patch, please (untested, but should work)?

include/net/ip.h +2 -1
index 9f50d4f..fc6b650 100644
@@ -330,7 +330,8 @@ enum ip_defrag_users
 	IP_DEFRAG_VS_FWD
 };
 
-int ip_defrag(struct sk_buff *skb, u32 user);
+struct net;
+int ip_defrag(struct net *net, struct sk_buff *skb, u32 user);
 int ip_frag_mem(struct net *net);
 int ip_frag_nqueues(struct net *net);
net/ipv4/ip_fragment.c +1 -3
index a2e92f9..aad6652 100644
@@ -561,14 +561,12 @@ out_fail:
 }
 
 /* Process an incoming IP datagram fragment. */
-int ip_defrag(struct sk_buff *skb, u32 user)
+int ip_defrag(struct net *net, struct sk_buff *skb, u32 user)
 {
 	struct ipq *qp;
-	struct net *net;
 
 	IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
 
-	net = skb->dev->nd_net;
 	/* Start by cleaning up the memory. */
 	if (atomic_read(&net->ipv4.frags.mem) > net->ipv4.frags.high_thresh)
 		ip_evictor(net);
net/ipv4/ip_input.c +3 -2
index 6563139..25c6846 100644
@@ -172,7 +172,8 @@ int ip_call_ra_chain(struct sk_buff *skb)
 		    (!sk->sk_bound_dev_if ||
 		     sk->sk_bound_dev_if == skb->dev->ifindex)) {
 			if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
-				if (ip_defrag(skb, IP_DEFRAG_CALL_RA_CHAIN)) {
+				if (ip_defrag(skb->dev->nd_net, skb,
+						IP_DEFRAG_CALL_RA_CHAIN)) {
 					read_unlock(&ip_ra_lock);
 					return 1;
 				}

@@ -256,7 +257,7 @@ int ip_local_deliver(struct sk_buff *skb)
 	 */
 
 	if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
-		if (ip_defrag(skb, IP_DEFRAG_LOCAL_DELIVER))
+		if (ip_defrag(skb->dev->nd_net, skb, IP_DEFRAG_LOCAL_DELIVER))
 			return 0;
 	}
net/ipv4/ipvs/ip_vs_core.c +1 -1
index 963981a..d2c3c0f 100644
@@ -506,7 +506,7 @@ __sum16 ip_vs_checksum_complete(struct sk_buff *skb, int offset)
 
 static inline int ip_vs_gather_frags(struct sk_buff *skb, u_int32_t user)
 {
-	int err = ip_defrag(skb, user);
+	int err = ip_defrag(&init_net, skb, user);
 
 	if (!err)
 		ip_send_check(ip_hdr(skb));
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c +14 -6
index a65b845..00a0f63 100644
@@ -58,14 +58,15 @@ static int ipv4_print_tuple(struct seq_file *s,
 }
 
 /* Returns new sk_buff, or NULL */
-static int nf_ct_ipv4_gather_frags(struct sk_buff *skb, u_int32_t user)
+static int nf_ct_ipv4_gather_frags(struct net *net, struct sk_buff *skb,
+		u_int32_t user)
 {
 	int err;
 
 	skb_orphan(skb);
 
 	local_bh_disable();
-	err = ip_defrag(skb, user);
+	err = ip_defrag(net, skb, user);
 	local_bh_enable();
 
 	if (!err)

@@ -138,6 +139,9 @@ static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
 					  const struct net_device *out,
 					  int (*okfn)(struct sk_buff *))
 {
+	u_int32_t user;
+	struct net *net;
+
 	/* Previously seen (loopback)?  Ignore.  Do this before
 	   fragment check. */
 	if (skb->nfct)

@@ -145,10 +149,14 @@ static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
 
 	/* Gather fragments. */
 	if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
-		if (nf_ct_ipv4_gather_frags(skb,
-					    hooknum == NF_INET_PRE_ROUTING ?
-					    IP_DEFRAG_CONNTRACK_IN :
-					    IP_DEFRAG_CONNTRACK_OUT))
+		if (hooknum == NF_INET_PRE_ROUTING) {
+			user = IP_DEFRAG_CONNTRACK_IN;
+			net = in->nd_net;
+		} else {
+			user = IP_DEFRAG_CONNTRACK_OUT;
+			net = out->nd_net;
+		}
+		if (nf_ct_ipv4_gather_frags(net, skb, user))
 			return NF_STOLEN;
 	}
 	return NF_ACCEPT;
Quoted 41 lines

Phil

ip_defrag_bug: 10.253.13.122 -> 224.0.0.5

 ------------[ cut here ]------------
 WARNING: at net/ipv4/ip_fragment.c:574 ip_defrag+0x9d/0xa0d()
 Pid: 1662, comm: ospfd Not tainted 2.6.25-rc4 #4
 
 Call Trace:
  [<ffffffff8022742a>] warn_on_slowpath+0x53/0x66
  [<ffffffff80228230>] ? printk+0x67/0x69
  [<ffffffff803b36f5>] ? skb_release_data+0xa8/0xad
  [<ffffffff803b35b3>] ? __kfree_skb+0x74/0x78
  [<ffffffff803d77c7>] ip_defrag+0x9d/0xa0d
  [<ffffffff803b15c6>] ? sock_def_write_space+0x18/0x89
  [<ffffffff80404324>] ipv4_conntrack_defrag+0x67/0x96
  [<ffffffff803caa7b>] nf_iterate+0x41/0x81
  [<ffffffff803f29b0>] ? dst_output+0x0/0x10
  [<ffffffff803cab19>] nf_hook_slow+0x5e/0xbe
  [<ffffffff803f29b0>] ? dst_output+0x0/0x10
  [<ffffffff803f3b6e>] raw_sendmsg+0x586/0x758
  [<ffffffff803fb169>] inet_sendmsg+0x46/0x53
  [<ffffffff803adef9>] sock_sendmsg+0xdf/0xf8
  [<ffffffff80421cd1>] ? _spin_lock_bh+0x11/0x29
  [<ffffffff803afbfc>] ? release_sock+0x9b/0xa3
  [<ffffffff80238b37>] ? autoremove_wake_function+0x0/0x38
  [<ffffffff803ad359>] ? move_addr_to_kernel+0x25/0x35
  [<ffffffff803c4c6f>] ? verify_compat_iovec+0x60/0x9e
  [<ffffffff803ae0f3>] sys_sendmsg+0x1e1/0x253
  [<ffffffff802332ee>] ? getrusage+0x1c9/0x1e6
  [<ffffffff804206d4>] ? thread_return+0x3d/0x9c
  [<ffffffff803c45d0>] compat_sys_sendmsg+0xf/0x11
  [<ffffffff803c4e82>] compat_sys_socketcall+0x13f/0x158
  [<ffffffff8021cc12>] sysenter_do_call+0x1b/0x66
 
 ---[ end trace 48218d00aa061d3c ]---

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

#2 lore raw
Quoted 9 lines · Pavel Emelyanov
Quoted 5 lines · Phil Oester

And the packets causing the problem are all multicast fragments being generated by Quagga's OSPFD (see debug output below). Tried manually generating some multicast fragments with iperf, but couldn't reproduce it.

Any ideas?

This is the same as the problem fixed here:

Quoted 7 lines

commit 4136cd523eb0c0bd53173e16fd7406d31d05824f [IPV4]: route: fix crash ip_route_input

The sk_buff does not have a valid dev sometimes in ip_defrag() :( and you have to setup conntrack rules to make packets go this way. But unlike the above problem we cannot even trust the skb->dst to be not NULL...

We can on output. Usually we don't even see fragments in conntrack on output since we've defer fragmentation until all netfilter hooks have been processed. Quagga is generating fragments using raw sockets and IP_HDRINCL though.

Quoted 1 line

Can you check with this patch, please (untested, but should work)?

This is getting pretty ugly. Shouldn't

int ip_defrag(struct sk_buff *skb, u32 user)
{
...
- net = skb->dev->nd_net;
+ net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;

work as well?

#3 lore raw
Quoted 33 lines · Patrick McHardy
Quoted 9 lines · Pavel Emelyanov
Quoted 5 lines · Phil Oester

And the packets causing the problem are all multicast fragments being generated by Quagga's OSPFD (see debug output below). Tried manually generating some multicast fragments with iperf, but couldn't reproduce it.

Any ideas?

This is the same as the problem fixed here:

Quoted 7 lines

commit 4136cd523eb0c0bd53173e16fd7406d31d05824f [IPV4]: route: fix crash ip_route_input

The sk_buff does not have a valid dev sometimes in ip_defrag() :( and you have to setup conntrack rules to make packets go this way. But unlike the above problem we cannot even trust the skb->dst to be not NULL...

We can on output. Usually we don't even see fragments in conntrack on output since we've defer fragmentation until all netfilter hooks have been processed. Quagga is generating fragments using raw sockets and IP_HDRINCL though.

Quoted 1 line

Can you check with this patch, please (untested, but should work)?

This is getting pretty ugly. Shouldn't

int ip_defrag(struct sk_buff *skb, u32 user)
{
...

(quoted diff) +1 -1
-         net = skb->dev->nd_net;
+         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
Quoted 1 line

From my POW the we can just get skb->dst->dev. Could we?

I think that on IP level dealing with fragments we always have the destination entry. I'll recheck this tomorrow.

#4 lore raw
Quoted 14 lines · Denis V. Lunev
Quoted 11 lines · Patrick McHardy
Quoted 1 line · Pavel Emelyanov

Can you check with this patch, please (untested, but should work)?

This is getting pretty ugly. Shouldn't

int ip_defrag(struct sk_buff *skb, u32 user)
{
...

(quoted diff) +1 -1
-         net = skb->dev->nd_net;
+         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
Quoted 1 line

From my POW the we can just get skb->dst->dev. Could we?

Quoted 2 lines

I think that on IP level dealing with fragments we always have the destination entry. I'll recheck this tomorrow.

Unfortunately no, on the receive path connection tracking will call ip_defrag before routing took place.

author
#5 lore raw
In reply to Patrick McHardy
Quoted 11 lines · Patrick McHardy
Quoted 1 line · Pavel Emelyanov

Can you check with this patch, please (untested, but should work)?

This is getting pretty ugly. Shouldn't

int ip_defrag(struct sk_buff *skb, u32 user)
{
...

(quoted diff) +1 -1
-         net = skb->dev->nd_net;
+         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
Quoted 1 line

work as well?

I can try either patch, but Patrick's looks simpler and thus perhaps a better candidate this late in the -rc stage.

Pavel - you concur?

Phil

#6 lore raw
Quoted 5 lines · Phil Oester
Quoted 4 lines · Patrick McHardy
Quoted 1 line · Pavel Emelyanov

Can you check with this patch, please (untested, but should work)?

This is getting pretty ugly. Shouldn't

Quoted 7 lines
Quoted 7 lines

int ip_defrag(struct sk_buff *skb, u32 user)
{
...

(quoted diff) +1 -1
-         net = skb->dev->nd_net;
+         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;

work as well?

It will, though I do not like such implicit decisions.

Quoted 4 lines

I can try either patch, but Patrick's looks simpler and thus perhaps a better candidate this late in the -rc stage.

Pavel - you concur?

No one, except for a complaint, that we (me and Denis) warned other net namespaces developers that getting a struct net from function arguments, sockets, dst entries, devices, etc. is going to become a pain in the ... anyway. We proposed to create current_net macro, that would give us a correct net in any place... Such thing would allow us to avoid this problem. But I'm afraid, that it's just too late to change the model :(

Quoted 2 lines

Phil

#7 lore raw

[snip]

Quoted 6 lines
Quoted 4 lines

I can try either patch, but Patrick's looks simpler and thus perhaps a better candidate this late in the -rc stage.

Pavel - you concur?

No one, except for a complaint, ...

I wonder what I was thinking about when I wrote this :(

Phil, I agree with Patrick's patch.

author
#8 lore raw
Quoted 6 lines · Pavel Emelyanov
Quoted 4 lines
Quoted 4 lines

I can try either patch, but Patrick's looks simpler and thus perhaps a better candidate this late in the -rc stage.

Pavel - you concur?

Phil, I agree with Patrick's patch.

And it has survived 1.5 days without issue. Patrick - will you be pushing to David?

Phil

#9 lore raw
Quoted 9 lines · Phil Oester
Quoted 5 lines · Pavel Emelyanov
Quoted 4 lines
Quoted 4 lines

I can try either patch, but Patrick's looks simpler and thus perhaps a better candidate this late in the -rc stage.

Pavel - you concur?

Phil, I agree with Patrick's patch.

And it has survived 1.5 days without issue. Patrick - will you be pushing to David?

Could you send the patch you've been using?

author Tested-by
#10 lore raw
Quoted 4 lines · Patrick McHardy
Quoted 2 lines

And it has survived 1.5 days without issue. Patrick - will you be pushing to David?

Could you send the patch you've been using?

Sure - attached below against current git. I assume you will add your signoff, but you can add

Tested-by Phil Oester <[email protected]>

Phil

net/ipv4/ip_fragment.c +1 -1
index a2e92f9..3b2e5ad 100644
@@ -568,7 +568,7 @@ int ip_defrag(struct sk_buff *skb, u32 user)
 
 	IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
 
-	net = skb->dev->nd_net;
+	net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
 	/* Start by cleaning up the memory. */
 	if (atomic_read(&net->ipv4.frags.mem) > net->ipv4.frags.high_thresh)
 		ip_evictor(net);