Included bogon crashes, after fix: empty_geneve_definition_crash:2:9-16: Error: Could not process rule: Invalid argument Since this feature is undocumented (hint, hint) I don't know if there are cases where ip daddr can be elided. If not, a followup patch should reject empty dst upfront so users get a more verbose error message. Signed-off-by: Florian Westphal --- src/evaluate.c | 9 +++++---- .../testcases/bogons/nft-f/empty_geneve_definition_crash | 4 ++++ 2 files changed, 9 insertions(+), 4 deletions(-) create mode 100644 tests/shell/testcases/bogons/nft-f/empty_geneve_definition_crash diff --git a/src/evaluate.c b/src/evaluate.c index 0c7d90f8f43b..ac482c83cce2 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -5865,11 +5865,12 @@ static int tunnel_evaluate(struct eval_ctx *ctx, struct obj *obj) obj->tunnel.dst->dtype->size); if (expr_evaluate(ctx, &obj->tunnel.dst) < 0) return -1; - } - if (obj->tunnel.src->dtype != obj->tunnel.dst->dtype) - return __stmt_binary_error(ctx, &obj->location, NULL, - "specify either ip or ip6 for address"); + if (obj->tunnel.src && + obj->tunnel.src->dtype != obj->tunnel.dst->dtype) + return __stmt_binary_error(ctx, &obj->location, NULL, + "specify either ip or ip6 for address"); + } return 0; } diff --git a/tests/shell/testcases/bogons/nft-f/empty_geneve_definition_crash b/tests/shell/testcases/bogons/nft-f/empty_geneve_definition_crash new file mode 100644 index 000000000000..d1bc76c56bd5 --- /dev/null +++ b/tests/shell/testcases/bogons/nft-f/empty_geneve_definition_crash @@ -0,0 +1,4 @@ +table netdev x { + tunnel geneve-t { + } +} -- 2.51.0 Included bogons crash with segfault and assertion. After fix: tunnel_with_garbage_dst:3:12-14: Error: syntax error, unexpected tcp, expecting string or quoted string or string with a trailing asterisk or '$' ip saddr tcp dport { } ^^^ The parser change restricts the grammar to no longer allow this, we would crash here because we enter payload evaluation path that tries to insert a dependency into the rule, but we don't have one (ctx->rule and ctx->stmt are NULL as expected here). The eval stage change makes sure we will reject non-value symbols: tunnel_with_anon_set_assert:1:12-31: Error: must be a value, not set define s = { 1.2.3.4, 5.6.7.8 } ^^^^^^^^^^^^^^^^^^^^ Signed-off-by: Florian Westphal --- src/evaluate.c | 20 +++++++++++++++++-- src/parser_bison.y | 8 ++++---- .../bogons/nft-f/tunnel_with_anon_set_assert | 8 ++++++++ .../bogons/nft-f/tunnel_with_garbage_dst | 5 +++++ 4 files changed, 35 insertions(+), 6 deletions(-) create mode 100644 tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert create mode 100644 tests/shell/testcases/bogons/nft-f/tunnel_with_garbage_dst diff --git a/src/evaluate.c b/src/evaluate.c index ac482c83cce2..a5cc41819198 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -5851,19 +5851,35 @@ static int ct_timeout_evaluate(struct eval_ctx *ctx, struct obj *obj) return 0; } +static int tunnel_evaluate_addr(struct eval_ctx *ctx, struct expr **exprp) +{ + struct expr *e; + int ret; + + ret = expr_evaluate(ctx, exprp); + if (ret < 0) + return ret; + + e = *exprp; + if (e->etype != EXPR_VALUE) + return expr_error(ctx->msgs, e, "must be a value, not %s", expr_name(e)); + + return 0; +} + static int tunnel_evaluate(struct eval_ctx *ctx, struct obj *obj) { if (obj->tunnel.src) { expr_set_context(&ctx->ectx, obj->tunnel.src->dtype, obj->tunnel.src->dtype->size); - if (expr_evaluate(ctx, &obj->tunnel.src) < 0) + if (tunnel_evaluate_addr(ctx, &obj->tunnel.src) < 0) return -1; } if (obj->tunnel.dst) { expr_set_context(&ctx->ectx, obj->tunnel.dst->dtype, obj->tunnel.dst->dtype->size); - if (expr_evaluate(ctx, &obj->tunnel.dst) < 0) + if (tunnel_evaluate_addr(ctx, &obj->tunnel.dst) < 0) return -1; if (obj->tunnel.src && diff --git a/src/parser_bison.y b/src/parser_bison.y index 100a5c871e61..b63c7df18a35 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -5068,22 +5068,22 @@ tunnel_config : ID NUM { $0->tunnel.id = $2; } - | IP SADDR expr close_scope_ip + | IP SADDR symbol_expr close_scope_ip { $0->tunnel.src = $3; datatype_set($3, &ipaddr_type); } - | IP DADDR expr close_scope_ip + | IP DADDR symbol_expr close_scope_ip { $0->tunnel.dst = $3; datatype_set($3, &ipaddr_type); } - | IP6 SADDR expr close_scope_ip6 + | IP6 SADDR symbol_expr close_scope_ip6 { $0->tunnel.src = $3; datatype_set($3, &ip6addr_type); } - | IP6 DADDR expr close_scope_ip6 + | IP6 DADDR symbol_expr close_scope_ip6 { $0->tunnel.dst = $3; datatype_set($3, &ip6addr_type); diff --git a/tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert b/tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert new file mode 100644 index 000000000000..6f7b212aefef --- /dev/null +++ b/tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert @@ -0,0 +1,8 @@ +define s = { 1.2.3.4, 5.6.7.8 } + +table netdev x { + tunnel t { + ip saddr $s + } + } + diff --git a/tests/shell/testcases/bogons/nft-f/tunnel_with_garbage_dst b/tests/shell/testcases/bogons/nft-f/tunnel_with_garbage_dst new file mode 100644 index 000000000000..85eb992cec16 --- /dev/null +++ b/tests/shell/testcases/bogons/nft-f/tunnel_with_garbage_dst @@ -0,0 +1,5 @@ +table netdev x { + tunnel t { + ip saddr tcp dport { } + } +} -- 2.51.0 minor change to the bogon makes it assert because symbolic expression will have wrong refcount (2) at scope teardown. Signed-off-by: Florian Westphal --- src/parser_bison.y | 17 +++++++++++++++++ .../bogons/nft-f/tunnel_with_anon_set_assert | 1 + 2 files changed, 18 insertions(+) diff --git a/src/parser_bison.y b/src/parser_bison.y index b63c7df18a35..4e028d31c165 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -5070,21 +5070,38 @@ tunnel_config : ID NUM } | IP SADDR symbol_expr close_scope_ip { + if (already_set($0->tunnel.src, &@3, state)) { + expr_free($3); + YYERROR; + } + $0->tunnel.src = $3; datatype_set($3, &ipaddr_type); } | IP DADDR symbol_expr close_scope_ip { + if (already_set($0->tunnel.dst, &@3, state)) { + expr_free($3); + YYERROR; + } $0->tunnel.dst = $3; datatype_set($3, &ipaddr_type); } | IP6 SADDR symbol_expr close_scope_ip6 { + if (already_set($0->tunnel.src, &@3, state)) { + expr_free($3); + YYERROR; + } $0->tunnel.src = $3; datatype_set($3, &ip6addr_type); } | IP6 DADDR symbol_expr close_scope_ip6 { + if (already_set($0->tunnel.dst, &@3, state)) { + expr_free($3); + YYERROR; + } $0->tunnel.dst = $3; datatype_set($3, &ip6addr_type); } diff --git a/tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert b/tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert index 6f7b212aefef..d02568944301 100644 --- a/tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert +++ b/tests/shell/testcases/bogons/nft-f/tunnel_with_anon_set_assert @@ -3,6 +3,7 @@ define s = { 1.2.3.4, 5.6.7.8 } table netdev x { tunnel t { ip saddr $s + ip saddr $s } } -- 2.51.0 Included bogon causes a crash because the list head isn't initialised due to tunnel->type == VXLAN. Signed-off-by: Florian Westphal --- src/parser_bison.y | 38 ++++++++++++++++--- .../bogons/nft-f/tunnel_in_tunnel_crash | 10 +++++ 2 files changed, 42 insertions(+), 6 deletions(-) create mode 100644 tests/shell/testcases/bogons/nft-f/tunnel_in_tunnel_crash diff --git a/src/parser_bison.y b/src/parser_bison.y index 4e028d31c165..3c21c7584d01 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -144,6 +144,19 @@ static bool already_set(const void *attr, const struct location *loc, return true; } +static bool tunnel_set_type(const struct location *loc, + struct obj *obj, enum tunnel_type type, const char *name, + struct parser_state *state) +{ + if (obj->tunnel.type) { + erec_queue(error(loc, "Cannot create new %s section inside another tunnel", name), state->msgs); + return false; + } + + obj->tunnel.type = type; + return true; +} + static struct expr *ifname_expr_alloc(const struct location *location, struct list_head *queue, const char *name) @@ -4980,11 +4993,15 @@ erspan_block : /* empty */ { $$ = $-1; } erspan_block_alloc : /* empty */ { $$ = $-1; + + if (!tunnel_set_type(&$$->location, $$, TUNNEL_ERSPAN, "erspan", state)) + YYERROR; } ; erspan_config : HDRVERSION NUM { + assert($0->tunnel.type == TUNNEL_ERSPAN); $0->tunnel.erspan.version = $2; } | INDEX NUM @@ -5017,6 +5034,10 @@ geneve_block : /* empty */ { $$ = $-1; } geneve_block_alloc : /* empty */ { $$ = $-1; + if (!tunnel_set_type(&$$->location, $$, TUNNEL_GENEVE, "geneve", state)) + YYERROR; + + init_list_head(&$$->tunnel.geneve_opts); } ; @@ -5024,6 +5045,8 @@ geneve_config : CLASS NUM OPTTYPE NUM DATA string { struct tunnel_geneve *geneve; + assert($0->tunnel.type == TUNNEL_GENEVE); + geneve = xmalloc(sizeof(struct tunnel_geneve)); geneve->geneve_class = $2; geneve->type = $4; @@ -5034,10 +5057,6 @@ geneve_config : CLASS NUM OPTTYPE NUM DATA string YYERROR; } - if (!$0->tunnel.type) { - $0->tunnel.type = TUNNEL_GENEVE; - init_list_head(&$0->tunnel.geneve_opts); - } list_add_tail(&geneve->list, &$0->tunnel.geneve_opts); free_const($6); } @@ -5055,11 +5074,15 @@ vxlan_block : /* empty */ { $$ = $-1; } vxlan_block_alloc : /* empty */ { $$ = $-1; + + if (!tunnel_set_type(&$$->location, $$, TUNNEL_VXLAN, "vxlan", state)) + YYERROR; } ; vxlan_config : GBP NUM { + assert($0->tunnel.type == TUNNEL_VXLAN); $0->tunnel.vxlan.gbp = $2; } ; @@ -5123,13 +5146,16 @@ tunnel_config : ID NUM } | ERSPAN erspan_block_alloc '{' erspan_block '}' { - $0->tunnel.type = TUNNEL_ERSPAN; + $2->location = @2; } | VXLAN vxlan_block_alloc '{' vxlan_block '}' { - $0->tunnel.type = TUNNEL_VXLAN; + $2->location = @2; } | GENEVE geneve_block_alloc '{' geneve_block '}' + { + $2->location = @2; + } ; tunnel_block : /* empty */ { $$ = $-1; } diff --git a/tests/shell/testcases/bogons/nft-f/tunnel_in_tunnel_crash b/tests/shell/testcases/bogons/nft-f/tunnel_in_tunnel_crash new file mode 100644 index 000000000000..9f029807f521 --- /dev/null +++ b/tests/shell/testcases/bogons/nft-f/tunnel_in_tunnel_crash @@ -0,0 +1,10 @@ +table netdev x { + tunnel geneve-t { + vxlan { + gbp 200 + } + geneve { + class 0x1 opt-type 0x1 data "0x12345678" + } + } + -- 2.51.0