Backport netfilter: nf_tables: autoload modules from the abort path
cover Backport netfilter: nf_tables: autoload modules from the abort path
The following series backports netfilter: nf_tables: autoload modules from abort path which fixes the bug mentioned in the following:
https://syzkaller.appspot.com/bug?extid=437bf61d165c87bd40fb
----
BUG: corrupted list in __nf_tables_abort
Status: fixed on 2020/03/17 22:09
Fix commit: eb014de4fd41 netfilter: nf_tables: autoload modules from the abort path First crash: 717d, last: 710d
Cause bisection: introduced by (bisect log) :
commit ec7470b834fe7b5d7eff11b6677f5d7fdf5e9a91
Author: Pablo Neira Ayuso <[email protected]>
Date: Mon Jan 13 17:09:58 2020 +0000
netfilter: nf_tables: store transaction list locally while requesting module
Crash: KASAN: use-after-free Read in __nf_tables_abort (log)
Repro: C syz .config
Fix bisection: fixed by (bisect log) :
commit 34682110abc50ffea7e002b0c2fd7ea9e0000ccc
Author: Max Chou <[email protected]>
Date: Wed Nov 27 03:01:07 2019 +0000
Bluetooth: btusb: Edit the logical value for Realtek Bluetooth reset
'
From 185a61df95c18e5111df5ba439b0e60a8a6e40cb Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <[email protected]>
Date: Fri, 5 Jul 2019 23:38:46 +0200
Subject: [PATCH 2/5] netfilter: nf_tables: add nft_expr_type_request_module()
This helper function makes sure the family specific extension is loaded.
--- net/netfilter/nf_tables_api.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
net/netfilter/nf_tables_api.c +14 -3
@@ -2009,6 +2009,19 @@ static const struct nft_expr_type *__nft_expr_type_get(u8 family,return NULL ; } +#ifdef CONFIG_MODULES +static int nft_expr_type_request_module (struct net *net,u8 family, +struct nlattr *nla) +{ +nft_request_module (net,"nft-expr-%u-%.*s" , family, +nla_len (nla), (char *)nla_data (nla)); +if (__nft_expr_type_get (family, nla)) +return -EAGAIN ; + +return 0 ; +} +#endif +static const struct nft_expr_type *nft_expr_type_get (struct net *net,u8 family,struct nlattr *nla)
@@ -2025,9 +2038,7 @@ static const struct nft_expr_type *nft_expr_type_get(struct net *net,lockdep_nfnl_nft_mutex_not_held ();#ifdef CONFIG_MODULES if (type ==NULL ) { -nft_request_module (net,"nft-expr-%u-%.*s" , family, -nla_len (nla), (char *)nla_data (nla)); -if (__nft_expr_type_get (family, nla)) +if (nft_expr_type_request_module (net, family, nla) == -EAGAIN )return ERR_PTR (-EAGAIN ); nft_request_module(net, "nft-expr-%.*s",
Signature · 1 line
2.34.1
From 791580bd2a8b75daddc0d110582198ab0ac854b2 Mon Sep 17 00:00:00 2001
From: Florian Westphal <[email protected]>
Date: Thu, 5 Mar 2020 11:15:36 +0100
Subject: [PATCH 4/5] netfilter: nf_tables: fix infinite loop when expr is not
available
nft will loop forever if the kernel doesn't support an expression:
1. nft_expr_type_get() appends the family specific name to the module list.
2. -EAGAIN is returned to nfnetlink, nfnetlink calls abort path.
3. abort path sets ->done to true and calls request_module for the expression.
4. nfnetlink replays the batch, we end up in nft_expr_type_get() again.
5. nft_expr_type_get attempts to append family-specific name. This one already exists on the list, so we continue
6. nft_expr_type_get adds the generic expression name to the module list. -EAGAIN is returned, nfnetlink calls abort path.
7. abort path encounters the family-specific expression which has 'done' set, so it gets removed.
8. abort path requests the generic expression name, sets done to true.
9. batch is replayed.
If the expression could not be loaded, then we will end up back at 1), because the family-specific name got removed and the cycle starts again.
Note that userspace can SIGKILL the nft process to stop the cycle, but the desired behaviour is to return an error after the generic expr name fails to load the expression.
--- net/netfilter/nf_tables_api.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
net/netfilter/nf_tables_api.c +3 -7
@@ -6698,13 +6698,8 @@ static void nf_tables_module_autoload(struct net *net)list_splice_init (&net->nft.module_list, &module_list);mutex_unlock (&net->nft.commit_mutex);list_for_each_entry_safe (req, next, &module_list, list) { -if (req->done) { -list_del (&req->list); -kfree (req); - }else { -request_module ("%s" , req->module); - req->done = true; - } +request_module ("%s" , req->module); + req->done = true; }mutex_lock (&net->nft.commit_mutex);list_splice (&module_list, &net->nft.module_list);
@@ -7481,6 +7476,7 @@ static void __net_exit nf_tables_exit_net(struct net *net)__nft_release_tables (net);mutex_unlock (&net->nft.commit_mutex);WARN_ON_ONCE (!list_empty (&net->nft.tables)); +WARN_ON_ONCE (!list_empty (&net->nft.module_list)); }static struct pernet_operations nf_tables_net_ops = {
Signature · 1 line
2.34.1
From e6cec303e31d347aa44beb37876fa6763cc0430c Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <[email protected]>
Date: Thu, 29 Oct 2020 13:50:03 +0100
Subject: [PATCH 5/5] netfilter: nf_tables: missing validation from the abort
path
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.
--- include/linux/netfilter/nfnetlink.h | 9 ++++++++- net/netfilter/nf_tables_api.c | 15 ++++++++++----- net/netfilter/nfnetlink.c | 22 ++++++++++++++++++---- 3 files changed, 36 insertions(+), 10 deletions(-)
include/linux/netfilter/nfnetlink.h +8 -1
@@ -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); };
net/netfilter/nf_tables_api.c +10 -5
@@ -6734,11 +6734,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) {
@@ -6851,7 +6855,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);
@@ -6864,9 +6868,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);
@@ -7472,7 +7477,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));
net/netfilter/nfnetlink.c +18 -4
@@ -314,7 +314,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 );
@@ -478,7 +478,7 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, } 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);
@@ -489,11 +489,25 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, 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);
Signature · 1 line
2.34.1
From dda33be9e3fadf0b47e2afc8a0b381c3667622c3 Mon Sep 17 00:00:00 2001
From: Florian Westphal <[email protected]>
Date: Wed, 29 Aug 2018 14:41:32 +0200
Subject: [PATCH 1/5] netfilter: nf_tables: asynchronous release
Release the committed transaction log from a work queue, moving expensive synchronize_rcu out of the locked section and providing opportunity to batch this.
On my test machine this cuts runtime of nft-test.py in half. Based on earlier patch from Pablo Neira Ayuso.
--- include/net/netfilter/nf_tables.h | 2 ++ net/netfilter/nf_tables_api.c | 56 +++++++++++++++++++++++++++---- 2 files changed, 52 insertions(+), 6 deletions(-)
include/net/netfilter/nf_tables.h +2 -0
@@ -1316,12 +1316,14 @@ static inline void nft_set_elem_clear_busy(struct nft_set_ext *ext)* * @list: used internally * @msg_type: message type +* @put_net: ctx->net needs to be put * @ctx: transaction context * @data: internal information related to the transaction */ struct nft_trans {struct list_head list;int msg_type; +bool put_net;struct nft_ctx ctx;char data[0 ]; };
net/netfilter/nf_tables_api.c +50 -6
115 lines not on this page · raw message
Signature · 1 line
2.34.1
From 5a9ec0a60c682805ceb0cca413d355f75c74b9ba Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <[email protected]>
Date: Tue, 21 Jan 2020 16:48:03 +0100
Subject: [PATCH 3/5] netfilter: nf_tables: autoload modules from the abort
path
This patch introduces a list of pending module requests. This new module list is composed of nft_module_request objects that contain the module name and one status field that tells if the module has been already loaded (the 'done' field).
In the first pass, from the preparation phase, the netlink command finds that a module is missing on this list. Then, a module request is allocated and added to this list and nft_request_module() returns -EAGAIN. This triggers the abort path with the autoload parameter set on from nfnetlink, request_module() is called and the module request enters the 'done' state. Since the mutex is released when loading modules from the abort phase, the module list is zapped so this is iteration occurs over a local list. Therefore, the request_module() calls happen when object lists are in consistent state (after fulling aborting the transaction) and the commit list is empty.
On the second pass, the netlink command will find that it already tried to load the module, so it does not request it again and nft_request_module() returns 0. Then, there is a look up to find the object that the command was missing. If the module was successfully loaded, the command proceeds normally since it finds the missing object in place, otherwise -ENOENT is reported to userspace.
This patch also updates nfnetlink to include the reason to enter the abort phase, which is required for this new autoload module rationale.
--- include/linux/netfilter/nfnetlink.h | 2 +- include/net/netns/nftables.h | 1 + net/netfilter/nf_tables_api.c | 126 ++++++++++++++++++++-------- net/netfilter/nfnetlink.c | 6 +- 4 files changed, 94 insertions(+), 41 deletions(-)
include/linux/netfilter/nfnetlink.h +1 -1
@@ -31,7 +31,7 @@ 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); +int (*abort)(struct net *net,struct sk_buff *skb,bool autoload);void (*cleanup)(struct net *net);bool (*valid_genid)(struct net *net,u32 genid); };
include/net/netns/nftables.h +1 -0
@@ -7,6 +7,7 @@struct netns_nftables {struct list_head tables;struct list_head commit_list; +struct list_head module_list;struct mutex commit_mutex;unsigned int base_seq;u8 gencursor;
net/netfilter/nf_tables_api.c +89 -37
241 lines not on this page · raw message
net/netfilter/nfnetlink.c +3 -3
@@ -478,7 +478,7 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, } done:if (status &NFNL_BATCH_REPLAY ) { - ss->abort (net, oskb); + ss->abort (net, oskb, true);nfnl_err_reset (&err_list);kfree_skb (skb);module_put (ss->owner);
@@ -489,11 +489,11 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, status |=NFNL_BATCH_REPLAY ; goto done; }else if (err) { - ss->abort (net, oskb); + ss->abort (net, oskb, false);netlink_ack (oskb,nlmsg_hdr (oskb), err,NULL ); } }else { - ss->abort (net, oskb); + ss->abort (net, oskb, false); }if (ss->cleanup) ss->cleanup (net);
Signature · 1 line
2.34.1
Greg KH
maintainerQuoted 5 lines · Amy Fong
The following series backports netfilter:
nf_tables: autoload modules from abort path which fixes the bug mentioned in the following:https://syzkaller.appspot.com/bug?extid=437bf61d165c87bd40fb
Please fix up the subject lines to match the subject lines of the patches you submited, and also properly sign off on the backport you did here so that they can be accepted. As-is, we can not take them.
thanks,
greg k-h
Greg KH
maintainerQuoted 4 lines · Amy Fong
The following series backports netfilter:
nf_tables: autoload modules from abort path which fixes the bug mentioned in the following:https://syzkaller.appspot.com/bug?extid=437bf61d165c87bd40fb
Also, why is this bug special? Can it be triggered by userspace? What is the fallout from it? Who is hitting this in real systems and why must just this one syzbot problem be backported, but others not?
In other words, we need a lot more information here to judge if this is really needed, and if the backport is acceptable.
thanks,
greg k-h