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

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

[David Miller]

From: Dave Jones <davej@redhat.com>
Date: Fri, 27 May 2011 20:36:51 -0400

> On Tue, Apr 19, 2011 at 08:41:05PM -0700, David Miller wrote:
>  > From: Dave Jones <davej@redhat.com>
>  > Date: Tue, 19 Apr 2011 21:42:22 -0400
>  > 
>  > > 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?
> 
> 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.

netfilter-devel and maintainers CC:'d

> -- 
> 
> Netlink message lengths can't be negative, so use unsigned variables.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> diff --git a/net/ipv4/netfilter/ip_queue.c b/net/ipv4/netfilter/ip_queue.c
> index d2c1311..f7f9bd7 100644
> --- a/net/ipv4/netfilter/ip_queue.c
> +++ b/net/ipv4/netfilter/ip_queue.c
> @@ -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;
> diff --git a/net/ipv6/netfilter/ip6_queue.c b/net/ipv6/netfilter/ip6_queue.c
> index 413ab07..065fe40 100644
> --- a/net/ipv6/netfilter/ip6_queue.c
> +++ b/net/ipv6/netfilter/ip6_queue.c
> @@ -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;

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

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

On 02.06.2011 22:57, David Miller wrote:
> From: Dave Jones <davej@redhat.com>
> Date: Fri, 27 May 2011 20:36:51 -0400
> 
>> On Tue, Apr 19, 2011 at 08:41:05PM -0700, David Miller wrote:
>>  > From: Dave Jones <davej@redhat.com>
>>  > Date: Tue, 19 Apr 2011 21:42:22 -0400
>>  > 
>>  > > 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?
>>
>> 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.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 920 bytes --]

diff --git a/net/ipv4/netfilter/ip_queue.c b/net/ipv4/netfilter/ip_queue.c
index f7f9bd7..8ded42d 100644
--- a/net/ipv4/netfilter/ip_queue.c
+++ b/net/ipv4/netfilter/ip_queue.c
@@ -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))
diff --git a/net/ipv6/netfilter/ip6_queue.c b/net/ipv6/netfilter/ip6_queue.c
index 065fe40..4ea6a9d 100644
--- a/net/ipv6/netfilter/ip6_queue.c
+++ b/net/ipv6/netfilter/ip6_queue.c
@@ -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))

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

[Patrick McHardy]

On 07.06.2011 14:59, Patrick McHardy wrote:
> On 02.06.2011 22:57, David Miller wrote:
>> From: Dave Jones <davej@redhat.com>
>> Date: Fri, 27 May 2011 20:36:51 -0400
>>
>>> 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.

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

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

[Dave Jones]

On Tue, Jun 07, 2011 at 04:19:08PM +0200, Patrick McHardy wrote:
 
 > >>> 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.
 > 
 > 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.

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

[Patrick McHardy]

On 07.06.2011 16:39, Dave Jones wrote:
> On Tue, Jun 07, 2011 at 04:19:08PM +0200, Patrick McHardy wrote:
>  
>  > >>> 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.
>  > 
>  > 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.

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

[Pablo Neira Ayuso]

On 28/05/11 02:36, Dave Jones wrote:
> On Tue, Apr 19, 2011 at 08:41:05PM -0700, David Miller wrote:
>  > From: Dave Jones <davej@redhat.com>
>  > Date: Tue, 19 Apr 2011 21:42:22 -0400
>  > 
>  > > 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?
> 
> 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.

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.