netfilter: nf_tables: missing validation from the abort path

BugLink: https://bugs.launchpad.net/bugs/1905618
[ Upstream commit c0391b6ab810381df632677a1dcbbbbd63d05b6d ]

If userspace does not include the trailing end of batch message, then
nfnetlink aborts the transaction. This allows to check that ruleset
updates trigger no errors.

After this patch, invoking this command from the prerouting chain:

 # nft -c add rule x y fib saddr . oif type local

fails since oif is not supported there.

This patch fixes the lack of rule validation from the abort/check path
to catch configuration errors such as the one above.

Fixes: a654de8fdc18 ("netfilter: nf_tables: fix chain dependency validation")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index 89016d08f6a278d9a7703cc4256c277f05fa7173..f6267e2883f261e0bd533a349759fad1b7e8d656 100644 (file)
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -24,6 +24,12 @@ struct nfnl_callback {
        const u_int16_t attr_count;             /* number of nlattr's */
 };
 
+enum nfnl_abort_action {
+       NFNL_ABORT_NONE         = 0,
+       NFNL_ABORT_AUTOLOAD,
+       NFNL_ABORT_VALIDATE,
+};
+
 struct nfnetlink_subsystem {
        const char *name;
        __u8 subsys_id;                 /* nfnetlink subsystem ID */
@@ -31,7 +37,8 @@ struct nfnetlink_subsystem {
        const struct nfnl_callback *cb; /* callback for individual types */
        struct module *owner;
        int (*commit)(struct net *net, struct sk_buff *skb);
-       int (*abort)(struct net *net, struct sk_buff *skb, bool autoload);
+       int (*abort)(struct net *net, struct sk_buff *skb,
+                    enum nfnl_abort_action action);
        void (*cleanup)(struct net *net);
        bool (*valid_genid)(struct net *net, u32 genid);
 };

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 5a77b7a177229c953553ba3d8a16d3e0edde5b01..51391d5d226567d42e419e73ad93947fa895c667 100644 (file)
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7010,11 +7010,15 @@ static void nf_tables_abort_release(struct nft_trans *trans)
        kfree(trans);
 }
 
-static int __nf_tables_abort(struct net *net, bool autoload)
+static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 {
        struct nft_trans *trans, *next;
        struct nft_trans_elem *te;
 
+       if (action == NFNL_ABORT_VALIDATE &&
+           nf_tables_validate(net) < 0)
+               return -EAGAIN;
+
        list_for_each_entry_safe_reverse(trans, next, &net->nft.commit_list,
                                         list) {
                switch (trans->msg_type) {
@@ -7132,7 +7136,7 @@ static int __nf_tables_abort(struct net *net, bool autoload)
                nf_tables_abort_release(trans);
        }
 
-       if (autoload)
+       if (action == NFNL_ABORT_AUTOLOAD)
                nf_tables_module_autoload(net);
        else
                nf_tables_module_autoload_cleanup(net);
@@ -7145,9 +7149,10 @@ static void nf_tables_cleanup(struct net *net)
        nft_validate_state_update(net, NFT_VALIDATE_SKIP);
 }
 
-static int nf_tables_abort(struct net *net, struct sk_buff *skb, bool autoload)
+static int nf_tables_abort(struct net *net, struct sk_buff *skb,
+                          enum nfnl_abort_action action)
 {
-       int ret = __nf_tables_abort(net, autoload);
+       int ret = __nf_tables_abort(net, action);
 
        mutex_unlock(&net->nft.commit_mutex);
 
@@ -7754,7 +7759,7 @@ static void __net_exit nf_tables_exit_net(struct net *net)
 {
        mutex_lock(&net->nft.commit_mutex);
        if (!list_empty(&net->nft.commit_list))
-               __nf_tables_abort(net, false);
+               __nf_tables_abort(net, NFNL_ABORT_NONE);
        __nft_release_tables(net);
        mutex_unlock(&net->nft.commit_mutex);
        WARN_ON_ONCE(!list_empty(&net->nft.tables));

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 6d03b0909621097d8ace39bddf99a191e6a493e9..81c86a156c6c0ab5488212cdc70264615649e4f4 100644 (file)
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -315,7 +315,7 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
                return netlink_ack(skb, nlh, -EINVAL, NULL);
 replay:
        status = 0;
-
+replay_abort:
        skb = netlink_skb_clone(oskb, GFP_KERNEL);
        if (!skb)
                return netlink_ack(oskb, nlh, -ENOMEM, NULL);
@@ -481,7 +481,7 @@ ack:
        }
 done:
        if (status & NFNL_BATCH_REPLAY) {
-               ss->abort(net, oskb, true);
+               ss->abort(net, oskb, NFNL_ABORT_AUTOLOAD);
                nfnl_err_reset(&err_list);
                kfree_skb(skb);
                module_put(ss->owner);
@@ -492,11 +492,25 @@ done:
                        status |= NFNL_BATCH_REPLAY;
                        goto done;
                } else if (err) {
-                       ss->abort(net, oskb, false);
+                       ss->abort(net, oskb, NFNL_ABORT_NONE);
                        netlink_ack(oskb, nlmsg_hdr(oskb), err, NULL);
                }
        } else {
-               ss->abort(net, oskb, false);
+               enum nfnl_abort_action abort_action;
+
+               if (status & NFNL_BATCH_FAILURE)
+                       abort_action = NFNL_ABORT_NONE;
+               else
+                       abort_action = NFNL_ABORT_VALIDATE;
+
+               err = ss->abort(net, oskb, abort_action);
+               if (err == -EAGAIN) {
+                       nfnl_err_reset(&err_list);
+                       kfree_skb(skb);
+                       module_put(ss->owner);
+                       status |= NFNL_BATCH_FAILURE;
+                       goto replay_abort;
+               }
        }
        if (ss->cleanup)
                ss->cleanup(net);