Skip to contentSkip to conversation

ipqueue allocation failure.

PATCH Applied Dave Jones 5 participants 12 messages Apr 20, 2011 – Jun 7, 2011

Subsystems (3)
Networking Network drivers netfilter
lore
Share & tools
lore

Dave Jones

author
#0 lore raw

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Dave

------------[ cut here ]------------
WARNING: at mm/page_alloc.c:2032 __alloc_pages_nodemask+0x17d/0x7e6()
Hardware name: GA-MA78GM-S2H
Modules linked in: rfcomm hidp can_raw can_bcm sctp libcrc32c ip_queue decnet pppoe pppox ppp_generic slhc can ipx p8022 p8023 phonet bluetooth rfkill a]
Pid: 20393, comm: trinity Not tainted 2.6.39-rc4+ #6
Call Trace:
 [<ffffffff81056a9c>] warn_slowpath_common+0x83/0x9b
 [<ffffffff81056ace>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff810ed743>] __alloc_pages_nodemask+0x17d/0x7e6
 [<ffffffff811206c9>] ? check_object+0x174/0x1ae
 [<ffffffff81120a43>] ? check_slab+0xc7/0xd5
 [<ffffffff813f42fe>] ? __alloc_skb+0x40/0x133
 [<ffffffff813f42fe>] ? __alloc_skb+0x40/0x133
 [<ffffffff814b7487>] kmalloc_large_node+0x56/0x95
 [<ffffffff811235c1>] __kmalloc_node_track_caller+0x32/0x139
 [<ffffffff81421b68>] ? netlink_ack+0x4a/0xe8
 [<ffffffffa0500475>] ? ipq_rcv_skb+0x27/0x340 [ip_queue]
 [<ffffffff813f4333>] __alloc_skb+0x75/0x133
 [<ffffffff81421b68>] netlink_ack+0x4a/0xe8
 [<ffffffffa050076a>] ipq_rcv_skb+0x31c/0x340 [ip_queue]
 [<ffffffff8142174a>] netlink_unicast+0xec/0x156
 [<ffffffff81421a33>] netlink_sendmsg+0x27f/0x2c0
 [<ffffffff813ed76c>] __sock_sendmsg+0x69/0x75
 [<ffffffff813ed834>] sock_aio_write+0xbc/0xcc
 [<ffffffff8113242b>] do_sync_write+0xbf/0xff
 [<ffffffff81206e18>] ? security_file_permission+0x2e/0x33
 [<ffffffff81132861>] ? rw_verify_area+0xb6/0xd3
 [<ffffffff81132b02>] vfs_write+0xb6/0xf6
 [<ffffffff8113400b>] ? fget_light+0x3a/0xa1
 [<ffffffff81132cf6>] sys_write+0x4d/0x74
 [<ffffffff814c5d82>] system_call_fastpath+0x16/0x1b
---[ end trace cd56dc75cfeab436 ]---
maintainer
#1 lore raw

From: Dave Jones <[email protected]>
Date: Tue, 19 Apr 2011 21:42:22 -0400

Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Maybe the thing to do is to simply pass __GFP_NOWARN to nlmsg_new() in netlink_ack()?

Anyone else have a better idea?

#2 lore raw
Quoted 12 lines · David Miller

From: Dave Jones <[email protected]>
Date: Tue, 19 Apr 2011 21:42:22 -0400

Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Quoted 5 lines

Maybe the thing to do is to simply pass __GFP_NOWARN to nlmsg_new() in netlink_ack()?

Anyone else have a better idea?
--

It should be fine, since we already checked sk->sk_sndbuf in netlink_sendmsg()

if (len > sk->sk_sndbuf - 32)

	goto out;

Maybe the 32 magic value is too small to take into account struct nlmsgerr that is prepended in netlink_ack()

Hmm, no, sizeof(struct nlmsgerr) is 20

author
#3 lore raw

PATCH 1/1 Use unsigned variables for packet lengths in ip[6]_queue.

In reply to David Miller
Quoted 12 lines · David Miller

From: Dave Jones <[email protected]>
Date: Tue, 19 Apr 2011 21:42:22 -0400

Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Quoted 4 lines

