[PATCH iptables] extensions: libebt_stp: fix range checking

[PATCH iptables] extensions: libebt_stp: fix range checking

[Florian Westphal]

This has to either consider ->nvals > 1 or check the values
post-no-range-fixup:

./iptables-test.py  extensions/libebt_stp.t
extensions/libebt_stp.t: ERROR: line 12 (cannot load: ebtables -A INPUT --stp-root-cost 1)

(it tests 0 < 1 and fails, but test should be 1 < 1).

Fixes: dc6efcfeac38 ("extensions: libebt_stp: Use guided option parser")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 extensions/libebt_stp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/libebt_stp.c b/extensions/libebt_stp.c
index 81054b26c1f0..371fa04c870f 100644
--- a/extensions/libebt_stp.c
+++ b/extensions/libebt_stp.c
@@ -142,7 +142,7 @@ static void brstp_parse(struct xt_option_call *cb)
 #define RANGE_ASSIGN(name, fname, val) {				    \
 		stpinfo->config.fname##l = val[0];			    \
 		stpinfo->config.fname##u = cb->nvals > 1 ? val[1] : val[0]; \
-		if (val[1] < val[0])					    \
+		if (stpinfo->config.fname##u < stpinfo->config.fname##l)    \
 			xtables_error(PARAMETER_PROBLEM,		    \
 				      "Bad --stp-" name " range");	    \
 }
-- 
2.43.0

Re: [PATCH iptables] extensions: libebt_stp: fix range checking

[Phil Sutter]

Hi Florian,

On Tue, Jan 23, 2024 at 05:49:33PM +0100, Florian Westphal wrote:
> This has to either consider ->nvals > 1 or check the values
> post-no-range-fixup:
> 
> ./iptables-test.py  extensions/libebt_stp.t
> extensions/libebt_stp.t: ERROR: line 12 (cannot load: ebtables -A INPUT --stp-root-cost 1)
> 
> (it tests 0 < 1 and fails, but test should be 1 < 1).
> 
> Fixes: dc6efcfeac38 ("extensions: libebt_stp: Use guided option parser")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  extensions/libebt_stp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/extensions/libebt_stp.c b/extensions/libebt_stp.c
> index 81054b26c1f0..371fa04c870f 100644
> --- a/extensions/libebt_stp.c
> +++ b/extensions/libebt_stp.c
> @@ -142,7 +142,7 @@ static void brstp_parse(struct xt_option_call *cb)
>  #define RANGE_ASSIGN(name, fname, val) {				    \
>  		stpinfo->config.fname##l = val[0];			    \
>  		stpinfo->config.fname##u = cb->nvals > 1 ? val[1] : val[0]; \
> -		if (val[1] < val[0])					    \
> +		if (stpinfo->config.fname##u < stpinfo->config.fname##l)    \
>  			xtables_error(PARAMETER_PROBLEM,		    \
>  				      "Bad --stp-" name " range");	    \
>  }

This is odd: xtopt_parse_mint() assigns UINT32_MAX to val[1] for
XTTYPE_UINT32RC if no upper end is given. Also, extensions/libebt_stp.t
passes for me. What's missing on my end?

Cheers, Phil

Re: [PATCH iptables] extensions: libebt_stp: fix range checking

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Tue, Jan 23, 2024 at 05:49:33PM +0100, Florian Westphal wrote:
> > This has to either consider ->nvals > 1 or check the values
> > post-no-range-fixup:
> > 
> > ./iptables-test.py  extensions/libebt_stp.t
> > extensions/libebt_stp.t: ERROR: line 12 (cannot load: ebtables -A INPUT --stp-root-cost 1)
> > 
> > (it tests 0 < 1 and fails, but test should be 1 < 1).
> > 
> > Fixes: dc6efcfeac38 ("extensions: libebt_stp: Use guided option parser")
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  extensions/libebt_stp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/extensions/libebt_stp.c b/extensions/libebt_stp.c
> > index 81054b26c1f0..371fa04c870f 100644
> > --- a/extensions/libebt_stp.c
> > +++ b/extensions/libebt_stp.c
> > @@ -142,7 +142,7 @@ static void brstp_parse(struct xt_option_call *cb)
> >  #define RANGE_ASSIGN(name, fname, val) {				    \
> >  		stpinfo->config.fname##l = val[0];			    \
> >  		stpinfo->config.fname##u = cb->nvals > 1 ? val[1] : val[0]; \
> > -		if (val[1] < val[0])					    \
> > +		if (stpinfo->config.fname##u < stpinfo->config.fname##l)    \
> >  			xtables_error(PARAMETER_PROBLEM,		    \
> >  				      "Bad --stp-" name " range");	    \
> >  }
> 
> This is odd: xtopt_parse_mint() assigns UINT32_MAX to val[1] for
> XTTYPE_UINT32RC if no upper end is given. Also, extensions/libebt_stp.t
> passes for me. What's missing on my end?

