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

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

[Pablo Neira Ayuso]

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;
+
 	m = kmalloc(struct_size(m, f, field_count), GFP_KERNEL);
 	if (!m)
 		return -ENOMEM;
-- 
2.30.2

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

[Florian Westphal]

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;

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

[Pablo Neira Ayuso]

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.