Skip to contentSkip to conversation

netfilter: return -EFAULT directly when counters_ptr is NULL

PATCH In review Duan Jiong 2 participants 3 messages Dec 3, 2014 – Dec 11, 2014

Subsystems (3)
Networking netfilter Ethernet bridge
lore
Share & tools
lore

Duan Jiong

author
#0 lore raw

PATCH 1/1 netfilter: return -EFAULT directly when counters_ptr is NULL

Now if the copy_to_user() fail, __do_replace just silent error, because new table is alread replaced. But at the beginning of __do_replace(), if we notice that user supply no memory to store counters, we should not replace table, so we can just returen -EFAULT directly.

Signed-off-by Duan Jiong <[email protected]>
---
 net/bridge/netfilter/ebtables.c | 3 +++
 net/ipv4/netfilter/arp_tables.c | 4 ++++
 net/ipv4/netfilter/ip_tables.c  | 4 ++++
 net/ipv6/netfilter/ip6_tables.c | 4 ++++
 4 files changed, 15 insertions(+)
net/bridge/netfilter/ebtables.c +3 -0
index d9a8c05..90ccc78 100644
@@ -983,6 +983,9 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 	struct ebt_table_info *table;
 	struct ebt_table *t;
 
+	if (!repl->counters)
+		return -EFAULT;
+
 	/* the user wants counters back
 	   the check on the size is done later, when we have the lock */
 	if (repl->num_counters) {
net/ipv4/netfilter/arp_tables.c +4 -0
index f95b6f9..654debe 100644
@@ -999,6 +999,10 @@ static int __do_replace(struct net *net, const char *name,
 	struct arpt_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
net/ipv4/netfilter/ip_tables.c +4 -0
index 99e810f..0acdcc8 100644
@@ -1186,6 +1186,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	struct ipt_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
net/ipv6/netfilter/ip6_tables.c +4 -0
index e080fbb..cb91f2b 100644
@@ -1196,6 +1196,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	struct ip6t_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
Signature · 1 line
1.8.3.1
maintainer
#1 lore raw
Quoted 5 lines · Duan Jiong

Now if the copy_to_user() fail, __do_replace just silent error, because new table is alread replaced. But at the beginning of __do_replace(), if we notice that user supply no memory to store counters, we should not replace table, so we can just returen -EFAULT directly.

This is independent of the change you mention, right?

I mean, userspace can send us null counters and we'll crash.

Another nitpick comment below:

Quoted 27 lines
Signed-off-by Duan Jiong <[email protected]>
---
 net/bridge/netfilter/ebtables.c | 3 +++
 net/ipv4/netfilter/arp_tables.c | 4 ++++
 net/ipv4/netfilter/ip_tables.c  | 4 ++++
 net/ipv6/netfilter/ip6_tables.c | 4 ++++
 4 files changed, 15 insertions(+)
net/bridge/netfilter/ebtables.c +3 -0
index d9a8c05..90ccc78 100644
@@ -983,6 +983,9 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 	struct ebt_table_info *table;
 	struct ebt_table *t;
 
+	if (!repl->counters)
+		return -EFAULT;
+
 	/* the user wants counters back
 	   the check on the size is done later, when we have the lock */
 	if (repl->num_counters) {
net/ipv4/netfilter/arp_tables.c +0 -0
index f95b6f9..654debe 100644

net/ipv4/netfilter/arp_tables.c static int __do_replace(struct net *net, const char *name, · line 1005

Quoted 7 lines
net/ipv4/netfilter/arp_tables.c +4 -0
@@ -999,6 +999,10 @@ static int __do_replace(struct net *net, const char *name,
 	struct arpt_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}

Please move this check before 'ret = 0'. So we avoid the unnecessary initialization. Same thing below.

Quoted 35 lines
net/ipv4/netfilter/arp_tables.c +0 -0
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
net/ipv4/netfilter/ip_tables.c +4 -0
index 99e810f..0acdcc8 100644
@@ -1186,6 +1186,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	struct ipt_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
net/ipv6/netfilter/ip6_tables.c +4 -0
index e080fbb..cb91f2b 100644
@@ -1196,6 +1196,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	struct ip6t_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
Signature · 1 line
1.8.3.1
author
#2 lore raw
Quoted 10 lines · Pablo Neira Ayuso
Quoted 5 lines · Duan Jiong

Now if the copy_to_user() fail, __do_replace just silent error, because new table is alread replaced. But at the beginning of __do_replace(), if we notice that user supply no memory to store counters, we should not replace table, so we can just returen -EFAULT directly.

This is independent of the change you mention, right?

I mean, userspace can send us null counters and we'll crash.

I have tested it, and it will not cause crash.
The goal here is to avoid mallocing memory to store counter and replacing table when userspace send us null counters.

Thanks,
Duan

Quoted 78 lines

Another nitpick comment below:

Quoted 27 lines
Signed-off-by Duan Jiong <[email protected]>
---
 net/bridge/netfilter/ebtables.c | 3 +++
 net/ipv4/netfilter/arp_tables.c | 4 ++++
 net/ipv4/netfilter/ip_tables.c  | 4 ++++
 net/ipv6/netfilter/ip6_tables.c | 4 ++++
 4 files changed, 15 insertions(+)
net/bridge/netfilter/ebtables.c +3 -0
index d9a8c05..90ccc78 100644
@@ -983,6 +983,9 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 	struct ebt_table_info *table;
 	struct ebt_table *t;
 
+	if (!repl->counters)
+		return -EFAULT;
+
 	/* the user wants counters back
 	   the check on the size is done later, when we have the lock */
 	if (repl->num_counters) {
net/ipv4/netfilter/arp_tables.c +0 -0
index f95b6f9..654debe 100644

net/ipv4/netfilter/arp_tables.c static int __do_replace(struct net *net, const char *name, · line 1005

Quoted 7 lines
net/ipv4/netfilter/arp_tables.c +4 -0
@@ -999,6 +999,10 @@ static int __do_replace(struct net *net, const char *name,
 	struct arpt_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}

Please move this check before 'ret = 0'. So we avoid the unnecessary initialization. Same thing below.

Quoted 35 lines
net/ipv4/netfilter/arp_tables.c +0 -0
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
net/ipv4/netfilter/ip_tables.c +4 -0
index 99e810f..0acdcc8 100644
@@ -1186,6 +1186,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	struct ipt_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
net/ipv6/netfilter/ip6_tables.c +4 -0
index e080fbb..cb91f2b 100644
@@ -1196,6 +1196,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	struct ip6t_entry *iter;
 
 	ret = 0;
+	if (!counters_ptr) {
+		ret = -EFAULT;
+		goto out;
+	}
 	counters = vzalloc(num_counters * sizeof(struct xt_counters));
 	if (!counters) {
 		ret = -ENOMEM;
Signature · 1 line
1.8.3.1

.