Re: [PATCH nf] netfilter: nft_set_pipapo: reject mismatched sum of field_len with key length

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Jan 28, 2025 at 01:30:12AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > The field length description provides the length of each separated key
> > fields in the concatenation. The set key length provides the total size
> > of the key aligned to 32-bits for the pipapo set backend. Reject with
> > EINVAL if the field length description and set key length provided by
> > userspace are inconsistent.
> > 
> > Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nft_set_pipapo.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> > index 7be342b495f5..3b1a53e68989 100644
> > --- a/net/netfilter/nft_set_pipapo.c
> > +++ b/net/netfilter/nft_set_pipapo.c
> > @@ -2235,6 +2235,7 @@ static int nft_pipapo_init(const struct nft_set *set,
> >  	struct nft_pipapo_match *m;
> >  	struct nft_pipapo_field *f;
> >  	int err, i, field_count;
> > +	unsigned int len = 0;
> >  
> >  	BUILD_BUG_ON(offsetof(struct nft_pipapo_elem, priv) != 0);
> >  
> > @@ -2246,6 +2247,12 @@ static int nft_pipapo_init(const struct nft_set *set,
> >  	if (field_count > NFT_PIPAPO_MAX_FIELDS)
> >  		return -EINVAL;
> >  
> > +	for (i = 0; i < field_count; i++)
> > +		len += round_up(desc->field_len[i], sizeof(u32));
> > +
> > +	if (len != set->klen)
> > +		return -EINVAL;
> > +
> 
> I fail to grasp why nft_set_desc_concat() doesn't catch it:
> 
>         for (i = 0; i < desc->field_count; i++)
>                 num_regs += DIV_ROUND_UP(desc->field_len[i], sizeof(u32));
> 
>         key_num_regs = DIV_ROUND_UP(desc->klen, sizeof(u32));
>         if (key_num_regs != num_regs);	----> here....
>                 return -EINVAL;

This check is loose, I will post a v2 fixing up nft_set_desc_concat().

Thanks.