fix icmp error source address over xfrm tunnel

Hi,
This fix, originally intended for XFRM/IPsec, has been recommended by Steffen Klassert to submit to the net tree.

The patch addresses a minor issue related to the IPv4 source address of ICMP error messages, which originated from an old 2011 commit:

415b3334a21a ("icmp: Fix regression in nexthop resolution during replies.")

The omission of a "Fixes" tag in the following commit is deliberate to prevent potential test failures and subsequent regression issues that may arise from backporting this patch all stable kerenels. This is a minor fix, anot not security fix.
With a seleftest I am submitting this to net-next tree.

v2->v3 : fix testscript. The IFS, space, got mangled. v1->v2 : add kernel selftest script

Antony Antony (2):
xfrm: fix source address in icmp error generation from IPsec gateway selftests/net: add ICMP unreachable over IPsec tunnel

 net/ipv4/icmp.c                           |   1 -
 tools/testing/selftests/net/Makefile      |   1 +
 tools/testing/selftests/net/xfrm_state.sh | 624 ++++++++++++++++++++++
 3 files changed, 625 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/xfrm_state.sh

--
2.30.2

When enabling support for XFRM lookup using reverse ICMP payload, we have identified an issue where the source address of the IPv4, e.g., "Destination Host Unreachable" message, is incorrect. The error message received by the sender appears to originate from a non-existing/unreachable host. IPv6 seems to behave correctly; respond with an existing source address from the gateway.

Here is example of incorrect source address for ICMP error response. When sending a ping to an unreachable host, the sender would receive an ICMP unreachable response with a fake source address. Rather the address of the host that generated ICMP Unreachable message. This is confusing and incorrect. A fllow up commit adds this example as a test case.

Example:
ping -W 9 -w 5 -c 1 10.1.4.3
PING 10.1.4.3 (10.1.4.3) 56(84) bytes of data. From 10.1.4.3 icmp_seq=1 Destination Host Unreachable

Notice : packet has the source address of the ICMP "Unreachable host!"

This issue can be traced back to commit
415b3334a21a ("icmp: Fix regression in nexthop resolution during replies.") which introduced a change that copied the source address from the ICMP payload.

This commit would force to use source address from the gatway/host. The ICMP error message source address correctly set from the host.

After fixing:
ping -W 5 -c 1 10.1.4.3
PING 10.1.4.3 (10.1.4.3) 56(84) bytes of data. From 10.1.3.2 icmp_seq=1 Destination Host Unreachable

Here is an snippt to reporduce the issue.

export AB="10.1"
for i in 1 2 3 4 5; do

        h="host${i}"
        ip netns add ${h}
        ip -netns ${h} link set lo up
        ip netns exec ${h} sysctl -wq net.ipv4.ip_forward=1
        if [ $i -lt 5 ]; then
                ip -netns ${h} link add eth0 type veth peer name eth10${i}
                ip -netns ${h} addr add "${AB}.${i}.1/24" dev eth0
                ip -netns ${h} link set up dev eth0
        fi

done

for i in 1 2 3 4 5; do

        h="host${i}"
        p=$((i - 1))
        ph="host${p}"
        # connect to previous host
        if [ $i -gt 1 ]; then
                ip -netns ${ph} link set eth10${p} netns ${h}
                ip -netns ${h} link set eth10${p} name eth1
                ip -netns ${h} link set up dev eth1
                ip -netns ${h} addr add "${AB}.${p}.2/24" dev eth1
        fi
        # add forward routes
        for k in $(seq ${i} $((5 - 1))); do
                ip -netns ${h} route 2>/dev/null | (grep "${AB}.${k}.0" 2>/dev/null) || \
                ip -netns ${h} route add "${AB}.${k}.0/24" via "${AB}.${i}.2" 2>/dev/nul
        done

        # add reverse routes
        for k in $(seq 1 $((i - 2))); do
                ip -netns ${h} route 2>/dev/null | grep "${AB}.${k}.0" 2>/dev/null || \
                ip -netns ${h} route add "${AB}.${k}.0/24" via "${AB}.${p}.1" 2>/dev/nul
        done

done