No fukcing clue. did git clean -d -f -x and it works now without the
patch.

I hate my life.

Re: [PATCH iptables] extensions: libebt_stp: fix range checking

[Phil Sutter]

On Wed, Jan 24, 2024 at 03:37:57PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Tue, Jan 23, 2024 at 05:49:33PM +0100, Florian Westphal wrote:
> > > This has to either consider ->nvals > 1 or check the values
> > > post-no-range-fixup:
> > > 
> > > ./iptables-test.py  extensions/libebt_stp.t
> > > extensions/libebt_stp.t: ERROR: line 12 (cannot load: ebtables -A INPUT --stp-root-cost 1)
> > > 
> > > (it tests 0 < 1 and fails, but test should be 1 < 1).
> > > 
> > > Fixes: dc6efcfeac38 ("extensions: libebt_stp: Use guided option parser")
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > >  extensions/libebt_stp.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/extensions/libebt_stp.c b/extensions/libebt_stp.c
> > > index 81054b26c1f0..371fa04c870f 100644
> > > --- a/extensions/libebt_stp.c
> > > +++ b/extensions/libebt_stp.c
> > > @@ -142,7 +142,7 @@ static void brstp_parse(struct xt_option_call *cb)
> > >  #define RANGE_ASSIGN(name, fname, val) {				    \
> > >  		stpinfo->config.fname##l = val[0];			    \
> > >  		stpinfo->config.fname##u = cb->nvals > 1 ? val[1] : val[0]; \
> > > -		if (val[1] < val[0])					    \
> > > +		if (stpinfo->config.fname##u < stpinfo->config.fname##l)    \
> > >  			xtables_error(PARAMETER_PROBLEM,		    \
> > >  				      "Bad --stp-" name " range");	    \
> > >  }
> > 
> > This is odd: xtopt_parse_mint() assigns UINT32_MAX to val[1] for
> > XTTYPE_UINT32RC if no upper end is given. Also, extensions/libebt_stp.t
> > passes for me. What's missing on my end?
> 
> No fukcing clue. did git clean -d -f -x and it works now without the
> patch.

My first guess was outdated libxtables, but xtopt_parse_mint() is
basically unchanged since 2011 at least.

> I hate my life.

While you correctly hate the game instead of its player, you probably
hate the wrong game: The code above indeed is confusing. Maybe one
should move that monotonicity check into libxtables which should
simplify it quite a bit. I'll have a look. :)

Thanks, Phil

Re: [PATCH iptables] extensions: libebt_stp: fix range checking

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> While you correctly hate the game instead of its player, you probably
> hate the wrong game: The code above indeed is confusing. Maybe one
> should move that monotonicity check into libxtables which should
> simplify it quite a bit. I'll have a look. :)

Something IS broken.  Still not working on FC 39 test machine
even after fresh clone.

On a "working" VM:
export XTABLES_LIBDIR=$(pwd)/extensions
iptables/xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
have 1 32765

@@ -150,7 +151,9 @@ static void brstp_parse(struct xt_option_call *cb)
                RANGE_ASSIGN("root-prio", root_prio, cb->val.u16_range);
                break;
        case O_RCOST:
+               fprintf(stderr, "have %u %u\n", cb->val.u32_range[0], cb->val.u32_range[1]);

I can't even figure out where the correct max value is supposed to be set.

Varying the input:

xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
have 1 32764

Looks to me as if the upper value is undefined.

Other users of *RC versions handle it in .parse, e.g. libxt_length.
No idea how this is working.

