[nf-next] netfilter: nf_tables: fix jumpstack depth validation

Message ID 20180611120439.2744-1-ap420073@gmail.com
State Under Review
Delegated to: Pablo Neira
Headers show
Series
  • [nf-next] netfilter: nf_tables: fix jumpstack depth validation
Related show

Commit Message

Taehee Yoo June 11, 2018, 12:04 p.m.
The level of struct nft_ctx is updated by nf_tables_check_loops().
That is used to validate jumpstack depth.
But jumpstack validation routine doesn't update and validate recursively.
So, in some cases, chain depth can be bigger than the NFT_JUMP_STACK_SIZE.

After this patch, The jumpstack validation routine is located in
the nft_chain_validate().
When new rules or new set elements are added,
the nft_table_validate() is called by the nf_tables_newrule
and the nf_tables_newsetelem.
The nft_table_validate() calls the nft_chain_validate()
that visit all their children chains recursively.
So it can update depth of chain certainly.

Reproducer:
   %cat ./test.sh
   #!/bin/bash
   nft add table ip filter
   nft add chain ip filter input { type filter hook input priority 0\; }
   for ((i=0;i<20;i++)); do
	nft add chain ip filter a$i
   done

   nft add rule ip filter input jump a1

   for ((i=0;i<10;i++)); do
	nft add rule ip filter a$i jump a$((i+1))
   done

   for ((i=11;i<19;i++)); do
	nft add rule ip filter a$i jump a$((i+1))
   done

   nft add rule ip filter a10 jump a11

Result:
[  168.803743] kernel BUG at net/netfilter/nf_tables_core.c:186!
[  168.810881] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
[  168.812091] Modules linked in: nf_tables nfnetlink ip_tables x_tables
[  168.812091] CPU: 0 PID: 8 Comm: ksoftirqd/0 Not tainted 4.17.0-rc7+ #186
[  168.812091] RIP: 0010:nft_do_chain+0x9fe/0xf50 [nf_tables]
[  168.812091] RSP: 0000:ffff88011a5475b0 EFLAGS: 00010212
[  168.812091] RAX: 00000000fffffffd RBX: ffff88011a5476a0 RCX: 0000000000000000
[  168.812091] RDX: 0000000000000010 RSI: ffff880111d69bd8 RDI: ffff88011a5476b0
[  168.812091] RBP: ffff88011a547870 R08: ffffed00234a8ed6 R09: ffffed00234a8ed5
[  168.812091] R10: ffff88011a5476af R11: ffffed00234a8ed6 R12: ffff88011a5478b8
[  168.881353] R13: ffff880111d69be0 R14: ffff880111d69bc0 R15: dffffc0000000000
[  168.887792] FS:  0000000000000000(0000) GS:ffff88011b600000(0000) knlGS:0000000000000000
[  168.887792] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  168.905313] CR2: 00007ffd58ee6148 CR3: 0000000067416000 CR4: 00000000001006f0
[  168.917438] Call Trace:
[  168.917438]  ? __save_stack_trace+0x73/0xd0
[  168.922459]  ? __nft_trace_packet+0x1a0/0x1a0 [nf_tables]
[  168.922459]  ? save_stack+0x92/0xa0
[  168.922459]  ? ip_rcv+0x802/0xe10
[  168.922459]  ? sched_clock_cpu+0x144/0x180
[  168.922459]  ? sched_clock_local+0xe2/0x150
[  168.922459]  ? __lock_acquire+0xcea/0x4ed0
[  168.922459]  ? sched_clock_cpu+0x144/0x180
[  168.922459]  ? debug_check_no_locks_freed+0x280/0x280
[  168.922459]  ? nft_do_chain_ipv4+0x16f/0x1e0 [nf_tables]
[  168.922459]  nft_do_chain_ipv4+0x16f/0x1e0 [nf_tables]
[  168.922459]  ? nft_do_chain_arp+0xa0/0xa0 [nf_tables]
[  168.922459]  ? lock_acquire+0x193/0x380
[  168.922459]  ? lock_acquire+0x193/0x380
[  168.922459]  ? ip_local_deliver+0x1c6/0x3c0
[  168.922459]  nf_hook_slow+0xae/0x170
[  168.922459]  ip_local_deliver+0x293/0x3c0
[  168.922459]  ? ip_call_ra_chain+0x490/0x490
[  168.922459]  ? ip_rcv_finish+0x1910/0x1910
[  168.922459]  ip_rcv+0x802/0xe10
[ ... ]


Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 include/net/netfilter/nf_tables.h |  4 ++--
 net/netfilter/nf_tables_api.c     | 10 +++-------
 net/netfilter/nft_immediate.c     |  3 +++
 net/netfilter/nft_lookup.c        | 13 +++++++++++--
 4 files changed, 19 insertions(+), 11 deletions(-)

Comments

Florian Westphal June 11, 2018, 12:14 p.m. | #1
Taehee Yoo <ap420073@gmail.com> wrote:
> The level of struct nft_ctx is updated by nf_tables_check_loops().

[..]