ip netns exec host1 ping -q -W 2 -w 1 -c 1 10.1.4.2 2>&1>/dev/null && echo "success 10.1.4.2 reachable" || echo "ERROR" ip netns exec host1 ping -W 9 -w 5 -c 1 10.1.4.3 || echo "note the source address of unreachble of gateway" ip -netns host1 route flush cache

ip netns exec host3 nft add table inet filter
ip netns exec host3 nft add chain inet filter FORWARD { type filter hook forward priority filter\; policy drop \; } ip netns exec host3 nft add rule inet filter FORWARD counter ip protocol icmp drop
ip netns exec host3 nft add rule inet filter FORWARD counter ip protocol esp accept
ip netns exec host3 nft add rule inet filter FORWARD counter drop

ip -netns host2 xfrm policy add src 10.1.1.0/24 dst 10.1.4.0/24 dir out \
flag icmp tmpl src 10.1.2.1 dst 10.1.3.2 proto esp reqid 1 mode tunnel

ip -netns host2 xfrm policy add src 10.1.4.0/24 dst 10.1.1.0/24 dir in \

        tmpl src 10.1.3.2 dst 10.1.2.1 proto esp reqid 2 mode tunnel

ip -netns host2 xfrm policy add src 10.1.4.0/24 dst 10.1.1.0/24 dir fwd \
flag icmp tmpl src 10.1.3.2 dst 10.1.2.1 proto esp reqid 2 mode tunnel

ip -netns host2 xfrm state add src 10.1.2.1 dst 10.1.3.2 proto esp spi 1 \

        reqid 1 replay-window 1  mode tunnel aead 'rfc4106(gcm(aes))' \
        0x1111111111111111111111111111111111111111 96 \
        sel src 10.1.1.0/24 dst 10.1.4.0/24

ip -netns host2 xfrm state add src 10.1.3.2 dst 10.1.2.1 proto esp spi 2 \

        flag icmp reqid 2 replay-window 10 mode tunnel aead 'rfc4106(gcm(aes))' \
        0x2222222222222222222222222222222222222222 96

ip -netns host4 xfrm policy add src 10.1.4.0/24 dst 10.1.1.0/24 dir out \
flag icmp tmpl src 10.1.3.2 dst 10.1.2.1 proto esp reqid 1 mode tunnel

ip -netns host4 xfrm policy add src 10.1.1.0/24 dst 10.1.4.0/24 dir in \

        tmpl src 10.1.2.1 dst 10.1.3.2 proto esp reqid 2  mode tunnel

ip -netns host4 xfrm policy add src 10.1.1.0/24 dst 10.1.4.0/24 dir fwd \
flag icmp tmpl src 10.1.2.1 dst 10.1.3.2 proto esp reqid 2 mode tunnel

ip -netns host4 xfrm state add src 10.1.3.2 dst 10.1.2.1 proto esp spi 2 \

        reqid 1 replay-window 1 mode tunnel aead 'rfc4106(gcm(aes))' \
        0x2222222222222222222222222222222222222222 96

ip -netns host4 xfrm state add src 10.1.2.1 dst 10.1.3.2 proto esp spi 1 \

        reqid 2 replay-window 20 flag icmp  mode tunnel aead 'rfc4106(gcm(aes))' \
        0x1111111111111111111111111111111111111111 96 \
        sel src 10.1.1.0/24 dst 10.1.4.0/24

ip netns exec host1 ping -W 5 -c 1 10.1.4.2 2>&1 > /dev/null && echo ""
ip netns exec host1 ping -W 5 -c 1 10.1.4.3 || echo "note source address of gateway 10.1.3.2"

Again before the fix
ping -W 5 -c 1 10.1.4.3
From 10.1.4.3 icmp_seq=1 Destination Host Unreachable

After the fix
From 10.1.3.2 icmp_seq=1 Destination Host Unreachable

---
 net/ipv4/icmp.c | 1 -
 1 file changed, 1 deletion(-)

