netfilter: nf_tables: fix netdev hook allocation memleak with dormant tables

sashiko says:
 could the related code in __nf_tables_abort() leak the struct nft_hook objects when the table is dormant?

 In __nf_tables_abort(), when rolling back a NEWCHAIN transaction that
 updates hooks, the code conditionally unregisters and frees the hooks only
 if the table is not dormant [..]
            if (!(table->flags & NFT_TABLE_F_DORMANT)) {
                nft_netdev_unregister_hooks(net,
                                            &nft_trans_chain_hooks(trans),
                                            true);
            }
            ...
            nft_trans_destroy(trans);

Unfortunately netdev family mixes hook registration and allocation.
Push table struct down and only check for the flag to unregister.

Fixes: 216e7bf7402c ("netfilter: nf_tables: skip netdev hook unregistration if table is dormant")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 38e33c66c61838f8bc17aef599e541fa5fe5d08b..87387adbca655f3501c77167a69bd2451e0260aa 100644 (file)
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -407,6 +407,7 @@ static void nft_netdev_unregister_trans_hook(struct net *net,
 }
 
 static void nft_netdev_unregister_hooks(struct net *net,
+                                       const struct nft_table *table,
                                        struct list_head *hook_list,
                                        bool release_netdev)
 {
@@ -414,8 +415,10 @@ static void nft_netdev_unregister_hooks(struct net *net,
        struct nf_hook_ops *ops;
 
        list_for_each_entry_safe(hook, next, hook_list, list) {
-               list_for_each_entry(ops, &hook->ops_list, list)
-                       nf_unregister_net_hook(net, ops);
+               if (!(table->flags & NFT_TABLE_F_DORMANT)) {
+                       list_for_each_entry(ops, &hook->ops_list, list)
+                               nf_unregister_net_hook(net, ops);
+               }
                if (release_netdev)
                        nft_netdev_hook_unlink_free_rcu(hook);
        }
@@ -452,20 +455,25 @@ static void __nf_tables_unregister_hook(struct net *net,
        struct nft_base_chain *basechain;
        const struct nf_hook_ops *ops;
 
-       if (table->flags & NFT_TABLE_F_DORMANT ||
-           !nft_is_base_chain(chain))
+       if (!nft_is_base_chain(chain))
                return;
        basechain = nft_base_chain(chain);
        ops = &basechain->ops;
 
+       /* must also be called for dormant tables */
+       if (nft_base_chain_netdev(table->family, basechain->ops.hooknum)) {
+               nft_netdev_unregister_hooks(net, table, &basechain->hook_list,
+                                           release_netdev);
+               return;
+       }
+
+       if (table->flags & NFT_TABLE_F_DORMANT)
+               return;
+
        if (basechain->type->ops_unregister)
                return basechain->type->ops_unregister(net, ops);
 
-       if (nft_base_chain_netdev(table->family, basechain->ops.hooknum))
-               nft_netdev_unregister_hooks(net, &basechain->hook_list,
-                                           release_netdev);
-       else
-               nf_unregister_net_hook(net, &basechain->ops);
+       nf_unregister_net_hook(net, &basechain->ops);
 }
 
 static void nf_tables_unregister_hook(struct net *net,
@@ -11282,11 +11290,9 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
                        break;
                case NFT_MSG_NEWCHAIN:
                        if (nft_trans_chain_update(trans)) {
-                               if (!(table->flags & NFT_TABLE_F_DORMANT)) {
-                                       nft_netdev_unregister_hooks(net,
-                                                                   &nft_trans_chain_hooks(trans),
-                                                                   true);
-                               }
+                               nft_netdev_unregister_hooks(net, table,
+                                                           &nft_trans_chain_hooks(trans),
+                                                           true);
                                free_percpu(nft_trans_chain_stats(trans));
                                kfree(nft_trans_chain_name(trans));
                                nft_trans_destroy(trans);