Message ID | 4DEE209C.2010104@trash.net |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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))