[PATCH nft] src: Don't parse string as verdict in map

[PATCH nft] src: Don't parse string as verdict in map

[Xiao Liang]

In verdict map, string values are accidentally treated as verdicts.

For example:
    table ip t {
        map foo {
           type mark : verdict
           elements = {
              0 : bar
           }
        }
    }
The value "bar" is sent to kernel as verdict.

Indeed, we don't parse verdicts during evaluation, but only chains,
which is of type string rather than verdict.

Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
---
 src/datatype.c | 12 ------------
 src/evaluate.c |  3 ++-
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index 2e31c858..002ed46a 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -321,23 +321,11 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
 	}
 }
 
-static struct error_record *verdict_type_parse(struct parse_ctx *ctx,
-					       const struct expr *sym,
-					       struct expr **res)
-{
-	*res = constant_expr_alloc(&sym->location, &string_type,
-				   BYTEORDER_HOST_ENDIAN,
-				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
-				   sym->identifier);
-	return NULL;
-}
-
 const struct datatype verdict_type = {
 	.type		= TYPE_VERDICT,
 	.name		= "verdict",
 	.desc		= "netfilter verdict",
 	.print		= verdict_type_print,
-	.parse		= verdict_type_parse,
 };
 
 static const struct symbol_table nfproto_tbl = {
diff --git a/src/evaluate.c b/src/evaluate.c
index 919c38c5..d9c9ca28 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2575,7 +2575,8 @@ static int stmt_evaluate_verdict(struct eval_ctx *ctx, struct stmt *stmt)
 		if (stmt->expr->verdict != NFT_CONTINUE)
 			stmt->flags |= STMT_F_TERMINAL;
 		if (stmt->expr->chain != NULL) {
-			if (expr_evaluate(ctx, &stmt->expr->chain) < 0)
+			if (stmt_evaluate_arg(ctx, stmt, &string_type, 0, 0,
+					      &stmt->expr->chain) < 0)
 				return -1;
 			if (stmt->expr->chain->etype != EXPR_VALUE) {
 				return expr_error(ctx->msgs, stmt->expr->chain,
-- 
2.37.1

Re: [PATCH nft] src: Don't parse string as verdict in map

[Florian Westphal]

Xiao Liang <shaw.leon@gmail.com> wrote:
> In verdict map, string values are accidentally treated as verdicts.
> 
> For example:
>     table ip t {
>         map foo {
>            type mark : verdict
>            elements = {
>               0 : bar
>            }
>         }
>     }
> The value "bar" is sent to kernel as verdict.
> 
> Indeed, we don't parse verdicts during evaluation, but only chains,
> which is of type string rather than verdict.

Can you explain what this is fixing?

This reverts the commit that adds support for defines as aliases:

commit c64457cff9673fbb41f613a67e158b4d62235c09
src: Allow goto and jump to a variable

Re: [PATCH nft] src: Don't parse string as verdict in map

[Xiao Liang]

On Thu, Aug 18, 2022 at 9:32 PM Florian Westphal <fw@strlen.de> wrote:
>
> Can you explain what this is fixing?

See this example:
table t {
    map foo {
        type ipv4_addr : verdict
        elements = {
            192.168.0.1 : bar
        }
    }
    chain output {
        type filter hook output priority mangle;
        ip daddr vmap @foo
    }
}

Though "bar" is not a valid verdict (should be "jump bar" or
something), the string is taken as the element value. Then
NFTA_DATA_VALUE is sent to the kernel instead of NFTA_DATA_VERDICT.
Recent kernel checks the type and returns error, but olders (e.g.
v5.4.x) doesn't, causing a warning when the rule is hit:

[5120263.467627] WARNING: CPU: 12 PID: 303303 at
net/netfilter/nf_tables_core.c:229 nft_do_chain+0x394/0x500
[nf_tables]

>
> This reverts the commit that adds support for defines as aliases:
>
> commit c64457cff9673fbb41f613a67e158b4d62235c09
> src: Allow goto and jump to a variable
>

This patch fixes it by parsing chain names as strings rather than
verdicts, i.e. "jump $var" is a verdict while "$var" is a string.

Re: [PATCH nft] src: Don't parse string as verdict in map

[Florian Westphal]

Xiao Liang <shaw.leon@gmail.com> wrote:
> On Thu, Aug 18, 2022 at 9:32 PM Florian Westphal <fw@strlen.de> wrote:
> >
> > Can you explain what this is fixing?
> 
> See this example:
> table t {
>     map foo {
>         type ipv4_addr : verdict
>         elements = {
>             192.168.0.1 : bar
>         }
>     }
>     chain output {
>         type filter hook output priority mangle;
>         ip daddr vmap @foo
>     }
> }
> 
> Though "bar" is not a valid verdict (should be "jump bar" or
> something), the string is taken as the element value. Then
> NFTA_DATA_VALUE is sent to the kernel instead of NFTA_DATA_VERDICT.
> Recent kernel checks the type and returns error, but olders (e.g.
> v5.4.x) doesn't, causing a warning when the rule is hit:
> 
> [5120263.467627] WARNING: CPU: 12 PID: 303303 at
> net/netfilter/nf_tables_core.c:229 nft_do_chain+0x394/0x500
> [nf_tables]

Thanks.  All of this info should be included in the commit message.

Perhaps adding a test case is warrented as well.