* [PATCH nft] segtree: fix string data initialisation
@ 2025-03-05 15:01 Florian Westphal
2025-03-05 21:47 ` Pablo Neira Ayuso
2025-03-05 22:12 ` Pablo Neira Ayuso
0 siblings, 2 replies; 4+ messages in thread
From: Florian Westphal @ 2025-03-05 15:01 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
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).
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);
return __expr_to_set_elem(low, expr);
}
diff --git a/tests/shell/testcases/sets/sets_with_ifnames b/tests/shell/testcases/sets/sets_with_ifnames
index a4bc5072938e..c65499b76bc5 100755
--- a/tests/shell/testcases/sets/sets_with_ifnames
+++ b/tests/shell/testcases/sets/sets_with_ifnames
@@ -105,10 +105,67 @@ check_matching_icmp_ppp()
fi
}
+check_add_del_ifnames()
+{
+ local what="$1"
+ local setname="$2"
+ local prefix="$3"
+ local data="$4"
+ local i=0
+
+ for i in $(seq 1 5);do
+ local cmd="element inet testifsets $setname { "
+ local to_batch=16
+
+ for j in $(seq 1 $to_batch);do
+ local name=$(printf '"%x-%d"' $i $j)
+
+ [ -n "$prefix" ] && cmd="$cmd $prefix . "
+
+ cmd="$cmd $name"
+
+ [ -n "$data" ] && cmd="$cmd : $data"
+
+ if [ $j -lt $to_batch ] ; then
+ cmd="$cmd, "
+ fi
+ done
+
+ cmd="$cmd }"
+
+ if ! $NFT "$what" "$cmd"; then
+ echo "$what $cmd failed."
+ $NFT list set inet testifsets $setname
+ exit 1
+ fi
+
+ if ! ip netns exec "$ns1" $NFT "$what" "$cmd"; then
+ echo "$ns1 $what $cmd failed."
+ ip netns exec "$ns1" $NFT list set inet testifsets $setname
+ exit 1
+ fi
+ done
+}
+
+check_add_ifnames()
+{
+ check_add_del_ifnames "add" "$1" "$2" "$3"
+}
+
+check_del_ifnames()
+{
+ check_add_del_ifnames "delete" "$1" "$2" "$3"
+}
+
ip netns add "$ns1" || exit 111
ip netns add "$ns2" || exit 111
ip netns exec "$ns1" $NFT -f "$dumpfile" || exit 3
+check_add_ifnames "simple" "" ""
+check_add_ifnames "simple_wild" "" ""
+check_add_ifnames "concat" "10.1.2.2" ""
+check_add_ifnames "map_wild" "" "drop"
+
for n in abcdef0 abcdef1 othername;do
check_elem simple $n
done
@@ -150,3 +207,8 @@ ip -net "$ns2" addr add 10.1.2.2/24 dev veth0
ip -net "$ns2" addr add 10.2.2.2/24 dev veth1
check_matching_icmp_ppp
+
+check_del_ifnames "simple" "" ""
+check_del_ifnames "simple_wild" "" ""
+check_del_ifnames "concat" "10.1.2.2" ""
+check_del_ifnames "map_wild" "" "drop"
--
2.48.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH nft] segtree: fix string data initialisation
2025-03-05 15:01 [PATCH nft] segtree: fix string data initialisation Florian Westphal
@ 2025-03-05 21:47 ` Pablo Neira Ayuso
2025-03-05 22:12 ` Pablo Neira Ayuso
1 sibling, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2025-03-05 21:47 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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).
>
> Fixes: 5e393ea1fc0a ("segtree: add string "range" reversal support")
> Signed-off-by: Florian Westphal <fw@strlen.de>
Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> 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);
>
> return __expr_to_set_elem(low, expr);
> }
> diff --git a/tests/shell/testcases/sets/sets_with_ifnames b/tests/shell/testcases/sets/sets_with_ifnames
> index a4bc5072938e..c65499b76bc5 100755
> --- a/tests/shell/testcases/sets/sets_with_ifnames
> +++ b/tests/shell/testcases/sets/sets_with_ifnames
> @@ -105,10 +105,67 @@ check_matching_icmp_ppp()
> fi
> }
>
> +check_add_del_ifnames()
> +{
> + local what="$1"
> + local setname="$2"
> + local prefix="$3"
> + local data="$4"
> + local i=0
> +
> + for i in $(seq 1 5);do
> + local cmd="element inet testifsets $setname { "
> + local to_batch=16
> +
> + for j in $(seq 1 $to_batch);do
> + local name=$(printf '"%x-%d"' $i $j)
> +
> + [ -n "$prefix" ] && cmd="$cmd $prefix . "
> +
> + cmd="$cmd $name"
> +
> + [ -n "$data" ] && cmd="$cmd : $data"
> +
> + if [ $j -lt $to_batch ] ; then
> + cmd="$cmd, "
> + fi
> + done
> +
> + cmd="$cmd }"
> +
> + if ! $NFT "$what" "$cmd"; then
> + echo "$what $cmd failed."
> + $NFT list set inet testifsets $setname
> + exit 1
> + fi
> +
> + if ! ip netns exec "$ns1" $NFT "$what" "$cmd"; then
> + echo "$ns1 $what $cmd failed."
> + ip netns exec "$ns1" $NFT list set inet testifsets $setname
> + exit 1
> + fi
> + done
> +}
> +
> +check_add_ifnames()
> +{
> + check_add_del_ifnames "add" "$1" "$2" "$3"
> +}
> +
> +check_del_ifnames()
> +{
> + check_add_del_ifnames "delete" "$1" "$2" "$3"
> +}
> +
> ip netns add "$ns1" || exit 111
> ip netns add "$ns2" || exit 111
> ip netns exec "$ns1" $NFT -f "$dumpfile" || exit 3
>
> +check_add_ifnames "simple" "" ""
> +check_add_ifnames "simple_wild" "" ""
> +check_add_ifnames "concat" "10.1.2.2" ""
> +check_add_ifnames "map_wild" "" "drop"
> +
> for n in abcdef0 abcdef1 othername;do
> check_elem simple $n
> done
> @@ -150,3 +207,8 @@ ip -net "$ns2" addr add 10.1.2.2/24 dev veth0
> ip -net "$ns2" addr add 10.2.2.2/24 dev veth1
>
> check_matching_icmp_ppp
> +
> +check_del_ifnames "simple" "" ""
> +check_del_ifnames "simple_wild" "" ""
> +check_del_ifnames "concat" "10.1.2.2" ""
> +check_del_ifnames "map_wild" "" "drop"
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH nft] segtree: fix string data initialisation
2025-03-05 15:01 [PATCH nft] segtree: fix string data initialisation Florian Westphal
2025-03-05 21:47 ` Pablo Neira Ayuso
@ 2025-03-05 22:12 ` Pablo Neira Ayuso
2025-03-06 3:52 ` Florian Westphal
1 sibling, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2025-03-05 22:12 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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?
> return __expr_to_set_elem(low, expr);
> }
> diff --git a/tests/shell/testcases/sets/sets_with_ifnames b/tests/shell/testcases/sets/sets_with_ifnames
> index a4bc5072938e..c65499b76bc5 100755
> --- a/tests/shell/testcases/sets/sets_with_ifnames
> +++ b/tests/shell/testcases/sets/sets_with_ifnames
> @@ -105,10 +105,67 @@ check_matching_icmp_ppp()
> fi
> }
>
> +check_add_del_ifnames()
> +{
> + local what="$1"
> + local setname="$2"
> + local prefix="$3"
> + local data="$4"
> + local i=0
> +
> + for i in $(seq 1 5);do
> + local cmd="element inet testifsets $setname { "
> + local to_batch=16
> +
> + for j in $(seq 1 $to_batch);do
> + local name=$(printf '"%x-%d"' $i $j)
> +
> + [ -n "$prefix" ] && cmd="$cmd $prefix . "
> +
> + cmd="$cmd $name"
> +
> + [ -n "$data" ] && cmd="$cmd : $data"
> +
> + if [ $j -lt $to_batch ] ; then
> + cmd="$cmd, "
> + fi
> + done
> +
> + cmd="$cmd }"
> +
> + if ! $NFT "$what" "$cmd"; then
> + echo "$what $cmd failed."
> + $NFT list set inet testifsets $setname
> + exit 1
> + fi
> +
> + if ! ip netns exec "$ns1" $NFT "$what" "$cmd"; then
> + echo "$ns1 $what $cmd failed."
> + ip netns exec "$ns1" $NFT list set inet testifsets $setname
> + exit 1
> + fi
> + done
> +}
> +
> +check_add_ifnames()
> +{
> + check_add_del_ifnames "add" "$1" "$2" "$3"
> +}
> +
> +check_del_ifnames()
> +{
> + check_add_del_ifnames "delete" "$1" "$2" "$3"
> +}
> +
> ip netns add "$ns1" || exit 111
> ip netns add "$ns2" || exit 111
> ip netns exec "$ns1" $NFT -f "$dumpfile" || exit 3
>
> +check_add_ifnames "simple" "" ""
> +check_add_ifnames "simple_wild" "" ""
> +check_add_ifnames "concat" "10.1.2.2" ""
> +check_add_ifnames "map_wild" "" "drop"
> +
> for n in abcdef0 abcdef1 othername;do
> check_elem simple $n
> done
> @@ -150,3 +207,8 @@ ip -net "$ns2" addr add 10.1.2.2/24 dev veth0
> ip -net "$ns2" addr add 10.2.2.2/24 dev veth1
>
> check_matching_icmp_ppp
> +
> +check_del_ifnames "simple" "" ""
> +check_del_ifnames "simple_wild" "" ""
> +check_del_ifnames "concat" "10.1.2.2" ""
> +check_del_ifnames "map_wild" "" "drop"
> --
> 2.48.1
>
>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH nft] segtree: fix string data initialisation
2025-03-05 22:12 ` Pablo Neira Ayuso
@ 2025-03-06 3:52 ` Florian Westphal
0 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2025-03-06 3:52 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-03-06 3:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-05 15:01 [PATCH nft] segtree: fix string data initialisation Florian Westphal
2025-03-05 21:47 ` Pablo Neira Ayuso
2025-03-05 22:12 ` Pablo Neira Ayuso
2025-03-06 3:52 ` Florian Westphal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).