Maybe the thing to do is to simply pass __GFP_NOWARN to nlmsg_new() in netlink_ack()?

Anyone else have a better idea?

So I went back to this today, and found something that doesn't look right. After adding some instrumentation, and re-running my tests, I found that the reason we were blowing up with enormous allocations was that we were passing down a nlmsglen's like -1061109568

Is there any reason for that to be signed ?
The nlmsg_len entry of nlmsghdr is a u32, so I'm assuming this is a bug.

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

Dave

Signature · 3 lines
Netlink message lengths can't be negative, so use unsigned variables.

Signed-off-by: Dave Jones <[email protected]>
net/ipv4/netfilter/ip_queue.c +2 -1
index d2c1311..f7f9bd7 100644
@@ -402,7 +402,8 @@ ipq_dev_drop(int ifindex)
 static inline void
 __ipq_rcv_skb(struct sk_buff *skb)
 {
-	int status, type, pid, flags, nlmsglen, skblen;
+	int status, type, pid, flags;
+	unsigned int nlmsglen, skblen;
 	struct nlmsghdr *nlh;
 
 	skblen = skb->len;
net/ipv6/netfilter/ip6_queue.c +2 -1
index 413ab07..065fe40 100644
@@ -403,7 +403,8 @@ ipq_dev_drop(int ifindex)
 static inline void
 __ipq_rcv_skb(struct sk_buff *skb)
 {
-	int status, type, pid, flags, nlmsglen, skblen;
+	int status, type, pid, flags;
+	unsigned int nlmsglen, skblen;
 	struct nlmsghdr *nlh;
 
 	skblen = skb->len;
author
#4 lore raw

Re: [PATCH] Use unsigned variables for packet lengths in ip[6]_queue.

Quoted 19 lines · Dave Jones
Quoted 9 lines
Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Quoted 4 lines

Maybe the thing to do is to simply pass __GFP_NOWARN to nlmsg_new() in netlink_ack()?

Anyone else have a better idea?

So I went back to this today, and found something that doesn't look right. After adding some instrumentation, and re-running my tests, I found that the reason we were blowing up with enormous allocations was that we were passing down a nlmsglen's like -1061109568

Quoted 6 lines

Is there any reason for that to be signed ?
The nlmsg_len entry of nlmsghdr is a u32, so I'm assuming this is a bug.

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

any feedback on this ? am I barking up the wrong tree ?

Dave

maintainer
#5 lore raw

Re: [PATCH] Use unsigned variables for packet lengths in ip[6]_queue.

In reply to Dave Jones

From: Dave Jones <[email protected]>
Date: Fri, 27 May 2011 20:36:51 -0400

Quoted 23 lines · David Miller
Quoted 12 lines

From: Dave Jones <[email protected]>
Date: Tue, 19 Apr 2011 21:42:22 -0400

Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Quoted 4 lines

Maybe the thing to do is to simply pass __GFP_NOWARN to nlmsg_new() in netlink_ack()?

Anyone else have a better idea?

So I went back to this today, and found something that doesn't look right. After adding some instrumentation, and re-running my tests, I found that the reason we were blowing up with enormous allocations was that we were passing down a nlmsglen's like -1061109568

Quoted 6 lines

Is there any reason for that to be signed ?
The nlmsg_len entry of nlmsghdr is a u32, so I'm assuming this is a bug.

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

netfilter-devel and maintainers CC:'d

Quoted 34 lines
Signature · 3 lines
Netlink message lengths can't be negative, so use unsigned variables.

Signed-off-by: Dave Jones <[email protected]>
net/ipv4/netfilter/ip_queue.c +2 -1
index d2c1311..f7f9bd7 100644
@@ -402,7 +402,8 @@ ipq_dev_drop(int ifindex)
 static inline void
 __ipq_rcv_skb(struct sk_buff *skb)
 {
-	int status, type, pid, flags, nlmsglen, skblen;
+	int status, type, pid, flags;
+	unsigned int nlmsglen, skblen;
 	struct nlmsghdr *nlh;
 
 	skblen = skb->len;
net/ipv6/netfilter/ip6_queue.c +2 -1
index 413ab07..065fe40 100644
@@ -403,7 +403,8 @@ ipq_dev_drop(int ifindex)
 static inline void
 __ipq_rcv_skb(struct sk_buff *skb)
 {
-	int status, type, pid, flags, nlmsglen, skblen;
+	int status, type, pid, flags;
+	unsigned int nlmsglen, skblen;
 	struct nlmsghdr *nlh;
 
 	skblen = skb->len;
#6 lore raw

Re: [PATCH] Use unsigned variables for packet lengths in ip[6]_queue.

Quoted 32 lines · David Miller

From: Dave Jones <[email protected]>
Date: Fri, 27 May 2011 20:36:51 -0400

Quoted 29 lines · David Miller
Quoted 12 lines

From: Dave Jones <[email protected]>
Date: Tue, 19 Apr 2011 21:42:22 -0400

Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Quoted 4 lines

Maybe the thing to do is to simply pass __GFP_NOWARN to nlmsg_new() in netlink_ack()?

Anyone else have a better idea?

So I went back to this today, and found something that doesn't look right. After adding some instrumentation, and re-running my tests, I found that the reason we were blowing up with enormous allocations was that we were passing down a nlmsglen's like -1061109568

Is there any reason for that to be signed ?
The nlmsg_len entry of nlmsghdr is a u32, so I'm assuming this is a bug.

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

This is fine, but I'm wondering whether this can really fix the problem you've been seeing. Before the packet is reallocated, the length of nlmsglen - NLMSGLEN(0) - sizeof(struct ipq_peer_msg) is compared to ipq_peer_msg->data_len, so both values need to be wrong. ipq_peer_msg->data_len is a size_t, so it's unsigned.

I think what we should additionally do is verify that data_len < 65535 since that's the maximum size of an IP packet.

Using __GFP_NOWARN also makes sense in my opinion since ip_queue prints a warning anyways and we return an errno code to userspace. On second thought, we could also simply use GFP_KERNEL, AFAICS packet reinjection does not happen in atomic context. I'll give that a try.

net/ipv4/netfilter/ip_queue.c +3 -0
index f7f9bd7..8ded42d 100644
@@ -279,6 +279,9 @@ ipq_mangle_ipv4(ipq_verdict_msg_t *v, struct nf_queue_entry *e)
 
 	if (v->data_len < sizeof(*user_iph))
 		return 0;
+	if (v->data_len > 65535)
+		return -EMSGSIZE;
+
 	diff = v->data_len - e->skb->len;
 	if (diff < 0) {
 		if (pskb_trim(e->skb, v->data_len))
net/ipv6/netfilter/ip6_queue.c +3 -0
index 065fe40..4ea6a9d 100644
@@ -279,6 +279,9 @@ ipq_mangle_ipv6(ipq_verdict_msg_t *v, struct nf_queue_entry *e)
 
 	if (v->data_len < sizeof(*user_iph))
 		return 0;
+	if (v->data_len > 65535)
+		return -EMSGSIZE;
+
 	diff = v->data_len - e->skb->len;
 	if (diff < 0) {
 		if (pskb_trim(e->skb, v->data_len))
#7 lore raw

Re: [PATCH] Use unsigned variables for packet lengths in ip[6]_queue.

Quoted 16 lines · Patrick McHardy
Quoted 14 lines · David Miller

From: Dave Jones <[email protected]>
Date: Fri, 27 May 2011 20:36:51 -0400

Quoted 11 lines

So I went back to this today, and found something that doesn't look right. After adding some instrumentation, and re-running my tests, I found that the reason we were blowing up with enormous allocations was that we were passing down a nlmsglen's like -1061109568

Is there any reason for that to be signed ?
The nlmsg_len entry of nlmsghdr is a u32, so I'm assuming this is a bug.

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

Quoted 8 lines

This is fine, but I'm wondering whether this can really fix the problem you've been seeing. Before the packet is reallocated, the length of nlmsglen - NLMSGLEN(0) - sizeof(struct ipq_peer_msg) is compared to ipq_peer_msg->data_len, so both values need to be wrong. ipq_peer_msg->data_len is a size_t, so it's unsigned.

I think what we should additionally do is verify that data_len < 65535 since that's the maximum size of an IP packet.

We're actually already doing this. This makes it even more strange that you're seeing this problem. Could you send me your testcase?

author
#8 lore raw

Re: [PATCH] Use unsigned variables for packet lengths in ip[6]_queue.

Quoted 10 lines · Patrick McHardy
Quoted 10 lines
Quoted 3 lines
Quoted 3 lines

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

This is fine, but I'm wondering whether this can really fix the problem you've been seeing. Before the packet is reallocated, the length of nlmsglen - NLMSGLEN(0) - sizeof(struct ipq_peer_msg) is compared to ipq_peer_msg->data_len, so both values need to be wrong. ipq_peer_msg->data_len is a size_t, so it's unsigned.

Quoted 5 lines
Quoted 2 lines

I think what we should additionally do is verify that data_len < 65535 since that's the maximum size of an IP packet.

We're actually already doing this. This makes it even more strange that you're seeing this problem. Could you send me your testcase?

I don't have a standalone test-case, just a generic fuzzing tool that passes sockets to various syscalls. You can clone it from git://git.codemonkey.org.uk/trinity.git/
(the test-random.sh should explain how to use it)

Dave.

#9 lore raw

Re: [PATCH] Use unsigned variables for packet lengths in ip[6]_queue.

Quoted 15 lines · Dave Jones
Quoted 13 lines · Patrick McHardy
Quoted 12 lines
Quoted 3 lines
Quoted 3 lines

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

This is fine, but I'm wondering whether this can really fix the problem you've been seeing. Before the packet is reallocated, the length of nlmsglen - NLMSGLEN(0) - sizeof(struct ipq_peer_msg) is compared to ipq_peer_msg->data_len, so both values need to be wrong. ipq_peer_msg->data_len is a size_t, so it's unsigned.

I think what we should additionally do is verify that data_len < 65535 since that's the maximum size of an IP packet.

Quoted 6 lines
Quoted 2 lines

We're actually already doing this. This makes it even more strange that you're seeing this problem. Could you send me your testcase?

I don't have a standalone test-case, just a generic fuzzing tool that passes sockets to various syscalls. You can clone it from git://git.codemonkey.org.uk/trinity.git/
(the test-random.sh should explain how to use it)

Thanks, that explains the weird values. Still wondering how it managed to get passed the ipq_peer_msg length check.

Anyways, I'll give it a try myself.

maintainer Applied
#10 lore raw

Re: [PATCH] Use unsigned variables for packet lengths in ip[6]_queue.

In reply to Dave Jones
Quoted 23 lines · Dave Jones
Quoted 12 lines · David Miller

From: Dave Jones <[email protected]>
Date: Tue, 19 Apr 2011 21:42:22 -0400

Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

Quoted 4 lines

Maybe the thing to do is to simply pass __GFP_NOWARN to nlmsg_new() in netlink_ack()?

Anyone else have a better idea?

So I went back to this today, and found something that doesn't look right. After adding some instrumentation, and re-running my tests, I found that the reason we were blowing up with enormous allocations was that we were passing down a nlmsglen's like -1061109568

Quoted 6 lines

Is there any reason for that to be signed ?
The nlmsg_len entry of nlmsghdr is a u32, so I'm assuming this is a bug.

With the patch below, I haven't been able to reproduce the problem, but I don't know if I've inadvertantly broken some other behaviour somewhere deeper in netlink where this is valid.

I have applied this. Thanks.

BTW, ip[6]_queue has been marked obsoleted since long time, probably we can schedule this for removal anytime soon. The nfnetlink_queue successor has been there to provide a replacement for this since long time.

#11 lore raw
In reply to Dave Jones

Am 20.04.2011 03:42, schrieb Dave Jones:

Quoted 8 lines

Not catastrophic, but ipqueue seems to be too trusting of what it gets passed from userspace, and passes it on down to the page allocator, where it will spew warnings if the page order is too high.

__ipq_rcv_skb has several checks for lengths too small, but doesn't
seem to have any for oversized ones. I'm not sure what the maximum
we should check for is. I'll code up a diff if anyone has any ideas on a sane maximum.

A sane maximum seems to be 2^16 - 1, the maximum size of an IPv4 packet. Please also update ip6queue and nfnetlink_queue.