All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH nf-next] netfilter: fix IS_ERR_VALUE usage
@ 2016-04-29  8:53 Pablo Neira Ayuso
  2016-04-29  9:17 ` Florian Westphal
  0 siblings, 1 reply; 3+ messages in thread
From: Pablo Neira Ayuso @ 2016-04-29  8:53 UTC (permalink / raw)
  To: netfilter-devel; +Cc: a.hajda

This is a forward-ported original patch from Andrzej Hajda, he said:

"IS_ERR_VALUE should be used only with unsigned long type.
Otherwise it can work incorrectly. To achieve this function
xt_percpu_counter_alloc is modified to return unsigned long,
and its result is assigned to temporary variable to perform
error checking, before assigning to .pcnt field.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581"

Original patch from Andrzej is here:

http://patchwork.ozlabs.org/patch/582970/

This patch has clashed with input validation fixes for x_tables.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/x_tables.h | 6 +++---
 net/ipv4/netfilter/arp_tables.c    | 6 ++++--
 net/ipv4/netfilter/ip_tables.c     | 6 ++++--
 net/ipv6/netfilter/ip6_tables.c    | 6 ++++--
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index 4dd9306..dc4f58a 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -380,16 +380,16 @@ static inline unsigned long ifname_compare_aligned(const char *_a,
  * allows us to return 0 for single core systems without forcing
  * callers to deal with SMP vs. NONSMP issues.
  */
-static inline u64 xt_percpu_counter_alloc(void)
+static inline unsigned long xt_percpu_counter_alloc(void)
 {
 	if (nr_cpu_ids > 1) {
 		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
 						    sizeof(struct xt_counters));
 
 		if (res == NULL)
-			return (u64) -ENOMEM;
+			return -ENOMEM;
 
-		return (u64) (__force unsigned long) res;
+		return (__force unsigned long) res;
 	}
 
 	return 0;
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 60f5161..3355ed7 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -513,11 +513,13 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 {
 	struct xt_entry_target *t;
 	struct xt_target *target;
+	unsigned long pcnt;
 	int ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	t = arpt_get_target(e);
 	target = xt_request_find_target(NFPROTO_ARP, t->u.user.name,
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 735d1ee..21ccc19 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -656,10 +656,12 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 	unsigned int j;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 73e606c..17874e8 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -669,10 +669,12 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 	unsigned int j;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH nf-next] netfilter: fix IS_ERR_VALUE usage
  2016-04-29  8:53 [PATCH nf-next] netfilter: fix IS_ERR_VALUE usage Pablo Neira Ayuso
@ 2016-04-29  9:17 ` Florian Westphal
  2016-04-29  9:30   ` Pablo Neira Ayuso
  0 siblings, 1 reply; 3+ messages in thread
From: Florian Westphal @ 2016-04-29  9:17 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, a.hajda

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> This is a forward-ported original patch from Andrzej Hajda, he said:
> 
> "IS_ERR_VALUE should be used only with unsigned long type.
> Otherwise it can work incorrectly. To achieve this function
> xt_percpu_counter_alloc is modified to return unsigned long,
> and its result is assigned to temporary variable to perform
> error checking, before assigning to .pcnt field.
> 
> The patch follows conclusion from discussion on LKML [1][2].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
> [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581"
> 
> Original patch from Andrzej is here:
> 
> http://patchwork.ozlabs.org/patch/582970/
> 
> This patch has clashed with input validation fixes for x_tables.

AFAICS this isn't a bug fix so I'm fine with this going into nf-next.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH nf-next] netfilter: fix IS_ERR_VALUE usage
  2016-04-29  9:17 ` Florian Westphal
@ 2016-04-29  9:30   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 3+ messages in thread
From: Pablo Neira Ayuso @ 2016-04-29  9:30 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel, a.hajda

On Fri, Apr 29, 2016 at 11:17:54AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > This is a forward-ported original patch from Andrzej Hajda, he said:
> > 
> > "IS_ERR_VALUE should be used only with unsigned long type.
> > Otherwise it can work incorrectly. To achieve this function
> > xt_percpu_counter_alloc is modified to return unsigned long,
> > and its result is assigned to temporary variable to perform
> > error checking, before assigning to .pcnt field.
> > 
> > The patch follows conclusion from discussion on LKML [1][2].
> > 
> > [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
> > [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581"
> > 
> > Original patch from Andrzej is here:
> > 
> > http://patchwork.ozlabs.org/patch/582970/
> > 
> > This patch has clashed with input validation fixes for x_tables.
> 
> AFAICS this isn't a bug fix so I'm fine with this going into nf-next.

This seems to affect 32-bits arch, we could qualify this as nf. But
this has been there for a while without nobody noticing in runtime
kernels, this problem got uncovered by analysis I think.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-04-29  9:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-29  8:53 [PATCH nf-next] netfilter: fix IS_ERR_VALUE usage Pablo Neira Ayuso
2016-04-29  9:17 ` Florian Westphal
2016-04-29  9:30   ` Pablo Neira Ayuso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.