Re: [PATCH iptables] extensions: libebt_stp: fix range checking

[Phil Sutter]

On Wed, Jan 24, 2024 at 04:42:16PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > While you correctly hate the game instead of its player, you probably
> > hate the wrong game: The code above indeed is confusing. Maybe one
> > should move that monotonicity check into libxtables which should
> > simplify it quite a bit. I'll have a look. :)
> 
> Something IS broken.  Still not working on FC 39 test machine
> even after fresh clone.
> 
> On a "working" VM:
> export XTABLES_LIBDIR=$(pwd)/extensions
> iptables/xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> have 1 32765
> 
> @@ -150,7 +151,9 @@ static void brstp_parse(struct xt_option_call *cb)
>                 RANGE_ASSIGN("root-prio", root_prio, cb->val.u16_range);
>                 break;
>         case O_RCOST:
> +               fprintf(stderr, "have %u %u\n", cb->val.u32_range[0], cb->val.u32_range[1]);
> 
> I can't even figure out where the correct max value is supposed to be set.
> 
> Varying the input:
> 
> xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> have 1 32764
> 
> Looks to me as if the upper value is undefined.
> 
> Other users of *RC versions handle it in .parse, e.g. libxt_length.
> No idea how this is working.

In xtopt_parse_mint(), there is:

| const uintmax_t lmax = xtopt_max_by_type(entry->type);
| [...]
| if (*arg == '\0' || *arg == sep) {
|         /* Default range components when field not spec'd. */
|         end = (char *)arg;
|         value = (cb->nvals == 1) ? lmax : 0;

But that branch appears to be dead code. So this is indeed a bug and a
specific build may or may not hit it as your experience shows. I'll see
how xtopt_parse_mint() can be fixed.

The reason why extensions may sanitize the values is because kernel code
may expect upper==lower or upper==MAX if not given.

Cheers, Phil

Re: [PATCH iptables] extensions: libebt_stp: fix range checking

[Phil Sutter]

On Wed, Jan 24, 2024 at 05:13:45PM +0100, Phil Sutter wrote:
> On Wed, Jan 24, 2024 at 04:42:16PM +0100, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > While you correctly hate the game instead of its player, you probably
> > > hate the wrong game: The code above indeed is confusing. Maybe one
> > > should move that monotonicity check into libxtables which should
> > > simplify it quite a bit. I'll have a look. :)
> > 
> > Something IS broken.  Still not working on FC 39 test machine
> > even after fresh clone.
> > 
> > On a "working" VM:
> > export XTABLES_LIBDIR=$(pwd)/extensions
> > iptables/xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> > have 1 32765
> > 
> > @@ -150,7 +151,9 @@ static void brstp_parse(struct xt_option_call *cb)
> >                 RANGE_ASSIGN("root-prio", root_prio, cb->val.u16_range);
> >                 break;
> >         case O_RCOST:
> > +               fprintf(stderr, "have %u %u\n", cb->val.u32_range[0], cb->val.u32_range[1]);
> > 
> > I can't even figure out where the correct max value is supposed to be set.
> > 
> > Varying the input:
> > 
> > xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> > have 1 32764
> > 
> > Looks to me as if the upper value is undefined.
> > 
> > Other users of *RC versions handle it in .parse, e.g. libxt_length.
> > No idea how this is working.
> 
> In xtopt_parse_mint(), there is:
> 
> | const uintmax_t lmax = xtopt_max_by_type(entry->type);
> | [...]
> | if (*arg == '\0' || *arg == sep) {
> |         /* Default range components when field not spec'd. */
> |         end = (char *)arg;
> |         value = (cb->nvals == 1) ? lmax : 0;
> 
> But that branch appears to be dead code. So this is indeed a bug and a
> specific build may or may not hit it as your experience shows. I'll see
> how xtopt_parse_mint() can be fixed.

The big elucidation was the code is called only for ranges and somehow I
managed to miss the point that your sample command doesn't contain a
range in the first place.

So while I still think it makes sense to have the 'low <= high' check
done by the parser, I applied your patch for now as it indeed fixes that
bug in libebt_stp extension parser. Sorry for all the confusion I must
have caused. :(

Meanwhile I've added test cases for ranges in various formats which
uncovered quite a few things to fix.

Thanks, Phil