From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: pf af-to breaks traceroute
To: Alexandr Nedvedicky <sashan@fastmail.net>
Cc: Kristof Provost <kp@freebsd.org>, tech@openbsd.org
Date: Mon, 3 Mar 2025 23:09:11 +0100

On Mon, Mar 03, 2025 at 01:28:43AM +0100, Alexandr Nedvedicky wrote:
> Hello,
> 
> thanks for giving it a shot on regress. The failure was
> caused by the first chunk in earlier diff. I've just
> added the change to branch if (pf_icmp_mappin(...) == 0)
> which deals with echo request/reply. This was wrong and
> I think Kristof also has pointed that out.
> 
> The updated diff passes pf_foward test. 
> 
> I've also re-arranged code in ICMP branch. Hope I got
> your suggestion right.

More or less.  If you keep the order of pf_addrcpy() calls, the
code is more consistent to the last case IPPROTO_ICMPV6.
And one line is too long.

I have attached a fixed diff to avoid confusion what I mean.

> OK to commit diff below?

I did run my smaller diff successfully through regress.

OK bluhm@

Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
diff -u -p -r1.1207 pf.c
--- net/pf.c	26 Dec 2024 10:15:27 -0000	1.1207
+++ net/pf.c	3 Mar 2025 12:38:16 -0000
@@ -5925,10 +5925,6 @@ pf_test_state_icmp(struct pf_pdesc *pd, 
 					    pd, &pd2, &nk->addr[sidx],
 					    &nk->addr[didx], pd->af, nk->af))
 						return (PF_DROP);
-					if (nk->af == AF_INET)
-						pd->proto = IPPROTO_ICMP;
-					else
-						pd->proto = IPPROTO_ICMPV6;
 					pd->m->m_pkthdr.ph_rtableid =
 					    nk->rdomain;
 					pd->destchg = 1;
@@ -5936,6 +5932,20 @@ pf_test_state_icmp(struct pf_pdesc *pd, 
 					    &nk->addr[pd2.sidx], nk->af);
 					pf_addrcpy(&pd->ndaddr,
 					    &nk->addr[pd2.didx], nk->af);
+					if (nk->af == AF_INET) {
+						pd->proto = IPPROTO_ICMP;
+					} else {
+						pd->proto = IPPROTO_ICMPV6;
+						/*
+						 * IPv4 becomes IPv6 so we must
+						 * copy IPv4 src addr to least
+						 * 32bits in IPv6 address to
+						 * keep traceroute/icmp
+						 * working.
+						 */
+						pd->nsaddr.addr32[3] =
+						    pd->src->addr32[0];
+					}
 					pd->naf = nk->af;
 
 					pf_patch_16(pd,
@@ -6045,10 +6055,6 @@ pf_test_state_icmp(struct pf_pdesc *pd, 
 					    pd, &pd2, &nk->addr[sidx],
 					    &nk->addr[didx], pd->af, nk->af))
 						return (PF_DROP);
-					if (nk->af == AF_INET)
-						pd->proto = IPPROTO_ICMP;
-					else
-						pd->proto = IPPROTO_ICMPV6;
 					pd->m->m_pkthdr.ph_rtableid =
 					    nk->rdomain;
 					pd->destchg = 1;
@@ -6056,6 +6062,20 @@ pf_test_state_icmp(struct pf_pdesc *pd, 
 					    &nk->addr[pd2.sidx], nk->af);
 					pf_addrcpy(&pd->ndaddr,
 					    &nk->addr[pd2.didx], nk->af);
+					if (nk->af == AF_INET) {
+						pd->proto = IPPROTO_ICMP;
+					} else {
+						pd->proto = IPPROTO_ICMPV6;
+						/*
+						 * IPv4 becomes IPv6 so we must
+						 * copy IPv4 src addr to least
+						 * 32bits in IPv6 address to
+						 * keep traceroute/icmp
+						 * working.
+						 */
+						pd->nsaddr.addr32[3] =
+						    pd->src->addr32[0];
+					}
 					pd->naf = nk->af;
 
 					pf_patch_16(pd,
@@ -6186,6 +6206,14 @@ pf_test_state_icmp(struct pf_pdesc *pd, 
 					    &nk->addr[pd2.sidx], nk->af);
 					pf_addrcpy(&pd->ndaddr,
 					    &nk->addr[pd2.didx], nk->af);
+					/*
+					 * IPv4 becomes IPv6 so we must copy
+					 * IPv4 src addr to least 32bits in
+					 * IPv6 address to keep traceroute
+					 * working.
+					 */
+					pd->nsaddr.addr32[3] =
+					    pd->src->addr32[0];
 					pd->naf = nk->af;
 					return (PF_AFRT);
 				}