[PATCH nf] netfilter: nf_tables: disallow non-stateful expression in sets earlier

[PATCH nf] netfilter: nf_tables: disallow non-stateful expression in sets earlier

[Pablo Neira Ayuso]

Since 3e135cd499bf ("netfilter: nft_dynset: dynamic stateful expression
instantiation"), it is possible to attach stateful expressions to set
elements.

cd5125d8f518 ("netfilter: nf_tables: split set destruction in deactivate
and destroy phase") introduces conditional destruction on the object to
accomodate transaction semantics.

nft_expr_init() calls expr->ops->init() first, then check for
NFT_STATEFUL_EXPR, this stills allows to initialize a non-stateful
lookup expressions which points to a set, which might lead to UAF since
the set is not properly detached from the set->binding for this case.
Anyway, this combination is non-sense from nf_tables perspective.

This patch fixes this problem by checking for NFT_STATEFUL_EXPR before
expr->ops->init() is called.

The reporter provides a KASAN splat and a poc reproducer (similar to
those autogenerated by syzbot to report use-after-free errors). It is
unknown to me if they are using syzbot or if used similar automated tool
to locate the bug that they are reporting.

For the record, this is the KASAN splat.

[   85.431824] ==================================================================
[   85.432901] BUG: KASAN: use-after-free in nf_tables_bind_set+0x81b/0xa20
[   85.433825] Write of size 8 at addr ffff8880286f0e98 by task poc/776
[   85.434756]
[   85.434999] CPU: 1 PID: 776 Comm: poc Tainted: G        W         5.18.0+ #2
[   85.436023] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014

Fixes: 0b2d8a7b638b ("netfilter: nf_tables: add helper functions for expression handling")
Reported-and-tested-by: edg-e <edg-e@nccgroup.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a096b9fbbbdf..3c7fab36133b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2873,27 +2873,31 @@ static struct nft_expr *nft_expr_init(const struct nft_ctx *ctx,
 
 	err = nf_tables_expr_parse(ctx, nla, &expr_info);
 	if (err < 0)
-		goto err1;
+		goto err_expr_parse;
+
+	err = -EOPNOTSUPP;
+	if (!(expr_info.ops->type->flags & NFT_EXPR_STATEFUL))
+		goto err_expr_stateful;
 
 	err = -ENOMEM;
 	expr = kzalloc(expr_info.ops->size, GFP_KERNEL_ACCOUNT);
 	if (expr == NULL)
-		goto err2;
+		goto err_expr_stateful;
 
 	err = nf_tables_newexpr(ctx, &expr_info, expr);
 	if (err < 0)
-		goto err3;
+		goto err_expr_new;
 
 	return expr;
-err3:
+err_expr_new:
 	kfree(expr);
-err2:
+err_expr_stateful:
 	owner = expr_info.ops->type->owner;
 	if (expr_info.ops->type->release_ops)
 		expr_info.ops->type->release_ops(expr_info.ops);
 
 	module_put(owner);
-err1:
+err_expr_parse:
 	return ERR_PTR(err);
 }
 
@@ -5412,10 +5416,6 @@ struct nft_expr *nft_set_elem_expr_alloc(const struct nft_ctx *ctx,
 	if (IS_ERR(expr))
 		return expr;
 
-	err = -EOPNOTSUPP;
-	if (!(expr->ops->type->flags & NFT_EXPR_STATEFUL))
-		goto err_set_elem_expr;
-
 	if (expr->ops->type->flags & NFT_EXPR_GC) {
 		if (set->flags & NFT_SET_TIMEOUT)
 			goto err_set_elem_expr;
-- 
2.30.2

Re: [PATCH nf] netfilter: nf_tables: disallow non-stateful expression in sets earlier

[kernel test robot]

Hi Pablo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on nf/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Pablo-Neira-Ayuso/netfilter-nf_tables-disallow-non-stateful-expression-in-sets-earlier/20220526-190122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220526/202205262104.cowfvBMc-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6f4644d194da594562027a5d458d9fb7a20ebc39)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/08b6b97fd372a14e82e821bb2b4c9c10c1426080
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Pablo-Neira-Ayuso/netfilter-nf_tables-disallow-non-stateful-expression-in-sets-earlier/20220526-190122
        git checkout 08b6b97fd372a14e82e821bb2b4c9c10c1426080
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/netfilter/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_tables_api.c:5431:17: warning: variable 'err' is uninitialized when used here [-Wuninitialized]
           return ERR_PTR(err);
                          ^~~
   net/netfilter/nf_tables_api.c:5413:9: note: initialize the variable 'err' to silence this warning
           int err;
                  ^
                   = 0
   1 warning generated.


vim +/err +5431 net/netfilter/nf_tables_api.c

60319eb1ca351a Pablo Neira Ayuso 2014-04-04  5407  
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5408  struct nft_expr *nft_set_elem_expr_alloc(const struct nft_ctx *ctx,
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5409  					 const struct nft_set *set,
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5410  					 const struct nlattr *attr)
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5411  {
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5412  	struct nft_expr *expr;
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5413  	int err;
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5414  
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5415  	expr = nft_expr_init(ctx, attr);
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5416  	if (IS_ERR(expr))
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5417  		return expr;
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5418  
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5419  	if (expr->ops->type->flags & NFT_EXPR_GC) {
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5420  		if (set->flags & NFT_SET_TIMEOUT)
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5421  			goto err_set_elem_expr;
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5422  		if (!set->ops->gc_init)
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5423  			goto err_set_elem_expr;
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5424  		set->ops->gc_init(set);
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5425  	}
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5426  
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5427  	return expr;
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5428  
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5429  err_set_elem_expr:
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5430  	nft_expr_destroy(ctx, expr);
a7fc9368040841 Pablo Neira Ayuso 2020-03-11 @5431  	return ERR_PTR(err);
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5432  }
a7fc9368040841 Pablo Neira Ayuso 2020-03-11  5433  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp