netdev - Re: [PATCH net 02/14] netfilter: nf_tables: fix chain binding transaction logic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZJCzcufoMlCGE64U@corigine.com>
Date: Mon, 19 Jun 2023 21:58:42 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Pablo Neira Ayuso <pablo@...filter.org>
Cc: netfilter-devel@...r.kernel.org, davem@...emloft.net,
	netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com
Subject: Re: [PATCH net 02/14] netfilter: nf_tables: fix chain binding
 transaction logic

On Mon, Jun 19, 2023 at 04:57:53PM +0200, Pablo Neira Ayuso wrote:
> Add bound flag to rule and chain transactions as in 6a0a8d10a366
> ("netfilter: nf_tables: use-after-free in failing rule with bound set")
> to skip them in case that the chain is already bound from the abort
> path.
> 
> This patch fixes an imbalance in the chain use refcnt that triggers a
> WARN_ON on the table and chain destroy path.
> 
> This patch also disallows nested chain bindings, which is not
> supported from userspace.
> 
> The logic to deal with chain binding in nft_data_hold() and
> nft_data_release() is not correct. The NFT_TRANS_PREPARE state needs a
> special handling in case a chain is bound but next expressions in the
> same rule fail to initialize as described by 1240eb93f061 ("netfilter:
> nf_tables: incorrect error path handling with NFT_MSG_NEWRULE").
> 
> Fixes: d0e2c7de92c7 ("netfilter: nf_tables: add NFT_CHAIN_BINDING")
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>

...

> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 69bceefaa5c8..7b3a0bf254e3 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -193,6 +193,48 @@ static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set)
>  	}
>  }
>  
> +static void nft_chain_trans_bind(const struct nft_ctx *ctx, struct nft_chain *chain)
> +{
> +	struct nftables_pernet *nft_net;
> +	struct net *net = ctx->net;
> +	struct nft_trans *trans;
> +
> +	if (!nft_chain_binding(chain))
> +		return;
> +
> +	nft_net = nft_pernet(net);
> +	list_for_each_entry_reverse(trans, &nft_net->commit_list, list) {
> +		switch (trans->msg_type) {
> +		case NFT_MSG_NEWCHAIN:
> +			if (nft_trans_chain(trans) == chain)
> +				nft_trans_chain_bound(trans) = true;
> +			break;
> +		case NFT_MSG_NEWRULE:
> +			if (trans->ctx.chain == chain)
> +				nft_trans_rule_bound(trans) = bind;

Hi Pablo,

Maybe something got mixed up here somehow.
It seems that on x86_64 allmodconfig bind is not defined here.

gcc says:

 net/netfilter/nf_tables_api.c: In function 'nft_chain_trans_bind':
 net/netfilter/nf_tables_api.c:214:63: error: 'bind' undeclared (first use in this function)
   214 |                                 nft_trans_rule_bound(trans) = bind;
       |                                                               ^~~~
 net/netfilter/nf_tables_api.c:214:63: note: each undeclared identifier is reported only once for each function it appears in

> +			break;
> +		}
> +	}
> +}

-- 
pw-bot: cr