| Message ID | 20231213190222.3681-2-phil@nwl.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [libnftnl,RFC,1/2] expr: Repurpose struct expr_ops::max_attr field | expand |
Phil Sutter <phil@nwl.cc> wrote: > Similar to kernel's nla_policy, enable expressions to inform about > restrictions on attribute use. This allows the generic expression code > to perform sanity checks before dispatching to expression ops. > > For now, this holds only the maximum data len which may be passed to > nftnl_expr_set(). > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > include/expr_ops.h | 5 +++++ > src/expr.c | 6 ++++++ > src/expr/bitwise.c | 11 +++++++++++ > src/expr/byteorder.c | 9 +++++++++ > src/expr/immediate.c | 9 +++++++++ > 5 files changed, 40 insertions(+) > > diff --git a/include/expr_ops.h b/include/expr_ops.h > index 51b221483552c..6c95297bfcd58 100644 > --- a/include/expr_ops.h > +++ b/include/expr_ops.h > @@ -8,10 +8,15 @@ struct nlattr; > struct nlmsghdr; > struct nftnl_expr; > > +struct attr_policy { > + size_t maxlen; I'd make this an uint32_t since that is also whats passed to expr_set(). > + if (expr->ops->attr_policy && > + type <= expr->ops->nftnl_max_attr && > + expr->ops->attr_policy[type].maxlen && > + expr->ops->attr_policy[type].maxlen < data_len) > + return -1; > + I'd make this more strict, is there a reason to call ops->set if type > ->nftnl_max_attr? Something like: !attr_policy -> return -1; type > nftnl_max_attr -> return -1: data_len > maxlen -> return -1. We could also define a minlen to disallow setting e.g. 1 byte of something that is internally an u32. But I admit that this might break compatibility or otherwise leak internals into the api.
On Wed, Dec 13, 2023 at 09:20:35PM +0100, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > Similar to kernel's nla_policy, enable expressions to inform about > > restrictions on attribute use. This allows the generic expression code > > to perform sanity checks before dispatching to expression ops. > > > > For now, this holds only the maximum data len which may be passed to > > nftnl_expr_set(). > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > include/expr_ops.h | 5 +++++ > > src/expr.c | 6 ++++++ > > src/expr/bitwise.c | 11 +++++++++++ > > src/expr/byteorder.c | 9 +++++++++ > > src/expr/immediate.c | 9 +++++++++ > > 5 files changed, 40 insertions(+) > > > > diff --git a/include/expr_ops.h b/include/expr_ops.h > > index 51b221483552c..6c95297bfcd58 100644 > > --- a/include/expr_ops.h > > +++ b/include/expr_ops.h > > @@ -8,10 +8,15 @@ struct nlattr; > > struct nlmsghdr; > > struct nftnl_expr; > > > > +struct attr_policy { > > + size_t maxlen; > > I'd make this an uint32_t since that is also whats > passed to expr_set(). ACK. > > + if (expr->ops->attr_policy && > > + type <= expr->ops->nftnl_max_attr && > > + expr->ops->attr_policy[type].maxlen && > > + expr->ops->attr_policy[type].maxlen < data_len) > > + return -1; > > + > > I'd make this more strict, is there a reason to call ops->set > if type > ->nftnl_max_attr? Indeed. We might even drop the default case from expressions' set callbacks, if we assert type >= NFTNL_EXPR_BASE, too. > Something like: > > !attr_policy -> return -1; Which means I can't procrastinate writing the policies for all 40 expressions. ;) > type > nftnl_max_attr -> return -1: > data_len > maxlen -> return -1. I wanted to make this an opt-in approach, so I'd rather go with maxlen && maxlen < data_len -> return -1. > We could also define a minlen to disallow setting > e.g. 1 byte of something that is internally an u32. This at least may easily be added later, or now but with the default minlen of 0 for most attributes. > But I admit that this might break compatibility > or otherwise leak internals into the api. It's not entirely straightforward, anyway. NFTNL_EXPR_IMM_CHAIN for instance accepts literally anything as long as it's a NUL-terminated string. I was unsure whether libnftnl should enforce NFT_CHAIN_MAXNAMELEN there or not. Thanks, Phil
Phil Sutter <phil@nwl.cc> wrote: > > Something like: > > > > !attr_policy -> return -1; > > Which means I can't procrastinate writing the policies for all 40 > expressions. ;) Oh, right. > > type > nftnl_max_attr -> return -1: > > data_len > maxlen -> return -1. > > I wanted to make this an opt-in approach, so I'd rather go with > maxlen && maxlen < data_len -> return -1. Alright, it could be made more strict later once all have been converted. > > But I admit that this might break compatibility > > or otherwise leak internals into the api. > > It's not entirely straightforward, anyway. NFTNL_EXPR_IMM_CHAIN for > instance accepts literally anything as long as it's a NUL-terminated > string. I was unsure whether libnftnl should enforce > NFT_CHAIN_MAXNAMELEN there or not. Good point. ATM libnftnl and nftables are more tightly coupled, so I don't see why enforcing NFT_CHAIN_MAXNAMELEN would bite us at some point. We could also cap to 65528 which is what netlink would permit (nla attr header plus playload).
diff --git a/include/expr_ops.h b/include/expr_ops.h index 51b221483552c..6c95297bfcd58 100644 --- a/include/expr_ops.h +++ b/include/expr_ops.h @@ -8,10 +8,15 @@ struct nlattr; struct nlmsghdr; struct nftnl_expr; +struct attr_policy { + size_t maxlen; +}; + struct expr_ops { const char *name; uint32_t alloc_len; int nftnl_max_attr; + struct attr_policy *attr_policy; void (*init)(const struct nftnl_expr *e); void (*free)(const struct nftnl_expr *e); int (*set)(struct nftnl_expr *e, uint16_t type, const void *data, uint32_t data_len); diff --git a/src/expr.c b/src/expr.c index b4581f1a79ff6..3e06c48aeb5f3 100644 --- a/src/expr.c +++ b/src/expr.c @@ -71,6 +71,12 @@ int nftnl_expr_set(struct nftnl_expr *expr, uint16_t type, case NFTNL_EXPR_NAME: /* cannot be modified */ return 0; default: + if (expr->ops->attr_policy && + type <= expr->ops->nftnl_max_attr && + expr->ops->attr_policy[type].maxlen && + expr->ops->attr_policy[type].maxlen < data_len) + return -1; + if (expr->ops->set(expr, type, data, data_len) < 0) return -1; } diff --git a/src/expr/bitwise.c b/src/expr/bitwise.c index d11272fbd37b0..0a881eb3545dd 100644 --- a/src/expr/bitwise.c +++ b/src/expr/bitwise.c @@ -274,10 +274,21 @@ nftnl_expr_bitwise_snprintf(char *buf, size_t size, return err; } +static struct attr_policy bitwise_attr_policy[__NFTNL_EXPR_BITWISE_MAX] = { + [NFTNL_EXPR_BITWISE_SREG] = { .maxlen = sizeof(enum nft_registers) }, + [NFTNL_EXPR_BITWISE_DREG] = { .maxlen = sizeof(enum nft_registers) }, + [NFTNL_EXPR_BITWISE_LEN] = { .maxlen = sizeof(uint8_t) }, + [NFTNL_EXPR_BITWISE_MASK] = { .maxlen = NFT_DATA_VALUE_MAXLEN }, + [NFTNL_EXPR_BITWISE_XOR] = { .maxlen = NFT_DATA_VALUE_MAXLEN }, + [NFTNL_EXPR_BITWISE_OP] = { .maxlen = sizeof(enum nft_bitwise_ops) }, + [NFTNL_EXPR_BITWISE_DATA] = { .maxlen = NFT_DATA_VALUE_MAXLEN }, +}; + struct expr_ops expr_ops_bitwise = { .name = "bitwise", .alloc_len = sizeof(struct nftnl_expr_bitwise), .nftnl_max_attr = __NFTNL_EXPR_BITWISE_MAX - 1, + .attr_policy = bitwise_attr_policy, .set = nftnl_expr_bitwise_set, .get = nftnl_expr_bitwise_get, .parse = nftnl_expr_bitwise_parse, diff --git a/src/expr/byteorder.c b/src/expr/byteorder.c index f05ae59b688eb..6d085197b40c7 100644 --- a/src/expr/byteorder.c +++ b/src/expr/byteorder.c @@ -212,10 +212,19 @@ nftnl_expr_byteorder_snprintf(char *buf, size_t remain, return offset; } +static struct attr_policy byteorder_attr_policy[__NFTNL_EXPR_BYTEORDER_MAX] = { + [NFTNL_EXPR_BYTEORDER_DREG] = { .maxlen = sizeof(enum nft_registers) }, + [NFTNL_EXPR_BYTEORDER_SREG] = { .maxlen = sizeof(enum nft_registers) }, + [NFTNL_EXPR_BYTEORDER_OP] = { .maxlen = sizeof(enum nft_byteorder_ops) }, + [NFTNL_EXPR_BYTEORDER_LEN] = { .maxlen = sizeof(uint8_t) }, + [NFTNL_EXPR_BYTEORDER_SIZE] = { .maxlen = sizeof(uint8_t) }, +}; + struct expr_ops expr_ops_byteorder = { .name = "byteorder", .alloc_len = sizeof(struct nftnl_expr_byteorder), .nftnl_max_attr = __NFTNL_EXPR_BYTEORDER_MAX - 1, + .attr_policy = byteorder_attr_policy, .set = nftnl_expr_byteorder_set, .get = nftnl_expr_byteorder_get, .parse = nftnl_expr_byteorder_parse, diff --git a/src/expr/immediate.c b/src/expr/immediate.c index 2ffa0c5e9ebc3..774ed3dcfea7e 100644 --- a/src/expr/immediate.c +++ b/src/expr/immediate.c @@ -220,10 +220,19 @@ static void nftnl_expr_immediate_free(const struct nftnl_expr *e) nftnl_free_verdict(&imm->data); } +static struct attr_policy immediate_attr_policy[__NFTNL_EXPR_IMM_MAX] = { + [NFTNL_EXPR_IMM_DREG] = { .maxlen = sizeof(enum nft_registers) }, + [NFTNL_EXPR_IMM_DATA] = { .maxlen = NFT_DATA_VALUE_MAXLEN }, + [NFTNL_EXPR_IMM_VERDICT] = { .maxlen = sizeof(uint32_t) }, + /*[NFTNL_EXPR_IMM_CHAIN] = { .maxlen = NFT_CHAIN_MAXNAMELEN },*/ + [NFTNL_EXPR_IMM_CHAIN_ID] = { .maxlen = sizeof(uint32_t) }, +}; + struct expr_ops expr_ops_immediate = { .name = "immediate", .alloc_len = sizeof(struct nftnl_expr_immediate), .nftnl_max_attr = __NFTNL_EXPR_IMM_MAX - 1, + .attr_policy = immediate_attr_policy, .free = nftnl_expr_immediate_free, .set = nftnl_expr_immediate_set, .get = nftnl_expr_immediate_get,
Similar to kernel's nla_policy, enable expressions to inform about restrictions on attribute use. This allows the generic expression code to perform sanity checks before dispatching to expression ops. For now, this holds only the maximum data len which may be passed to nftnl_expr_set(). Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/expr_ops.h | 5 +++++ src/expr.c | 6 ++++++ src/expr/bitwise.c | 11 +++++++++++ src/expr/byteorder.c | 9 +++++++++ src/expr/immediate.c | 9 +++++++++ 5 files changed, 40 insertions(+)