| Message ID | 20230906094202.1712-1-pablo@netfilter.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | Florian Westphal |
| Headers | show |
| Series | [nf] netfilter: nf_tables: Unbreak audit log reset | expand |
On Wed, Sep 06, 2023 at 11:42:02AM +0200, Pablo Neira Ayuso wrote: > Deliver audit log from __nf_tables_dump_rules(), table dereference at > the end of the table list loop might point to the list head, leading to > this crash. Ah, of course. Sorry for the mess. I missed the fact that one may just call 'reset rules' and have it apply to all existing tables. [...] > Deliver audit log only once at the end of the rule dump+reset for > consistency with the set dump+reset. This may seem like number of audit logs is reduced, when it is actually increased: With your patch, there will be at least one notification for each chain, multiple with large chains (due to skb exhaustion). Not necessarily a problem, but worth mentioning. Also, I wonder if one should go the extra mile and add the chain name to log entries. I had considered to pass a string like "mytable:123 chain=mychain" to audit_log_nfcfg() for that matter, but it's quite a hack. > Ensure audit reset access to table under rcu read side lock. The table > list iteration holds rcu read lock side, but recent audit code > dereferences table object out of the rcu read lock side. > > Fixes: ea078ae9108e ("netfilter: nf_tables: Audit log rule reset") > Fixes: 7e9be1124dbe ("netfilter: nf_tables: Audit log setelem reset") > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> Acked-by: Phil Sutter <phil@nwl.cc>
On Wed, Sep 06, 2023 at 01:32:50PM +0200, Phil Sutter wrote: > On Wed, Sep 06, 2023 at 11:42:02AM +0200, Pablo Neira Ayuso wrote: > > Deliver audit log from __nf_tables_dump_rules(), table dereference at > > the end of the table list loop might point to the list head, leading to > > this crash. > > Ah, of course. Sorry for the mess. I missed the fact that one may just > call 'reset rules' and have it apply to all existing tables. > > [...] > > Deliver audit log only once at the end of the rule dump+reset for > > consistency with the set dump+reset. > > This may seem like number of audit logs is reduced, when it is actually > increased: With your patch, there will be at least one notification for > each chain, multiple with large chains (due to skb exhaustion). With your patch, this is called for each netlink_recvmsg() invocation, which is more audit logs. @@ -3540,9 +3544,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb, done: rcu_read_unlock(); - if (reset && idx > cb->args[0]) - audit_log_rule_reset(table, cb->seq, idx - cb->args[0]); - cb->args[0] = idx; return skb->len; } netlink dump is composed of a series of recvmsg() from userspace to fetch the multiple chunks that represent the rules in this table. If I read fine above, as the netlink dump makes progress, idx will always go over cb->args[0], which evaluates true for each recvmsg() call. With my patch this is less audit logs because audit log is only delivered once for each chain, not for each recvmsg() call. So patch description is fine as it is :) > Not necessarily a problem, but worth mentioning. Also, I wonder if > one should go the extra mile and add the chain name to log entries. > I had considered to pass a string like "mytable:123 chain=mychain" > to audit_log_nfcfg() for that matter, but it's quite a hack. Agreed. Audit folks can extend this later on according to their needs in case they need more fine grain reporting. > > Ensure audit reset access to table under rcu read side lock. The table > > list iteration holds rcu read lock side, but recent audit code > > dereferences table object out of the rcu read lock side. > > > > Fixes: ea078ae9108e ("netfilter: nf_tables: Audit log rule reset") > > Fixes: 7e9be1124dbe ("netfilter: nf_tables: Audit log setelem reset") > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > Acked-by: Phil Sutter <phil@nwl.cc>
On Wed, Sep 06, 2023 at 01:47:34PM +0200, Pablo Neira Ayuso wrote: > On Wed, Sep 06, 2023 at 01:32:50PM +0200, Phil Sutter wrote: > > On Wed, Sep 06, 2023 at 11:42:02AM +0200, Pablo Neira Ayuso wrote: > > > Deliver audit log from __nf_tables_dump_rules(), table dereference at > > > the end of the table list loop might point to the list head, leading to > > > this crash. > > > > Ah, of course. Sorry for the mess. I missed the fact that one may just > > call 'reset rules' and have it apply to all existing tables. > > > > [...] > > > Deliver audit log only once at the end of the rule dump+reset for > > > consistency with the set dump+reset. > > > > This may seem like number of audit logs is reduced, when it is actually > > increased: With your patch, there will be at least one notification for > > each chain, multiple with large chains (due to skb exhaustion). > > With your patch, this is called for each netlink_recvmsg() invocation, > which is more audit logs. > > @@ -3540,9 +3544,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb, > done: > rcu_read_unlock(); > > - if (reset && idx > cb->args[0]) > - audit_log_rule_reset(table, cb->seq, idx - cb->args[0]); > - > cb->args[0] = idx; > return skb->len; > } > > netlink dump is composed of a series of recvmsg() from userspace to > fetch the multiple chunks that represent the rules in this table. > If I read fine above, as the netlink dump makes progress, idx will > always go over cb->args[0], which evaluates true for each recvmsg() > call. With my patch this is less audit logs because audit log is only > delivered once for each chain, not for each recvmsg() call. Here's a simple example: | for table in t1 t2; do | echo "add table $table" | for chain in c1 c2 c3; do | echo "add chain $table $chain" | echo "add rule $table $chain counter accept" | echo "add rule $table $chain counter accept" | echo "add rule $table $chain counter accept" | done | done | nft -f - | nft reset rules | nft reset rules table t1 Before your patch, audit NETFILTER_CFG messages are: | type=NETFILTER_CFG msg=audit(1694018800.149:1917): table=(null):5 family=0 entries=18 op=nft_reset_rule pid=6558 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694018848.399:1918): table=t1:5 family=2 entries=9 op=nft_reset_rule pid=6565 subj=kernel comm="nft" (Note the NULL table name pointer in the first one.) With your patch applied, these are the respective audit messages: | type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t2:2 family=2 entries=12 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t2:2 family=2 entries=15 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.459:3): table=t2:2 family=2 entries=18 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t2:2 family=2 entries=12 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t2:2 family=2 entries=15 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020131.489:4): table=t2:2 family=2 entries=18 op=nft_reset_rule pid=3119 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020206.579:5): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3126 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020206.579:5): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3126 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020206.579:5): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3126 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020206.609:6): table=t1:2 family=2 entries=3 op=nft_reset_rule pid=3126 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020206.609:6): table=t1:2 family=2 entries=6 op=nft_reset_rule pid=3126 subj=kernel comm="nft" | type=NETFILTER_CFG msg=audit(1694020206.609:6): table=t1:2 family=2 entries=9 op=nft_reset_rule pid=3126 subj=kernel comm="nft" The last six come from the 'reset rules table t1' command. While on one hand it looks like nftables fits only three rules into a single skb, your fix seems to have a problem in that it doesn't subtract s_idx from *idx. Cheers, Phil
On 2023-09-06 13:47, Pablo Neira Ayuso wrote: > On Wed, Sep 06, 2023 at 01:32:50PM +0200, Phil Sutter wrote: > > On Wed, Sep 06, 2023 at 11:42:02AM +0200, Pablo Neira Ayuso wrote: > > > Deliver audit log from __nf_tables_dump_rules(), table dereference at > > > the end of the table list loop might point to the list head, leading to > > > this crash. > > > > Ah, of course. Sorry for the mess. I missed the fact that one may just > > call 'reset rules' and have it apply to all existing tables. > > > > [...] > > > Deliver audit log only once at the end of the rule dump+reset for > > > consistency with the set dump+reset. > > > > This may seem like number of audit logs is reduced, when it is actually > > increased: With your patch, there will be at least one notification for > > each chain, multiple with large chains (due to skb exhaustion). > > With your patch, this is called for each netlink_recvmsg() invocation, > which is more audit logs. > > @@ -3540,9 +3544,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb, > done: > rcu_read_unlock(); > > - if (reset && idx > cb->args[0]) > - audit_log_rule_reset(table, cb->seq, idx - cb->args[0]); > - > cb->args[0] = idx; > return skb->len; > } > > netlink dump is composed of a series of recvmsg() from userspace to > fetch the multiple chunks that represent the rules in this table. > If I read fine above, as the netlink dump makes progress, idx will > always go over cb->args[0], which evaluates true for each recvmsg() > call. With my patch this is less audit logs because audit log is only > delivered once for each chain, not for each recvmsg() call. > > So patch description is fine as it is :) > > > Not necessarily a problem, but worth mentioning. Also, I wonder if > > one should go the extra mile and add the chain name to log entries. > > I had considered to pass a string like "mytable:123 chain=mychain" > > to audit_log_nfcfg() for that matter, but it's quite a hack. (I think I missed the start of this thread?) Whatever you do, only ever add new fields to the *end* of the record to avoid confusing the userspace parser. I vaguely remember looking at adding a chain field and dismissed it. So if this needs to be done, the string needs to be parsed to see if extra fields have been added and print them at the end of the record. The better approach is to add a parameter to carry that field and optionally append it to the record. > Agreed. Audit folks can extend this later on according to their needs > in case they need more fine grain reporting. > > > > Ensure audit reset access to table under rcu read side lock. The table > > > list iteration holds rcu read lock side, but recent audit code > > > dereferences table object out of the rcu read lock side. > > > > > > Fixes: ea078ae9108e ("netfilter: nf_tables: Audit log rule reset") > > > Fixes: 7e9be1124dbe ("netfilter: nf_tables: Audit log setelem reset") > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > Acked-by: Phil Sutter <phil@nwl.cc> - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada Upstream IRC: SunRaycer Voice: +1.613.860 2354 SMS: +1.613.518.6570
On 2023-09-06 14:20, Richard Guy Briggs wrote: > On 2023-09-06 13:47, Pablo Neira Ayuso wrote: > > On Wed, Sep 06, 2023 at 01:32:50PM +0200, Phil Sutter wrote: > > > On Wed, Sep 06, 2023 at 11:42:02AM +0200, Pablo Neira Ayuso wrote: > > > > Deliver audit log from __nf_tables_dump_rules(), table dereference at > > > > the end of the table list loop might point to the list head, leading to > > > > this crash. > > > > > > Ah, of course. Sorry for the mess. I missed the fact that one may just > > > call 'reset rules' and have it apply to all existing tables. > > > > > > [...] > > > > Deliver audit log only once at the end of the rule dump+reset for > > > > consistency with the set dump+reset. > > > > > > This may seem like number of audit logs is reduced, when it is actually > > > increased: With your patch, there will be at least one notification for > > > each chain, multiple with large chains (due to skb exhaustion). > > > > With your patch, this is called for each netlink_recvmsg() invocation, > > which is more audit logs. > > > > @@ -3540,9 +3544,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb, > > done: > > rcu_read_unlock(); > > > > - if (reset && idx > cb->args[0]) > > - audit_log_rule_reset(table, cb->seq, idx - cb->args[0]); > > - > > cb->args[0] = idx; > > return skb->len; > > } > > > > netlink dump is composed of a series of recvmsg() from userspace to > > fetch the multiple chunks that represent the rules in this table. > > If I read fine above, as the netlink dump makes progress, idx will > > always go over cb->args[0], which evaluates true for each recvmsg() > > call. With my patch this is less audit logs because audit log is only > > delivered once for each chain, not for each recvmsg() call. > > > > So patch description is fine as it is :) > > > > > Not necessarily a problem, but worth mentioning. Also, I wonder if > > > one should go the extra mile and add the chain name to log entries. > > > I had considered to pass a string like "mytable:123 chain=mychain" > > > to audit_log_nfcfg() for that matter, but it's quite a hack. > > (I think I missed the start of this thread?) > > Whatever you do, only ever add new fields to the *end* of the record to > avoid confusing the userspace parser. I vaguely remember looking at > adding a chain field and dismissed it. And given that this could potentially affect userspace parsing tools, the original audit mailing list linux-audit@redhat.com should also be copied. Sorry about that... > So if this needs to be done, the string needs to be parsed to see if > extra fields have been added and print them at the end of the record. > The better approach is to add a parameter to carry that field and > optionally append it to the record. > > > Agreed. Audit folks can extend this later on according to their needs > > in case they need more fine grain reporting. > > > > > > Ensure audit reset access to table under rcu read side lock. The table > > > > list iteration holds rcu read lock side, but recent audit code > > > > dereferences table object out of the rcu read lock side. > > > > > > > > Fixes: ea078ae9108e ("netfilter: nf_tables: Audit log rule reset") > > > > Fixes: 7e9be1124dbe ("netfilter: nf_tables: Audit log setelem reset") > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > > > Acked-by: Phil Sutter <phil@nwl.cc> > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada Upstream IRC: SunRaycer Voice: +1.613.860 2354 SMS: +1.613.518.6570
On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote: [...] > The last six come from the 'reset rules table t1' command. While on one > hand it looks like nftables fits only three rules into a single skb, > your fix seems to have a problem in that it doesn't subtract s_idx from > *idx. Please, feel free to follow up to refine, thanks.
On Wed, Sep 6, 2023 at 2:46 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote: > [...] > > The last six come from the 'reset rules table t1' command. While on one > > hand it looks like nftables fits only three rules into a single skb, > > your fix seems to have a problem in that it doesn't subtract s_idx from > > *idx. > > Please, feel free to follow up to refine, thanks. Forgive me if I'm wrong, but it sounds as though Phil was pointing out a bug and not an area of refinement, is that correct Phil? If it is a bug, please submit a fix for this as soon as possible Pablo.
Hi Paul, On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote: > On Wed, Sep 6, 2023 at 2:46 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote: > > [...] > > > The last six come from the 'reset rules table t1' command. While on one > > > hand it looks like nftables fits only three rules into a single skb, > > > your fix seems to have a problem in that it doesn't subtract s_idx from > > > *idx. > > > > Please, feel free to follow up to refine, thanks. > > Forgive me if I'm wrong, but it sounds as though Phil was pointing out > a bug and not an area of refinement, is that correct Phil? From my point of view, yes. Though the third parameter "nentries" to audit_log_nfcfg() is sometimes used in rather creative ways, nf_tables_dump_obj() for instance passes the handle of the object being reset instead of a count. So I assume whoever parses audit logs won't rely too much upon the 'entries=NNN' part, anyway. > If it is a bug, please submit a fix for this as soon as possible Pablo. Thanks for your support, but I can take over, too. The number of notifications emitted even for a small ruleset is not ideal, also. It's just a bit sad that I ACKed the patch already and so it went out the door. Florian, can we still put a veto there? Cheers, Phil
On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote: > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote: [...] > > If it is a bug, please submit a fix for this as soon as possible Pablo. > > Thanks for your support, but I can take over, too. The number of > notifications emitted even for a small ruleset is not ideal, also. It's > just a bit sad that I ACKed the patch already and so it went out the > door. Florian, can we still put a veto there? Phil, kernel was crashing after your patch, this was resulting in a kernel panic when running tests here. I had to revert your patches locally to keep running tests. Please, just send an incremental fix to adjust the idx, revert will leave things in worse state. Audit does not show chains either, which is not very useful to locate what where exactly the rules have been reset, but that can probably discussed in net-next. Richard provided a way to extend this if audit maintainer find it useful too. Thanks.
On Wed, Sep 6, 2023 at 5:39 PM Phil Sutter <phil@nwl.cc> wrote: > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote: > > On Wed, Sep 6, 2023 at 2:46 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Wed, Sep 06, 2023 at 07:08:40PM +0200, Phil Sutter wrote: > > > [...] > > > > The last six come from the 'reset rules table t1' command. While on one > > > > hand it looks like nftables fits only three rules into a single skb, > > > > your fix seems to have a problem in that it doesn't subtract s_idx from > > > > *idx. > > > > > > Please, feel free to follow up to refine, thanks. > > > > Forgive me if I'm wrong, but it sounds as though Phil was pointing out > > a bug and not an area of refinement, is that correct Phil? > > From my point of view, yes. Though the third parameter "nentries" to > audit_log_nfcfg() is sometimes used in rather creative ways, > nf_tables_dump_obj() for instance passes the handle of the object being > reset instead of a count. So I assume whoever parses audit logs won't > rely too much upon the 'entries=NNN' part, anyway. > > > If it is a bug, please submit a fix for this as soon as possible Pablo. > > Thanks for your support, but I can take over, too. That works too. The only thing I really care about is making sure the code is correct and the kernel is behaving the way one would expect. Who gets it back to that state isn't much of a concern, so long as it does get fixed ;) > The number of > notifications emitted even for a small ruleset is not ideal, also. Understood. Let's first make sure that the audit records are correct, then we can work on improving the frequency.
On Wed, Sep 6, 2023 at 6:21 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote: > > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote: > [...] > > > If it is a bug, please submit a fix for this as soon as possible Pablo. > > > > Thanks for your support, but I can take over, too. The number of > > notifications emitted even for a small ruleset is not ideal, also. It's > > just a bit sad that I ACKed the patch already and so it went out the > > door. Florian, can we still put a veto there? > > Phil, kernel was crashing after your patch, this was resulting in a > kernel panic when running tests here. I had to revert your patches > locally to keep running tests. > > Please, just send an incremental fix to adjust the idx, revert will > leave things in worse state. If we can get a fix out soon then I'm fine with that, if we can't get a fix out soon then a revert may be wise. > Audit does not show chains either, which is not very useful to locate > what where exactly the rules have been reset, but that can probably > discussed in net-next. Richard provided a way to extend this if audit > maintainer find it useful too. Richard was correct in saying that new fields must be added to the end of the record. The only correction I would make to Richard's comments is that we tend to prefer that if a field is present in a record, it is always present in a record; if there is no useful information to log in that field, a "?" can be substituted for the value (e.g. "nftfield=?").
On Wed, Sep 06, 2023 at 06:41:13PM -0400, Paul Moore wrote: > On Wed, Sep 6, 2023 at 6:21 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote: > > > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote: > > [...] > > > > If it is a bug, please submit a fix for this as soon as possible Pablo. > > > > > > Thanks for your support, but I can take over, too. The number of > > > notifications emitted even for a small ruleset is not ideal, also. It's > > > just a bit sad that I ACKed the patch already and so it went out the > > > door. Florian, can we still put a veto there? > > > > Phil, kernel was crashing after your patch, this was resulting in a > > kernel panic when running tests here. I had to revert your patches > > locally to keep running tests. > > > > Please, just send an incremental fix to adjust the idx, revert will > > leave things in worse state. > > If we can get a fix out soon then I'm fine with that, if we can't get > a fix out soon then a revert may be wise. I believe it should be possible to fix this in the next -rc, which should be quick. If Phil is busy I will jump on this and I will keep you on Cc so you and Richard can review. I apologize for forgetting to Cc you in first place. > > Audit does not show chains either, which is not very useful to locate > > what where exactly the rules have been reset, but that can probably > > discussed in net-next. Richard provided a way to extend this if audit > > maintainer find it useful too. > > Richard was correct in saying that new fields must be added to the end > of the record. The only correction I would make to Richard's comments > is that we tend to prefer that if a field is present in a record, it > is always present in a record; if there is no useful information to > log in that field, a "?" can be substituted for the value (e.g. > "nftfield=?"). Thanks for clarification, hopefully this will help to explore extensions to include chain information in the logs. I think that might help users to understand better the kind of updated that happened in the Netfilter subsystem.
On Wed, Sep 6, 2023 at 7:01 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Wed, Sep 06, 2023 at 06:41:13PM -0400, Paul Moore wrote: > > On Wed, Sep 6, 2023 at 6:21 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Wed, Sep 06, 2023 at 11:39:41PM +0200, Phil Sutter wrote: > > > > On Wed, Sep 06, 2023 at 03:56:41PM -0400, Paul Moore wrote: > > > [...] > > > > > If it is a bug, please submit a fix for this as soon as possible Pablo. > > > > > > > > Thanks for your support, but I can take over, too. The number of > > > > notifications emitted even for a small ruleset is not ideal, also. It's > > > > just a bit sad that I ACKed the patch already and so it went out the > > > > door. Florian, can we still put a veto there? > > > > > > Phil, kernel was crashing after your patch, this was resulting in a > > > kernel panic when running tests here. I had to revert your patches > > > locally to keep running tests. > > > > > > Please, just send an incremental fix to adjust the idx, revert will > > > leave things in worse state. > > > > If we can get a fix out soon then I'm fine with that, if we can't get > > a fix out soon then a revert may be wise. > > I believe it should be possible to fix this in the next -rc, which > should be quick. If Phil is busy I will jump on this and I will keep > you on Cc so you and Richard can review. Great, thank you. > I apologize for forgetting to Cc you in first place. No worries :) > > > Audit does not show chains either, which is not very useful to locate > > > what where exactly the rules have been reset, but that can probably > > > discussed in net-next. Richard provided a way to extend this if audit > > > maintainer find it useful too. > > > > Richard was correct in saying that new fields must be added to the end > > of the record. The only correction I would make to Richard's comments > > is that we tend to prefer that if a field is present in a record, it > > is always present in a record; if there is no useful information to > > log in that field, a "?" can be substituted for the value (e.g. > > "nftfield=?"). > > Thanks for clarification, hopefully this will help to explore > extensions to include chain information in the logs. I think that > might help users to understand better the kind of updated that > happened in the Netfilter subsystem. Great, I'll look forward to the patches.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 2c81cee858d6..e429ebba74b3 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3480,6 +3480,10 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, cont_skip: (*idx)++; } + + if (reset && *idx) + audit_log_rule_reset(table, cb->seq, *idx); + return 0; } @@ -3540,9 +3544,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb, done: rcu_read_unlock(); - if (reset && idx > cb->args[0]) - audit_log_rule_reset(table, cb->seq, idx - cb->args[0]); - cb->args[0] = idx; return skb->len; } @@ -5760,8 +5761,6 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb) if (!args.iter.err && args.iter.count == cb->args[0]) args.iter.err = nft_set_catchall_dump(net, skb, set, reset, cb->seq); - rcu_read_unlock(); - nla_nest_end(skb, nest); nlmsg_end(skb, nlh); @@ -5769,6 +5768,8 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb) audit_log_nft_set_reset(table, cb->seq, args.iter.count - args.iter.skip); + rcu_read_unlock(); + if (args.iter.err && args.iter.err != -EMSGSIZE) return args.iter.err; if (args.iter.count == cb->args[0])
Deliver audit log from __nf_tables_dump_rules(), table dereference at the end of the table list loop might point to the list head, leading to this crash. [ 4137.407349] BUG: unable to handle page fault for address: 00000000001f3c50 [ 4137.407357] #PF: supervisor read access in kernel mode [ 4137.407359] #PF: error_code(0x0000) - not-present page [ 4137.407360] PGD 0 P4D 0 [ 4137.407363] Oops: 0000 [#1] PREEMPT SMP PTI [ 4137.407365] CPU: 4 PID: 500177 Comm: nft Not tainted 6.5.0+ #277 [ 4137.407369] RIP: 0010:string+0x49/0xd0 [ 4137.407374] Code: ff 77 36 45 89 d1 31 f6 49 01 f9 66 45 85 d2 75 19 eb 1e 49 39 f8 76 02 88 07 48 83 c7 01 83 c6 01 48 83 c2 01 4c 39 cf 74 07 <0f> b6 02 84 c0 75 e2 4c 89 c2 e9 58 e5 ff ff 48 c7 c0 0e b2 ff 81 [ 4137.407377] RSP: 0018:ffff8881179737f0 EFLAGS: 00010286 [ 4137.407379] RAX: 00000000001f2c50 RBX: ffff888117973848 RCX: ffff0a00ffffff04 [ 4137.407380] RDX: 00000000001f3c50 RSI: 0000000000000000 RDI: 0000000000000000 [ 4137.407381] RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000ffffffff [ 4137.407383] R10: ffffffffffffffff R11: ffff88813584d200 R12: 0000000000000000 [ 4137.407384] R13: ffffffffa15cf709 R14: 0000000000000000 R15: ffffffffa15cf709 [ 4137.407385] FS: 00007fcfc18bb580(0000) GS:ffff88840e700000(0000) knlGS:0000000000000000 [ 4137.407387] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 4137.407388] CR2: 00000000001f3c50 CR3: 00000001055b2001 CR4: 00000000001706e0 [ 4137.407390] Call Trace: [ 4137.407392] <TASK> [ 4137.407393] ? __die+0x1b/0x60 [ 4137.407397] ? page_fault_oops+0x6b/0xa0 [ 4137.407399] ? exc_page_fault+0x60/0x120 [ 4137.407403] ? asm_exc_page_fault+0x22/0x30 [ 4137.407408] ? string+0x49/0xd0 [ 4137.407410] vsnprintf+0x257/0x4f0 [ 4137.407414] kvasprintf+0x3e/0xb0 [ 4137.407417] kasprintf+0x3e/0x50 [ 4137.407419] nf_tables_dump_rules+0x1c0/0x360 [nf_tables] [ 4137.407439] ? __alloc_skb+0xc3/0x170 [ 4137.407442] netlink_dump+0x170/0x330 [ 4137.407447] __netlink_dump_start+0x227/0x300 [ 4137.407449] nf_tables_getrule+0x205/0x390 [nf_tables] Deliver audit log only once at the end of the rule dump+reset for consistency with the set dump+reset. Ensure audit reset access to table under rcu read side lock. The table list iteration holds rcu read lock side, but recent audit code dereferences table object out of the rcu read lock side. Fixes: ea078ae9108e ("netfilter: nf_tables: Audit log rule reset") Fixes: 7e9be1124dbe ("netfilter: nf_tables: Audit log setelem reset") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- Supersedes: netfilter: nf_tables: ensure audit reset access to table under rcu read side lock https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230902185656.12022-1-pablo@netfilter.org/ net/netfilter/nf_tables_api.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)