On Wed, Dec 13, 2023 at 09:20:35PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> 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@xxxxxx>
> > ---
> > 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
