Re: [PATCH nft] segtree: fix string data initialisation

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Mar 05, 2025 at 04:01:48PM +0100, Florian Westphal wrote:
> > This uses the wrong length.  This must re-use the length of the datatype,
> > not the string length.
> > 
> > The added test cases will fail without the fix due to erroneous
> > overlap detection, which in itself is due to incorrect sorting of
> > the elements.
> > 
> > Example error:
> >  netlink: Error: interval overlaps with an existing one
> >  add element inet testifsets simple_wild {  "2-1" } failed.
> >  table inet testifsets {
> >       ...       elements = { "1-1", "abcdef*", "othername", "ppp0" }
> > 
> > ... but clearly "2-1" doesn't overlap with any existing members.
> > The false detection is because of the "acvdef*" wildcard getting sorted
> > at the beginning of the list which is because its erronously initialised
> > as a 64bit number instead of 128 bits (16 bytes / IFNAMSIZ).
> 
> One question here.
> 
> > Fixes: 5e393ea1fc0a ("segtree: add string "range" reversal support")
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  src/segtree.c                                |  2 +-
> >  tests/shell/testcases/sets/sets_with_ifnames | 62 ++++++++++++++++++++
> >  2 files changed, 63 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/segtree.c b/src/segtree.c
> > index 2e32a3291979..11cf27c55dcb 100644
> > --- a/src/segtree.c
> > +++ b/src/segtree.c
> > @@ -471,7 +471,7 @@ static struct expr *interval_to_string(struct expr *low, struct expr *i, const m
> >  
> >  	expr = constant_expr_alloc(&low->location, low->dtype,
> >  				   BYTEORDER_HOST_ENDIAN,
> > -				   (str_len + 1) * BITS_PER_BYTE, data);
> > +				   len * BITS_PER_BYTE, data);
> 
> BTW, is this also needed?
> 
> diff --git a/src/segtree.c b/src/segtree.c
> index 2e32a3291979..b7a89383fae0 100644
> --- a/src/segtree.c
> +++ b/src/segtree.c
> @@ -453,7 +453,7 @@ static struct expr *interval_to_string(struct expr *low, struct expr *i, const m
>  {
>         unsigned int len = div_round_up(i->len, BITS_PER_BYTE);
>         unsigned int prefix_len, str_len;
> -       char data[len + 2];
> +       char data[len + 2] = {};
>         struct expr *expr;
>  
>         prefix_len = expr_value(i)->len - mpz_scan0(range, 0);
> 
> otherwise uninitialized data could be send to the kernel?

No, I don't think so, data is filled with len bytes:

       mpz_export_data(data, expr_value(low)->value, BYTEORDER_BIG_ENDIAN, len);

So I don't see the reimport to fetch anything that was onstack garbage.