ping! OK from anyone?

thanks and
regards
sashan


On Fri, May 24, 2024 at 09:23:54AM +0200, Alexandr Nedvedicky wrote:
> Hello,
> 
> The issue has been brought up and analyzed by Giannis Kapetanakis here [1].
> Giannis went back in history and found a culprit:
> 
>     pf.c (2023/01/05) 1.1169
> 
> The change in 1.1169 fixes stack overflow which may be triggered with deeply
> nested anchors. Unfortunately that fix introduces issue reported by Giannis.
> To trigger it one has to use rule anchors like for example here:
> 
>     @0 match in all scrub (no-df random-id)
>     @1 pass out log proto tcp from self to any port 12345
>     @2 anchor "relayd/*"
>     @3 anchor "test" {
> 	@0 pass out log proto tcp from self to any port 12346
> 	@1 anchor "foo" {
> 	    @0 pass out log proto tcp from self to any port 12348
> 	}
> 	@2 pass out log proto tcp from self to any port 12349
>     }
>     @4 pass out log proto tcp from self to any port 12347
> 
> Rules above use the same numbering style which is also used by command
>     'pfctl -sr -v' 
> 
> if packet is sent to remote port 12349 it matches the rule @2
> in anchor @3 ('test'). pflog (and also state shown by 'pfctl -ss -vv')
> should report that in form:
>     anchor 3, rule 2
> however the pf in current reports this:
>     anchor 1, rule 2
> 
> The diff below is based on idea of simple one-liner tweak
> proposed by Giannis off-list. His idea is to just forget
> the reference to anchor as pf backtraces back towards root.
> Such fix would work if there would be no rule @2 in anchor @3.
> For general case we need to save/restore reference to anchor
> as we traverse the rulesets.
> 
> Note: I think pf never recorded a full path from root to rule,
> pf just reports leaf anchor where the rule belongs to. To record
> the full path a follow-up change is needed.
> 
> OK to commit diff below?
> 
> thanks and
> regards
> sashan
> 
> [1] https://marc.info/?t=171611737500001&r=1&w=2
> 
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 8591b044e43..007a00c6bc6 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -3666,8 +3666,8 @@ pf_anchor_stack_top(void)
>  }
>  
>  int
> -pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r,
> -    struct pf_anchor *child, int jump_target)
> +pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *anchor,
> +    struct pf_rule *r, struct pf_anchor *child, int jump_target)
>  {
>  	struct pf_anchor_stackframe *stack;
>  	struct pf_anchor_stackframe *top_sf = pf_anchor_stack_top();
> @@ -3677,6 +3677,7 @@ pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r,
>  		return (-1);
>  
>  	top_sf->sf_rs = rs;
> +	top_sf->sf_anchor = anchor;
>  	top_sf->sf_r = r;
>  	top_sf->sf_child = child;
>  	top_sf->sf_jump_target = jump_target;
> @@ -3693,8 +3694,8 @@ pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r,
>  }
>  
>  int
> -pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **r,
> -    struct pf_anchor **child, int *jump_target)
> +pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **anchor,
> +    struct pf_rule **r, struct pf_anchor **child, int *jump_target)
>  {
>  	struct pf_anchor_stackframe *top_sf = pf_anchor_stack_top();
>  	struct pf_anchor_stackframe *stack;
> @@ -3710,6 +3711,7 @@ pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **r,
>  			    __func__);
>  
>  		*rs = top_sf->sf_rs;
> +		*anchor = top_sf->sf_anchor;
>  		*r = top_sf->sf_r;
>  		*child = top_sf->sf_child;
>  		*jump_target = top_sf->sf_jump_target;
> @@ -4306,25 +4308,27 @@ enter_ruleset:
>  			if (r->quick)
>  				return (PF_TEST_QUICK);
>  		} else {
> -			ctx->a = r;
>  			ctx->aruleset = &r->anchor->ruleset;
>  			if (r->anchor_wildcard) {
>  				RB_FOREACH(child, pf_anchor_node,
>  				    &r->anchor->children) {
> -					if (pf_anchor_stack_push(ruleset, r,
> -					    child, PF_NEXT_CHILD) != 0)
> +					if (pf_anchor_stack_push(ruleset,
> +					    ctx->a, r, child,
> +					    PF_NEXT_CHILD) != 0)
>  						return (PF_TEST_FAIL);
>  
> +					ctx->a = r;
>  					ruleset = &child->ruleset;
>  					goto enter_ruleset;
>  next_child:
>  					continue;	/* with RB_FOREACH() */
>  				}
>  			} else {
> -				if (pf_anchor_stack_push(ruleset, r, child,
> -				    PF_NEXT_RULE) != 0)
> +				if (pf_anchor_stack_push(ruleset, ctx->a,
> +				    r, child, PF_NEXT_RULE) != 0)
>  					return (PF_TEST_FAIL);
>  
> +				ctx->a = r;
>  				ruleset = &r->anchor->ruleset;
>  				child = NULL;
>  				goto enter_ruleset;
> @@ -4335,7 +4339,9 @@ next_rule:
>  		r = TAILQ_NEXT(r, entries);
>  	}
>  
> -	if (pf_anchor_stack_pop(&ruleset, &r, &child, &target) == 0) {
> +	if (pf_anchor_stack_pop(&ruleset, &ctx->a, &r, &child,
> +	    &target) == 0) {
> +
>  		/* stop if any rule matched within quick anchors. */
>  		if (r->quick == PF_TEST_QUICK && *ctx->am == r)
>  			return (PF_TEST_QUICK);
> diff --git a/sys/net/pfvar_priv.h b/sys/net/pfvar_priv.h
> index 7a420bfa308..559273046ee 100644
> --- a/sys/net/pfvar_priv.h
> +++ b/sys/net/pfvar_priv.h
> @@ -321,6 +321,7 @@ struct pf_pdesc {
>  
>  struct pf_anchor_stackframe {
>  	struct pf_ruleset	*sf_rs;
> +	struct pf_rule		*sf_anchor;
>  	union {
>  		struct pf_rule			*u_r;
>  		struct pf_anchor_stackframe	*u_stack_top;
>