index 207482d30dc7..317ad5165408 100644
@@ -558,7 +558,6 @@ static struct rtable *icmp_route_lookup(struct net *net,
 	rt2 = dst_rtable(dst2);
 	if (!IS_ERR(dst2)) {
 		dst_release(&rt->dst);
-		memcpy(fl4, &fl4_dec, sizeof(*fl4));
 		rt = rt2;
 	} else if (PTR_ERR(dst2) == -EPERM) {
 		if (rt)

2.30.2

Add IPsec tunnel, aka xfrm state, tests with ICMP flags enabled. IPv4 and IPv6, unreachable tests over xfrm/IPsec tunnels, xfrm SA with "flag icmp" set.

---
v2->v3: fix the IFS whitespace. It got mangled

---
 tools/testing/selftests/net/Makefile      |   1 +
 tools/testing/selftests/net/xfrm_state.sh | 624 ++++++++++++++++++++++
 2 files changed, 625 insertions(+)
 create mode 100755 tools/testing/selftests/net/xfrm_state.sh

index 5befca249452..7d96b3e411b7 100644
@@ -53,6 +53,7 @@ TEST_PROGS += bind_bhash.sh
 TEST_PROGS += ip_local_port_range.sh
 TEST_PROGS += rps_default_mask.sh
 TEST_PROGS += big_tcp.sh
+TEST_PROGS += xfrm_state.sh
 TEST_PROGS_EXTENDED := toeplitz_client.sh toeplitz.sh
 TEST_GEN_FILES =  socket nettest
 TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any

2.30.2

Doesn't seem to work:

# selftests: net: xfrm_state.sh
# ./xfrm_state.sh: line 91: test_: command not found
# TEST: unreachable_ipv4IPv6 unreachable from router r3 [ FAIL ]
# ./xfrm_state.sh: line 91: test_: command not found
# TEST: unreachable_gw_ipv6IPv6 unreachable from IPsec gateway s2 [ FAIL ] # ./xfrm_state.sh: line 91: test_: command not found
# TEST: mtu_ipv6_r2IPv6 MTU exceeded from ESP router r2 [ FAIL ]
# ./xfrm_state.sh: line 91: test_: command not found
# TEST: mtu_ipv6_r3IPv6 MTU exceeded router r3 [ FAIL ]
not ok 1 selftests: net: xfrm_state.sh # exit=1

Hi Jakub,

thanks. I am looking into it. I notice two issues.

This appears to be an error from the v2 run, which was sent yesterday. The v3 patch should have superseded it.

The branch net-dev-testing/net-next-2024-05-06--12-00 contains the v2 patch. I wonder if net-dev testing recognized v3 patch.

git diff net-next-2024-05-06--12-00 net-next-2024-05-06--03-00 ./tools/testing/selftests/net/xfrm_state.sh is missing the expected one line diff in IFS.

I suspect there is another another issue with tools/testing/selftests/net/config . It does not appear to support nftables match for ESP. Which this script assumes.

# ip netns exec ns_r2-39oUmE nft add rule inet filter FORWARD counter ip protocol esp counter log accept
#
# Error: Could not process rule: No such file or directory
# add rule inet filter FORWARD counter ip protocol esp counter log accept
#               ^^^^^^

I learning vng also. I will send v4 with change to config, then I hope the test runner will pick up the latest patch.

-antony

You're right! I guess the pw-bot didn't discard v2 because of the capitalization change in the subject of the cover letter.

👍️

Hi Antony,

[...]

[...]

[...]

[...]

Sabrina

What kind of regression do you expect? If there's a risk of regression, I'm not sure net-next is that much "better" than net or stable. If a user complains about the new behavior breaking their setup, my understanding is that you would likely have to revert the patch anyway, or at least add some way to toggle the behavior.

Sabrina

Hi Sabrina,

For example, an old testing scripts with hardcoded source IP address assume that the "Unreachable response" will have the previous behavior. Such testing script may trigger regression when this patch is backported. Consequently, there may be discussions on whether this patch has broken the 10-year-old test scripts, which may be hard to fix.

My hope is that if this patch is applied to net-next without a "Fixes" tag, users would fix their testing scripts properly. Additionally, another piece of the puzzle for a complete fix is "forwarding of ICMP Error messages" patch that is in the kerenl 6.8, which is new feature and applied via ipsec-next.

-antony

Ok, that seems like an acceptable level of "regression" to me. Thanks for explaining.

I don't think the lack of a fixes tag will make people fix broken test scripts, but maybe I'm too pessimistic.

Sabrina