* [nf-next PATCH] netfilter: nf_tables: Introduce NFTA_RULE_ALT_EXPRESSIONS
@ 2022-12-15 20:43 Phil Sutter
2022-12-16 1:16 ` Florian Westphal
0 siblings, 1 reply; 4+ messages in thread
From: Phil Sutter @ 2022-12-15 20:43 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal
With identical content as NFTA_RULE_EXPRESSIONS, data in this attribute
is dumped in place of the live expressions, which in turn are dumped as
NFTA_RULE_ALT_EXPRESSIONS.
This allows for newer user space to provide a rule representation
understood by older user space while still able to verify the rule's
actual expressions applied to packets.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
include/net/netfilter/nf_tables.h | 12 ++++++
include/uapi/linux/netfilter/nf_tables.h | 3 ++
net/netfilter/nf_tables_api.c | 47 +++++++++++++++++++++---
3 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index e69ce23566eab..b08e01d19e835 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -946,10 +946,21 @@ struct nft_expr_ops {
void *data;
};
+/**
+ * struct nft_alt_expr - nft_tables rule alternate expressions
+ * @dlen: length of @data
+ * @data: blob used as payload of NFTA_RULE_EXPRESSIONS attribute
+ */
+struct nft_alt_expr {
+ int dlen;
+ char data[];
+};
+
/**
* struct nft_rule - nf_tables rule
*
* @list: used internally
+ * @alt_expr: Expression blob to dump instead of live data
* @handle: rule handle
* @genmask: generation mask
* @dlen: length of expression data
@@ -958,6 +969,7 @@ struct nft_expr_ops {
*/
struct nft_rule {
struct list_head list;
+ struct nft_alt_expr *alt_expr;
u64 handle:42,
genmask:2,
dlen:12,
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index cfa844da1ce61..2dff92f527429 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -247,6 +247,8 @@ enum nft_chain_attributes {
* @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
* @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
* @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule (NLA_U32)
+ * @NFTA_RULE_CHAIN_ID: add the rule to chain by ID, alternative to @NFTA_RULE_CHAIN (NLA_U32)
+ * @NFTA_RULE_ALT_EXPRESSIONS: expressions to swap with @NFTA_RULE_EXPRESSIONS for dumps (NLA_NESTED: nft_expr_attributes)
*/
enum nft_rule_attributes {
NFTA_RULE_UNSPEC,
@@ -261,6 +263,7 @@ enum nft_rule_attributes {
NFTA_RULE_ID,
NFTA_RULE_POSITION_ID,
NFTA_RULE_CHAIN_ID,
+ NFTA_RULE_ALT_EXPRESSIONS,
__NFTA_RULE_MAX
};
#define NFTA_RULE_MAX (__NFTA_RULE_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 6269b0d9977c6..d9b95a19bb028 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3064,14 +3064,33 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
if (chain->flags & NFT_CHAIN_HW_OFFLOAD)
nft_flow_rule_stats(chain, rule);
- list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
- if (list == NULL)
- goto nla_put_failure;
- nft_rule_for_each_expr(expr, next, rule) {
- if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
+ if (rule->alt_expr) {
+ if (nla_put(skb, NFTA_RULE_EXPRESSIONS,
+ rule->alt_expr->dlen, rule->alt_expr->data) < 0)
+ goto nla_put_failure;
+ } else {
+ list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
+ if (!list)
goto nla_put_failure;
+
+ nft_rule_for_each_expr(expr, next, rule) {
+ if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
+ goto nla_put_failure;
+ }
+ nla_nest_end(skb, list);
+ }
+
+ if (rule->alt_expr) {
+ list = nla_nest_start_noflag(skb, NFTA_RULE_ALT_EXPRESSIONS);
+ if (!list)
+ goto nla_put_failure;
+
+ nft_rule_for_each_expr(expr, next, rule) {
+ if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
+ goto nla_put_failure;
+ }
+ nla_nest_end(skb, list);
}
- nla_nest_end(skb, list);
if (rule->udata) {
struct nft_userdata *udata = nft_userdata(rule);
@@ -3366,6 +3385,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
nf_tables_expr_destroy(ctx, expr);
expr = next;
}
+ kfree(rule->alt_expr);
kfree(rule);
}
@@ -3443,6 +3463,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
struct nft_rule *rule, *old_rule = NULL;
struct nft_expr_info *expr_info = NULL;
u8 family = info->nfmsg->nfgen_family;
+ struct nft_alt_expr *alt_expr = NULL;
struct nft_flow_rule *flow = NULL;
struct net *net = info->net;
struct nft_userdata *udata;
@@ -3556,6 +3577,19 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
if (size >= 1 << 12)
goto err_release_expr;
+ if (nla[NFTA_RULE_ALT_EXPRESSIONS]) {
+ int dlen = nla_len(nla[NFTA_RULE_ALT_EXPRESSIONS]);
+
+ alt_expr = kvmalloc(sizeof(*alt_expr) + dlen, GFP_KERNEL);
+ if (!alt_expr) {
+ err = -ENOMEM;
+ goto err_release_expr;
+ }
+ alt_expr->dlen = dlen;
+ nla_memcpy(alt_expr->data,
+ nla[NFTA_RULE_ALT_EXPRESSIONS], dlen);
+ }
+
if (nla[NFTA_RULE_USERDATA]) {
ulen = nla_len(nla[NFTA_RULE_USERDATA]);
if (ulen > 0)
@@ -3572,6 +3606,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
rule->handle = handle;
rule->dlen = size;
rule->udata = ulen ? 1 : 0;
+ rule->alt_expr = alt_expr;
if (ulen) {
udata = nft_userdata(rule);
--
2.38.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Introduce NFTA_RULE_ALT_EXPRESSIONS
2022-12-15 20:43 [nf-next PATCH] netfilter: nf_tables: Introduce NFTA_RULE_ALT_EXPRESSIONS Phil Sutter
@ 2022-12-16 1:16 ` Florian Westphal
2022-12-16 12:36 ` Phil Sutter
0 siblings, 1 reply; 4+ messages in thread
From: Florian Westphal @ 2022-12-16 1:16 UTC (permalink / raw)
To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel, Florian Westphal
Phil Sutter <phil@nwl.cc> wrote:
> With identical content as NFTA_RULE_EXPRESSIONS, data in this attribute
> is dumped in place of the live expressions, which in turn are dumped as
> NFTA_RULE_ALT_EXPRESSIONS.
This name isn't very descriptive.
Or at least mention that this is for iptables-nft/NFT_COMPAT sake?
Perhaps its better to use two distinct attributes?
NFTA_RULE_EXPRESSIONS_COMPAT (input)
NFTA_RULE_EXPRESSIONS_ACTUAL (output)?
The switcheroo of ALT (old crap on input, actual live ruleset on output)
is very unintuitive.
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> include/net/netfilter/nf_tables.h | 12 ++++++
> include/uapi/linux/netfilter/nf_tables.h | 3 ++
> net/netfilter/nf_tables_api.c | 47 +++++++++++++++++++++---
> 3 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index e69ce23566eab..b08e01d19e835 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -946,10 +946,21 @@ struct nft_expr_ops {
> void *data;
> };
>
> +/**
> + * struct nft_alt_expr - nft_tables rule alternate expressions
> + * @dlen: length of @data
> + * @data: blob used as payload of NFTA_RULE_EXPRESSIONS attribute
> + */
> +struct nft_alt_expr {
> + int dlen;
> + char data[];
> +};
> +
> /**
> * struct nft_rule - nf_tables rule
> *
> * @list: used internally
> + * @alt_expr: Expression blob to dump instead of live data
> * @handle: rule handle
> * @genmask: generation mask
> * @dlen: length of expression data
> @@ -958,6 +969,7 @@ struct nft_expr_ops {
> */
> struct nft_rule {
> struct list_head list;
> + struct nft_alt_expr *alt_expr;
> u64 handle:42,
> genmask:2,
> dlen:12,
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index cfa844da1ce61..2dff92f527429 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -247,6 +247,8 @@ enum nft_chain_attributes {
> * @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
> * @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
> * @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule (NLA_U32)
> + * @NFTA_RULE_CHAIN_ID: add the rule to chain by ID, alternative to @NFTA_RULE_CHAIN (NLA_U32)
> + * @NFTA_RULE_ALT_EXPRESSIONS: expressions to swap with @NFTA_RULE_EXPRESSIONS for dumps (NLA_NESTED: nft_expr_attributes)
> */
> enum nft_rule_attributes {
> NFTA_RULE_UNSPEC,
> @@ -261,6 +263,7 @@ enum nft_rule_attributes {
> NFTA_RULE_ID,
> NFTA_RULE_POSITION_ID,
> NFTA_RULE_CHAIN_ID,
> + NFTA_RULE_ALT_EXPRESSIONS,
You need to update the nla_policy too.
> + if (nla_put(skb, NFTA_RULE_EXPRESSIONS,
> + rule->alt_expr->dlen, rule->alt_expr->data) < 0)
> + goto nla_put_failure;
> + } else {
> + list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
> + if (!list)
> goto nla_put_failure;
> +
> + nft_rule_for_each_expr(expr, next, rule) {
> + if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
> + goto nla_put_failure;
> + }
> + nla_nest_end(skb, list);
> + }
> +
> + if (rule->alt_expr) {
> + list = nla_nest_start_noflag(skb, NFTA_RULE_ALT_EXPRESSIONS);
> + if (!list)
> + goto nla_put_failure;
> +
> + nft_rule_for_each_expr(expr, next, rule) {
> + if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
> + goto nla_put_failure;
> + }
> + nla_nest_end(skb, list);
How does userspace know if NFTA_RULE_EXPRESSIONS is the backward annotated
kludge or the live/real ruleset? Check for presence of ALT attribute?
> - nla_nest_end(skb, list);
>
> if (rule->udata) {
> struct nft_userdata *udata = nft_userdata(rule);
> @@ -3366,6 +3385,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
> nf_tables_expr_destroy(ctx, expr);
> expr = next;
> }
> + kfree(rule->alt_expr);
> kfree(rule);
> }
>
> @@ -3443,6 +3463,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
> struct nft_rule *rule, *old_rule = NULL;
> struct nft_expr_info *expr_info = NULL;
> u8 family = info->nfmsg->nfgen_family;
> + struct nft_alt_expr *alt_expr = NULL;
> struct nft_flow_rule *flow = NULL;
> struct net *net = info->net;
> struct nft_userdata *udata;
> @@ -3556,6 +3577,19 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
> if (size >= 1 << 12)
> goto err_release_expr;
>
> + if (nla[NFTA_RULE_ALT_EXPRESSIONS]) {
> + int dlen = nla_len(nla[NFTA_RULE_ALT_EXPRESSIONS]);
> + alt_expr = kvmalloc(sizeof(*alt_expr) + dlen, GFP_KERNEL);
Once nla_policy provides a maxlen this is fine.
> + nla_memcpy(alt_expr->data,
> + nla[NFTA_RULE_ALT_EXPRESSIONS], dlen);
Hmm, I wonder if this isn't a problem.
The kernel will now dump arbitrary data to userspace, whereas without
this patch the nla data is generated by kernel, i.e. never malformed.
I wonder if the alt blob needs to go through some type of validation
too?
Also I think that this attribute should always be ignored for
NFT_COMPAT=n builds, we should document this kludge is only for
iptables-nft sake (or rather, incorrect a**umptions by 3rd
party wrappers of iptables userspace...).
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Introduce NFTA_RULE_ALT_EXPRESSIONS
2022-12-16 1:16 ` Florian Westphal
@ 2022-12-16 12:36 ` Phil Sutter
2022-12-16 13:26 ` Florian Westphal
0 siblings, 1 reply; 4+ messages in thread
From: Phil Sutter @ 2022-12-16 12:36 UTC (permalink / raw)
To: Florian Westphal; +Cc: Pablo Neira Ayuso, netfilter-devel
Hi,
On Fri, Dec 16, 2022 at 02:16:41AM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > With identical content as NFTA_RULE_EXPRESSIONS, data in this attribute
> > is dumped in place of the live expressions, which in turn are dumped as
> > NFTA_RULE_ALT_EXPRESSIONS.
>
> This name isn't very descriptive.
>
> Or at least mention that this is for iptables-nft/NFT_COMPAT sake?
I could move it into NFTA_RULE_COMPAT.
> Perhaps its better to use two distinct attributes?
>
> NFTA_RULE_EXPRESSIONS_COMPAT (input)
> NFTA_RULE_EXPRESSIONS_ACTUAL (output)?
With NFTA_RULE_EXPRESSIONS_ACTUAL replacing NFTA_RULE_EXPRESSIONS?
> The switcheroo of ALT (old crap on input, actual live ruleset on output)
> is very unintuitive.
Well, providing something that replaces the content of
NFTA_RULE_EXPRESSIONS is the basic concept of this approach. Restricting
it to output only is not possible since old user space uses it for
input. I guess one could make new user space use the *_COMPAT/*_ACTUAL
attributes above but kernel still has to fall back to plain
*_EXPRESSIONS.
Adding the two attributes above and using *_COMPAT for input only and
*_ACTUAL for output only is probably a trivial change though.
[...]
> > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > index cfa844da1ce61..2dff92f527429 100644
> > --- a/include/uapi/linux/netfilter/nf_tables.h
> > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > @@ -247,6 +247,8 @@ enum nft_chain_attributes {
> > * @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
> > * @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
> > * @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule (NLA_U32)
> > + * @NFTA_RULE_CHAIN_ID: add the rule to chain by ID, alternative to @NFTA_RULE_CHAIN (NLA_U32)
> > + * @NFTA_RULE_ALT_EXPRESSIONS: expressions to swap with @NFTA_RULE_EXPRESSIONS for dumps (NLA_NESTED: nft_expr_attributes)
> > */
> > enum nft_rule_attributes {
> > NFTA_RULE_UNSPEC,
> > @@ -261,6 +263,7 @@ enum nft_rule_attributes {
> > NFTA_RULE_ID,
> > NFTA_RULE_POSITION_ID,
> > NFTA_RULE_CHAIN_ID,
> > + NFTA_RULE_ALT_EXPRESSIONS,
>
> You need to update the nla_policy too.
ACK, thanks!
> > + if (nla_put(skb, NFTA_RULE_EXPRESSIONS,
> > + rule->alt_expr->dlen, rule->alt_expr->data) < 0)
> > + goto nla_put_failure;
> > + } else {
> > + list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
> > + if (!list)
> > goto nla_put_failure;
> > +
> > + nft_rule_for_each_expr(expr, next, rule) {
> > + if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
> > + goto nla_put_failure;
> > + }
> > + nla_nest_end(skb, list);
> > + }
> > +
> > + if (rule->alt_expr) {
> > + list = nla_nest_start_noflag(skb, NFTA_RULE_ALT_EXPRESSIONS);
> > + if (!list)
> > + goto nla_put_failure;
> > +
> > + nft_rule_for_each_expr(expr, next, rule) {
> > + if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
> > + goto nla_put_failure;
> > + }
> > + nla_nest_end(skb, list);
>
> How does userspace know if NFTA_RULE_EXPRESSIONS is the backward annotated
> kludge or the live/real ruleset? Check for presence of ALT attribute?
Yes, by presence check. iptables-nft may default to ALT_EXPRESSIONS or
parse both into iptables_command_state objects for comparison.
> > - nla_nest_end(skb, list);
> >
> > if (rule->udata) {
> > struct nft_userdata *udata = nft_userdata(rule);
> > @@ -3366,6 +3385,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
> > nf_tables_expr_destroy(ctx, expr);
> > expr = next;
> > }
> > + kfree(rule->alt_expr);
> > kfree(rule);
> > }
> >
> > @@ -3443,6 +3463,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
> > struct nft_rule *rule, *old_rule = NULL;
> > struct nft_expr_info *expr_info = NULL;
> > u8 family = info->nfmsg->nfgen_family;
> > + struct nft_alt_expr *alt_expr = NULL;
> > struct nft_flow_rule *flow = NULL;
> > struct net *net = info->net;
> > struct nft_userdata *udata;
> > @@ -3556,6 +3577,19 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
> > if (size >= 1 << 12)
> > goto err_release_expr;
> >
> > + if (nla[NFTA_RULE_ALT_EXPRESSIONS]) {
> > + int dlen = nla_len(nla[NFTA_RULE_ALT_EXPRESSIONS]);
> > + alt_expr = kvmalloc(sizeof(*alt_expr) + dlen, GFP_KERNEL);
>
> Once nla_policy provides a maxlen this is fine.
Hmm. I could define this max length as
| NFT_RULE_MAXEXPRS * max(nft_expr_ops::size)
given the currently existing nft_expr_ops instances in kernel. This
isn't pretty accurate though. Or I do a "fake" parsing of the nested
expression list only checking the number of elements? IIUC, this is the
only restriction applied to NFTA_RULE_EXPRESSIONS - at least I don't see
the size of each NFTA_LIST_ELEM being checked against its
nft_expr_ops::size. Or am I missing something?
> > + nla_memcpy(alt_expr->data,
> > + nla[NFTA_RULE_ALT_EXPRESSIONS], dlen);
>
> Hmm, I wonder if this isn't a problem.
> The kernel will now dump arbitrary data to userspace, whereas without
> this patch the nla data is generated by kernel, i.e. never malformed.
>
> I wonder if the alt blob needs to go through some type of validation
> too?
Given that the kernel doesn't use the ALT data, I liked the fact that it
may contain expressions it doesn't even support anymore. Upholding this
allows to disable nft_compat in kernel once iptables-nft produces native
expressions for everything.
I get your point about dumping tainted data. Does a shallow syntactic
check suffice, is it required to verify each expression's syntax or even
its semantics?
> Also I think that this attribute should always be ignored for
> NFT_COMPAT=n builds, we should document this kludge is only for
> iptables-nft sake (or rather, incorrect a**umptions by 3rd
> party wrappers of iptables userspace...).
Fine with me. For now, iptables-nft is not usable without NFT_COMPAT
anyway. If so, we could still relax this into ALT_EXPRESSIONS but no
compat interface itself.
Thanks, Phil
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Introduce NFTA_RULE_ALT_EXPRESSIONS
2022-12-16 12:36 ` Phil Sutter
@ 2022-12-16 13:26 ` Florian Westphal
0 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2022-12-16 13:26 UTC (permalink / raw)
To: Phil Sutter, Florian Westphal, Pablo Neira Ayuso, netfilter-devel
Phil Sutter <phil@nwl.cc> wrote:
> > Or at least mention that this is for iptables-nft/NFT_COMPAT sake?
>
> I could move it into NFTA_RULE_COMPAT.
Fine with me.
> > Perhaps its better to use two distinct attributes?
> >
> > NFTA_RULE_EXPRESSIONS_COMPAT (input)
> > NFTA_RULE_EXPRESSIONS_ACTUAL (output)?
>
> With NFTA_RULE_EXPRESSIONS_ACTUAL replacing NFTA_RULE_EXPRESSIONS?
Yes, NFTA_RULE_EXPRESSIONS is the 'compat kludge',
NFTA_RULE_EXPRESSIONS_ACTUAL the active one.
> > The switcheroo of ALT (old crap on input, actual live ruleset on output)
> > is very unintuitive.
>
> Well, providing something that replaces the content of
> NFTA_RULE_EXPRESSIONS is the basic concept of this approach. Restricting
> it to output only is not possible since old user space uses it for
> input. I guess one could make new user space use the *_COMPAT/*_ACTUAL
> attributes above but kernel still has to fall back to plain
> *_EXPRESSIONS.
Yep.
> Adding the two attributes above and using *_COMPAT for input only and
> *_ACTUAL for output only is probably a trivial change though.
Right. I don't mind but its perhaps a bit clearer as to what is going
on.
> [...]
> > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > > index cfa844da1ce61..2dff92f527429 100644
> > > --- a/include/uapi/linux/netfilter/nf_tables.h
> > > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > > @@ -247,6 +247,8 @@ enum nft_chain_attributes {
> > > * @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
> > > * @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
> > > * @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule (NLA_U32)
> > > + * @NFTA_RULE_CHAIN_ID: add the rule to chain by ID, alternative to @NFTA_RULE_CHAIN (NLA_U32)
> > > + * @NFTA_RULE_ALT_EXPRESSIONS: expressions to swap with @NFTA_RULE_EXPRESSIONS for dumps (NLA_NESTED: nft_expr_attributes)
> > > */
> > > enum nft_rule_attributes {
> > > NFTA_RULE_UNSPEC,
> > > @@ -261,6 +263,7 @@ enum nft_rule_attributes {
> > > NFTA_RULE_ID,
> > > NFTA_RULE_POSITION_ID,
> > > NFTA_RULE_CHAIN_ID,
> > > + NFTA_RULE_ALT_EXPRESSIONS,
> >
> > You need to update the nla_policy too.
>
> ACK, thanks!
[..]
> > > + if (nla[NFTA_RULE_ALT_EXPRESSIONS]) {
> > > + int dlen = nla_len(nla[NFTA_RULE_ALT_EXPRESSIONS]);
> > > + alt_expr = kvmalloc(sizeof(*alt_expr) + dlen, GFP_KERNEL);
> >
> > Once nla_policy provides a maxlen this is fine.
>
> Hmm. I could define this max length as
> | NFT_RULE_MAXEXPRS * max(nft_expr_ops::size)
> given the currently existing nft_expr_ops instances in kernel. This
> isn't pretty accurate though. Or I do a "fake" parsing of the nested
> expression list only checking the number of elements? IIUC, this is the
> only restriction applied to NFTA_RULE_EXPRESSIONS - at least I don't see
> the size of each NFTA_LIST_ELEM being checked against its
> nft_expr_ops::size. Or am I missing something?
Actually you don't need a maxlen at all, sorry. I saw the nla_len and
thought NLA_BINARY but you could do this:
- [NFTA_RULE_EXPRESSIONS] = { .type = NLA_NESTED },
+ [NFTA_RULE_EXPRESSIONS] = NLA_POLICY_NESTED_ARRAY(nft_expr_policy),
... and use the same for the COMPAT_EXPRESSIONS (or whatever name
will be chosen).
(This won't do full validation of all attributes because the policy
for expr 'X' isn't known at compile time).
> > > + nla_memcpy(alt_expr->data,
> > > + nla[NFTA_RULE_ALT_EXPRESSIONS], dlen);
> >
> > Hmm, I wonder if this isn't a problem.
> > The kernel will now dump arbitrary data to userspace, whereas without
> > this patch the nla data is generated by kernel, i.e. never malformed.
> >
> > I wonder if the alt blob needs to go through some type of validation
> > too?
>
> Given that the kernel doesn't use the ALT data, I liked the fact that it
> may contain expressions it doesn't even support anymore. Upholding this
> allows to disable nft_compat in kernel once iptables-nft produces native
> expressions for everything.
>
> I get your point about dumping tainted data. Does a shallow syntactic
> check suffice, is it required to verify each expression's syntax or even
> its semantics?
I was just wondering if we should at least verify that netlink
attributes and sizes match, not that the expressions themselves are
parseable/available.
But its impossible to do so if the expression isn't available because
we then have no policy either.
So perhaps above NLA_POLICY_NESTED_ARRAY() is enough after all.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-16 13:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-15 20:43 [nf-next PATCH] netfilter: nf_tables: Introduce NFTA_RULE_ALT_EXPRESSIONS Phil Sutter
2022-12-16 1:16 ` Florian Westphal
2022-12-16 12:36 ` Phil Sutter
2022-12-16 13:26 ` Florian Westphal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.