> [  168.803743] kernel BUG at net/netfilter/nf_tables_core.c:186!

Could you also send a followup patch to replace this BUG_ON with
WARN_ON_ONCE+ return NF_DROP?

There is no need to crash hard here.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Taehee Yoo June 11, 2018, 12:28 p.m. | #2
Thank you for reviewing!

2018년 6월 11일 (월) 오후 9:14, Florian Westphal <fw@strlen.de>님이 작성:
>
> Taehee Yoo <ap420073@gmail.com> wrote:
> > The level of struct nft_ctx is updated by nf_tables_check_loops().
>
> [..]
>
> > [  168.803743] kernel BUG at net/netfilter/nf_tables_core.c:186!
>
> Could you also send a followup patch to replace this BUG_ON with
> WARN_ON_ONCE+ return NF_DROP?
>
> There is no need to crash hard here.

Okay, I will send a followup patch!

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 08c005c..a7d6476 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -150,6 +150,7 @@  static inline void nft_data_debug(const struct nft_data *data)
  *	@portid: netlink portID of the original message
  *	@seq: netlink sequence number
  *	@family: protocol family
+ *	@level: depth of the chains
  *	@report: notify via unicast netlink message
  */
 struct nft_ctx {
@@ -160,6 +161,7 @@  struct nft_ctx {
 	u32				portid;
 	u32				seq;
 	u8				family;
+	u8				level;
 	bool				report;
 };
 
@@ -865,7 +867,6 @@  enum nft_chain_flags {
  *	@table: table that this chain belongs to
  *	@handle: chain handle
  *	@use: number of jump references to this chain
- *	@level: length of longest path to this chain
  *	@flags: bitmask of enum nft_chain_flags
  *	@name: name of the chain
  */
@@ -878,7 +879,6 @@  struct nft_chain {
 	struct nft_table		*table;
 	u64				handle;
 	u32				use;
-	u16				level;
 	u8				flags:6,
 					genmask:2;
 	char				*name;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index ca4c4d9..b7d8c25 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2384,6 +2384,9 @@  int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain)
 	struct nft_rule *rule;
 	int err;
 
+	if (ctx->level == NFT_JUMP_STACK_SIZE)
+		return -EMLINK;
+
 	list_for_each_entry(rule, &chain->rules, list) {
 		if (!nft_is_active_next(ctx->net, rule))
 			continue;
@@ -6826,13 +6829,6 @@  int nft_validate_register_store(const struct nft_ctx *ctx,
 			err = nf_tables_check_loops(ctx, data->verdict.chain);
 			if (err < 0)
 				return err;
-
-			if (ctx->chain->level + 1 >
-			    data->verdict.chain->level) {
-				if (ctx->chain->level + 1 == NFT_JUMP_STACK_SIZE)
-					return -EMLINK;
-				data->verdict.chain->level = ctx->chain->level + 1;
-			}
 		}
 
 		return 0;
diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c
index 15adf8c..9f0542e 100644
--- a/net/netfilter/nft_immediate.c
+++ b/net/netfilter/nft_immediate.c
@@ -99,6 +99,7 @@  static int nft_immediate_validate(const struct nft_ctx *ctx,
 {
 	const struct nft_immediate_expr *priv = nft_expr_priv(expr);
 	const struct nft_data *data;
+	struct nft_ctx *pctx = (struct nft_ctx *)ctx;
 	int err;
 
 	if (priv->dreg != NFT_REG_VERDICT)
@@ -109,9 +110,11 @@  static int nft_immediate_validate(const struct nft_ctx *ctx,
 	switch (data->verdict.code) {
 	case NFT_JUMP:
 	case NFT_GOTO:
+		pctx->level++;
 		err = nft_chain_validate(ctx, data->verdict.chain);
 		if (err < 0)
 			return err;
+		pctx->level--;
 		break;
 	default:
 		break;
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 42e6fad..af13093 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -156,6 +156,8 @@  static int nft_lookup_validate_setelem(const struct nft_ctx *ctx,
 {
 	const struct nft_set_ext *ext = nft_set_elem_ext(set, elem->priv);
 	const struct nft_data *data;
+	struct nft_ctx *pctx = (struct nft_ctx *)ctx;
+	int err;
 
 	if (nft_set_ext_exists(ext, NFT_SET_EXT_FLAGS) &&
 	    *nft_set_ext_flags(ext) & NFT_SET_ELEM_INTERVAL_END)
@@ -165,10 +167,17 @@  static int nft_lookup_validate_setelem(const struct nft_ctx *ctx,
 	switch (data->verdict.code) {
 	case NFT_JUMP:
 	case NFT_GOTO:
-		return nft_chain_validate(ctx, data->verdict.chain);
+		pctx->level++;
+		err = nft_chain_validate(ctx, data->verdict.chain);
+		if (err < 0)
+			return err;
+		pctx->level--;
+		break;
 	default:
-		return 0;
+		break;
 	}
+
+	return 0;
 }
 
 static int nft_lookup_validate(const struct nft_ctx *ctx,