Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> So the problem is only the use-after-free from the NEWSETELEM abort
> path, right?
That and the double-freeing of the set.
> Probably we can fix this problem with this patch too? Idea is to keep
> this 'bound' internal flag, in that case, this turns the NEWSET and
> NEWSETELEM abort path into noop.
Good idea, but your patch still causes UAF.
> @@ -6617,10 +6617,13 @@ static void nf_tables_abort_release(struct nft_trans *trans)
> nf_tables_rule_destroy(&trans->ctx, nft_trans_rule(trans));
> break;
> case NFT_MSG_NEWSET:
> - if (!nft_trans_set_bound(trans))
> - nft_set_destroy(nft_trans_set(trans));
> + if (nft_trans_set(trans)->bound)
> + break;
> + nft_set_destroy(nft_trans_set(trans));
Its not safe to use nft_trans_set() here anymore, as the set
has already been released by nf_tables_rule_destroy().
> case NFT_MSG_NEWSETELEM:
> + if (nft_trans_elem_set(trans)->bound)
> + break;
Same here.
Either the transactions themselves need to be yanked here, before
nf_tables_abort_release() is run, or the 'bound' flag needs to be
stored in the transaction struct.
I'd guess nft_trans_delete() is enough instead of plain break,
in case the set is bound we know expr destructor removes the set,
and if we avoid removal of the elements then those are removed too
when the set is destroyed.
This seems to make things work for me, on top of your patch:
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6786,8 +6786,10 @@ static int __nf_tables_abort(struct net *net)
break;
case NFT_MSG_NEWSET:
trans->ctx.table->use--;
- if (nft_trans_set(trans)->bound)
+ if (nft_trans_set(trans)->bound) {
+ nft_trans_destroy(trans);
break;
+ }
list_del_rcu(&nft_trans_set(trans)->list);
break;
case NFT_MSG_DELSET:
@@ -6796,8 +6798,10 @@ static int __nf_tables_abort(struct net *net)
nft_trans_destroy(trans);
break;
case NFT_MSG_NEWSETELEM:
- if (nft_trans_elem_set(trans)->bound)
+ if (nft_trans_elem_set(trans)->bound) {
+ nft_trans_destroy(trans);
break;
+ }
te = (struct nft_trans_elem *)trans->data;
te->set->ops->remove(net, te->set, &te->elem);
atomic_dec(&te->set->nelems);
