Re: [nf PATCH 1/2] netfilter: nf_tables: Audit log setelem reset
On Wed, Aug 30, 2023 at 4:54 PM Richard Guy Briggs wrote: > On 2023-08-30 17:46, Pablo Neira Ayuso wrote: > > On Tue, Aug 29, 2023 at 07:51:57PM +0200, Phil Sutter wrote: > > > Since set element reset is not integrated into nf_tables' transaction > > > logic, an explicit log call is needed, similar to NFT_MSG_GETOBJ_RESET > > > handling. > > > > > > For the sake of simplicity, catchall element reset will always generate > > > a dedicated log entry. This relieves nf_tables_dump_set() from having to > > > adjust the logged element count depending on whether a catchall element > > > was found or not. > > > > Applied, thanks Phil > > Thanks Phil, Pablo. If it isn't too late, please add my > Reviewed-by: Richard Guy Briggs Similarly, you can add my ACK. FWIW, if you're sending patches out during the first few days of the merge window it might be advisable to wait more than a day or two to give the relevant maintainers a chance to review the patch. Also, as a note for future submissions, we've moved the audit kernel mailing list to au...@vger.kernel.org. Acked-by: Paul Moore -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [nf PATCH 1/2] netfilter: nf_tables: Audit log setelem reset
On 2023-08-30 17:46, Pablo Neira Ayuso wrote: > On Tue, Aug 29, 2023 at 07:51:57PM +0200, Phil Sutter wrote: > > Since set element reset is not integrated into nf_tables' transaction > > logic, an explicit log call is needed, similar to NFT_MSG_GETOBJ_RESET > > handling. > > > > For the sake of simplicity, catchall element reset will always generate > > a dedicated log entry. This relieves nf_tables_dump_set() from having to > > adjust the logged element count depending on whether a catchall element > > was found or not. > > Applied, thanks Phil Thanks Phil, Pablo. If it isn't too late, please add my Reviewed-by: Richard Guy Briggs - RGB -- Richard Guy Briggs 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [nf PATCH 1/2] netfilter: nf_tables: Audit log setelem reset
On Tue, Aug 29, 2023 at 07:51:57PM +0200, Phil Sutter wrote: > Since set element reset is not integrated into nf_tables' transaction > logic, an explicit log call is needed, similar to NFT_MSG_GETOBJ_RESET > handling. > > For the sake of simplicity, catchall element reset will always generate > a dedicated log entry. This relieves nf_tables_dump_set() from having to > adjust the logged element count depending on whether a catchall element > was found or not. Applied, thanks Phil -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [nf PATCH 1/2] netfilter: nf_tables: Audit log setelem reset
On Tue, Aug 29, 2023 at 08:02:30PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 29, 2023 at 07:51:57PM +0200, Phil Sutter wrote:
> > Since set element reset is not integrated into nf_tables' transaction
> > logic, an explicit log call is needed, similar to NFT_MSG_GETOBJ_RESET
> > handling.
> >
> > For the sake of simplicity, catchall element reset will always generate
> > a dedicated log entry. This relieves nf_tables_dump_set() from having to
> > adjust the logged element count depending on whether a catchall element
> > was found or not.
> >
> > Cc: Richard Guy Briggs
> > Fixes: 079cd633219d7 ("netfilter: nf_tables: Introduce
> > NFT_MSG_GETSETELEM_RESET")
> > Signed-off-by: Phil Sutter
> > ---
> > include/linux/audit.h | 1 +
> > kernel/auditsc.c | 1 +
> > net/netfilter/nf_tables_api.c | 31 ---
> > 3 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 6a3a9e122bb5e..192bf03aacc52 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -117,6 +117,7 @@ enum audit_nfcfgop {
> > AUDIT_NFT_OP_OBJ_RESET,
> > AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> > AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> > + AUDIT_NFT_OP_SETELEM_RESET,
> > AUDIT_NFT_OP_INVALID,
> > };
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index addeed3df15d3..38481e3181975 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -143,6 +143,7 @@ static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
> > { AUDIT_NFT_OP_OBJ_RESET, "nft_reset_obj"},
> > { AUDIT_NFT_OP_FLOWTABLE_REGISTER, "nft_register_flowtable" },
> > { AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,"nft_unregister_flowtable" },
> > + { AUDIT_NFT_OP_SETELEM_RESET, "nft_reset_setelem"},
> > { AUDIT_NFT_OP_INVALID, "nft_invalid" },
> > };
> >
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 1ddbdca4e47d6..a1218ea4e0c3d 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -102,6 +102,7 @@ static const u8 nft2audit_op[NFT_MSG_MAX] = { // enum
> > nf_tables_msg_types
> > [NFT_MSG_NEWFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> > [NFT_MSG_GETFLOWTABLE] = AUDIT_NFT_OP_INVALID,
> > [NFT_MSG_DELFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> > + [NFT_MSG_GETSETELEM_RESET] = AUDIT_NFT_OP_SETELEM_RESET,
> > };
> >
> > static void nft_validate_state_update(struct nft_table *table, u8
> > new_validate_state)
> > @@ -5661,13 +5662,25 @@ static int nf_tables_dump_setelem(const struct
> > nft_ctx *ctx,
> > return nf_tables_fill_setelem(args->skb, set, elem, args->reset);
> > }
> >
> > +static void audit_log_nft_set_reset(const struct nft_table *table,
> > + unsigned int base_seq,
> > + unsigned int nentries)
> > +{
> > + char *buf = kasprintf(GFP_ATOMIC, "%s:%u", table->name, base_seq);
>
> No check for NULL?
>
> I can see we have more like this in the tree.
Well, I merely copy-pasted that bit. (No excuse, I know.) The pointer is
passed on to audit_log_format() as a string argument and will end at
vsnprintf(), which detects and sanitizes NULL string args.
Cheers, Phil
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [nf PATCH 1/2] netfilter: nf_tables: Audit log setelem reset
On Tue, Aug 29, 2023 at 07:51:57PM +0200, Phil Sutter wrote:
> Since set element reset is not integrated into nf_tables' transaction
> logic, an explicit log call is needed, similar to NFT_MSG_GETOBJ_RESET
> handling.
>
> For the sake of simplicity, catchall element reset will always generate
> a dedicated log entry. This relieves nf_tables_dump_set() from having to
> adjust the logged element count depending on whether a catchall element
> was found or not.
>
> Cc: Richard Guy Briggs
> Fixes: 079cd633219d7 ("netfilter: nf_tables: Introduce
> NFT_MSG_GETSETELEM_RESET")
> Signed-off-by: Phil Sutter
> ---
> include/linux/audit.h | 1 +
> kernel/auditsc.c | 1 +
> net/netfilter/nf_tables_api.c | 31 ---
> 3 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 6a3a9e122bb5e..192bf03aacc52 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -117,6 +117,7 @@ enum audit_nfcfgop {
> AUDIT_NFT_OP_OBJ_RESET,
> AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> + AUDIT_NFT_OP_SETELEM_RESET,
> AUDIT_NFT_OP_INVALID,
> };
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index addeed3df15d3..38481e3181975 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -143,6 +143,7 @@ static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
> { AUDIT_NFT_OP_OBJ_RESET, "nft_reset_obj"},
> { AUDIT_NFT_OP_FLOWTABLE_REGISTER, "nft_register_flowtable" },
> { AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,"nft_unregister_flowtable" },
> + { AUDIT_NFT_OP_SETELEM_RESET, "nft_reset_setelem"},
> { AUDIT_NFT_OP_INVALID, "nft_invalid" },
> };
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 1ddbdca4e47d6..a1218ea4e0c3d 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -102,6 +102,7 @@ static const u8 nft2audit_op[NFT_MSG_MAX] = { // enum
> nf_tables_msg_types
> [NFT_MSG_NEWFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> [NFT_MSG_GETFLOWTABLE] = AUDIT_NFT_OP_INVALID,
> [NFT_MSG_DELFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> + [NFT_MSG_GETSETELEM_RESET] = AUDIT_NFT_OP_SETELEM_RESET,
> };
>
> static void nft_validate_state_update(struct nft_table *table, u8
> new_validate_state)
> @@ -5661,13 +5662,25 @@ static int nf_tables_dump_setelem(const struct
> nft_ctx *ctx,
> return nf_tables_fill_setelem(args->skb, set, elem, args->reset);
> }
>
> +static void audit_log_nft_set_reset(const struct nft_table *table,
> + unsigned int base_seq,
> + unsigned int nentries)
> +{
> + char *buf = kasprintf(GFP_ATOMIC, "%s:%u", table->name, base_seq);
No check for NULL?
I can see we have more like this in the tree.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[nf PATCH 1/2] netfilter: nf_tables: Audit log setelem reset
Since set element reset is not integrated into nf_tables' transaction
logic, an explicit log call is needed, similar to NFT_MSG_GETOBJ_RESET
handling.
For the sake of simplicity, catchall element reset will always generate
a dedicated log entry. This relieves nf_tables_dump_set() from having to
adjust the logged element count depending on whether a catchall element
was found or not.
Cc: Richard Guy Briggs
Fixes: 079cd633219d7 ("netfilter: nf_tables: Introduce
NFT_MSG_GETSETELEM_RESET")
Signed-off-by: Phil Sutter
---
include/linux/audit.h | 1 +
kernel/auditsc.c | 1 +
net/netfilter/nf_tables_api.c | 31 ---
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 6a3a9e122bb5e..192bf03aacc52 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -117,6 +117,7 @@ enum audit_nfcfgop {
AUDIT_NFT_OP_OBJ_RESET,
AUDIT_NFT_OP_FLOWTABLE_REGISTER,
AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
+ AUDIT_NFT_OP_SETELEM_RESET,
AUDIT_NFT_OP_INVALID,
};
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index addeed3df15d3..38481e3181975 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -143,6 +143,7 @@ static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
{ AUDIT_NFT_OP_OBJ_RESET, "nft_reset_obj"},
{ AUDIT_NFT_OP_FLOWTABLE_REGISTER, "nft_register_flowtable" },
{ AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,"nft_unregister_flowtable" },
+ { AUDIT_NFT_OP_SETELEM_RESET, "nft_reset_setelem"},
{ AUDIT_NFT_OP_INVALID, "nft_invalid" },
};
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 1ddbdca4e47d6..a1218ea4e0c3d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -102,6 +102,7 @@ static const u8 nft2audit_op[NFT_MSG_MAX] = { // enum
nf_tables_msg_types
[NFT_MSG_NEWFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_REGISTER,
[NFT_MSG_GETFLOWTABLE] = AUDIT_NFT_OP_INVALID,
[NFT_MSG_DELFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
+ [NFT_MSG_GETSETELEM_RESET] = AUDIT_NFT_OP_SETELEM_RESET,
};
static void nft_validate_state_update(struct nft_table *table, u8
new_validate_state)
@@ -5661,13 +5662,25 @@ static int nf_tables_dump_setelem(const struct nft_ctx
*ctx,
return nf_tables_fill_setelem(args->skb, set, elem, args->reset);
}
+static void audit_log_nft_set_reset(const struct nft_table *table,
+ unsigned int base_seq,
+ unsigned int nentries)
+{
+ char *buf = kasprintf(GFP_ATOMIC, "%s:%u", table->name, base_seq);
+
+ audit_log_nfcfg(buf, table->family, nentries,
+ AUDIT_NFT_OP_SETELEM_RESET, GFP_ATOMIC);
+ kfree(buf);
+}
+
struct nft_set_dump_ctx {
const struct nft_set*set;
struct nft_ctx ctx;
};
static int nft_set_catchall_dump(struct net *net, struct sk_buff *skb,
-const struct nft_set *set, bool reset)
+const struct nft_set *set, bool reset,
+unsigned int base_seq)
{
struct nft_set_elem_catchall *catchall;
u8 genmask = nft_genmask_cur(net);
@@ -5683,6 +5696,8 @@ static int nft_set_catchall_dump(struct net *net, struct
sk_buff *skb,
elem.priv = catchall->elem;
ret = nf_tables_fill_setelem(skb, set, , reset);
+ if (reset && !ret)
+ audit_log_nft_set_reset(set->table, base_seq, 1);
break;
}
@@ -5762,12 +5777,17 @@ static int nf_tables_dump_set(struct sk_buff *skb,
struct netlink_callback *cb)
set->ops->walk(_ctx->ctx, set, );
if (!args.iter.err && args.iter.count == cb->args[0])
- args.iter.err = nft_set_catchall_dump(net, skb, set, reset);
+ 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);
+ if (reset && args.iter.count > args.iter.skip)
+ audit_log_nft_set_reset(table, cb->seq,
+ args.iter.count - args.iter.skip);
+
if (args.iter.err && args.iter.err != -EMSGSIZE)
return args.iter.err;
if (args.iter.count == cb->args[0])
@@ -5992,13 +6012,13 @@ static int nf_tables_getsetelem(struct sk_buff *skb,
struct netlink_ext_ack *extack = info->extack;
u8 genmask = nft_genmask_cur(info->net);
u8 family = info->nfmsg->nfgen_family;
+ int rem, err = 0, nelems = 0;
struct net *net = info->net;
struct nft_table *table;
struct nft_set *set;
struct nlattr *attr;
