| Message ID | 20230630153759.3349299-1-maze@google.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | FYI 6.4 xfrm_prepare_input/xfrm_inner_mode_encap_remove WARN_ON hit - related to ESPinUDP | expand |
On Fri, Jun 30, 2023 at 5:38 PM Maciej Żenczykowski <maze@google.com> wrote: > Steffan, this isn't of course a patch meant for inclusion, instead just a WARN_ON hit report. Sorry for the name typo (it's Stefan in Polish). > The patch is simply what prints the following extra info: > > xfrm_prepare_input: XFRM_MODE_SKB_CB(skb)->protocol: 17 > xfrm_inner_mode_encap_remove: x->props.mode: 1 XFRM_MODE_SKB_SB(skb)->protocol:17 > > (note: XFRM_MODE_TUNNEL=1 IPPROTO_UDP=17) > > Hit on Linux 6.4 by: > https://cs.android.com/android/platform/superproject/+/master:kernel/tests/net/test/xfrm_test.py > > likely related to line 230: > encap_sock.setsockopt(IPPROTO_UDP, xfrm.UDP_ENCAP, xfrm.UDP_ENCAP_ESPINUDP) > > I'm not the author of these tests, and I know very little about XFRM. > As such, I'm not sure if there isn't a bug in the tests themselves... > maybe we're generating invalid packets that aren't meant to be decapsulated??? > > Or are we missing some sort of assignment inside of the ESP in UDP decap codepath? > > Somewhere in the vicinity of xfrm4_udp_encap_rcv / xfrm4_rcv_encap > (and the v6 equivalents) I've done some bisection (well more like educated guesswork) and the regression (if one should call it that?) is caused by 6.4 commit 5f24f41e8ea62a6a9095f9bbafb8b3aebe265c68 Author: Herbert Xu <herbert@gondor.apana.org.au> xfrm: Remove inner/outer modes from input path The xfrm tests pass either way, but with the above reverted it no longer triggers the WARN_ON. $ git log --decorate --oneline --graph -n 3 * da7dc0870b19 (HEAD) Revert "xfrm: Remove inner/outer modes from input path" <-- passes, doesn't warn * 51d5381c5809 ANDROID: net: xfrm: make PF_KEY SHA256 use RFC-compliant truncation. <-- passes, does warn * 5f24f41e8ea6 xfrm: Remove inner/outer modes from input path <-- passes xfrm, fails pf_key, warns
On Fri, Jun 30, 2023 at 08:37:58AM -0700, Maciej Żenczykowski wrote: > Steffan, this isn't of course a patch meant for inclusion, instead just a WARN_ON hit report. > The patch is simply what prints the following extra info: > > xfrm_prepare_input: XFRM_MODE_SKB_CB(skb)->protocol: 17 > xfrm_inner_mode_encap_remove: x->props.mode: 1 XFRM_MODE_SKB_SB(skb)->protocol:17 This seems to make no sense. UDP encapsulation is supposed to sit on the outside of ESP. So by the time we hit xfrm_input it should be lone gone. On the inside of the packet, as it's tunnel mode we should have either IPIP or IPV6, definitely not UDP. Are you able to reduce this to a set of "ip xfrm" commands that I can use to reproduce this? Thanks,
On Sat, Jul 1, 2023 at 9:51 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > xfrm_prepare_input: XFRM_MODE_SKB_CB(skb)->protocol: 17 > > xfrm_inner_mode_encap_remove: x->props.mode: 1 XFRM_MODE_SKB_SB(skb)->protocol:17 > > This seems to make no sense. UDP encapsulation is supposed to sit > on the outside of ESP. So by the time we hit xfrm_input it should > be lone gone. On the inside of the packet, as it's tunnel mode we > should have either IPIP or IPV6, definitely not UDP. It's triggering in testIPv4UDPEncapRecvTunnel() in xfrm_test.py. Specifically, it's the self.ReceivePacketOn(netid, input_pkt) a dozen lines higher. The packet we end up writing into the tap fd is 02 00 00 00 C8 01 02 00 00 00 C8 00 08 00 45 00 00 44 00 01 00 00 40 11 98 96 08 08 08 08 0A 00 C8 02 11 94 BF 12 00 30 1C D0 00 00 12 34 00 00 00 01 45 00 00 20 00 01 00 00 40 11 98 BA 08 08 08 08 0A 00 C8 02 01 BB 7D 7B 00 0C 9B 7A 01 02 02 11 You can decode this with https://hpd.gasmi.net/ or https://packetor.com/ You can decode the inner packet (this is null esp crypto) by passing in 00 00 00 00 00 00 00 00 00 00 00 00 08 00 45 00 00 20 00 01 00 00 40 11 98 BA 08 08 08 08 0A 00 C8 02 01 BB 7D 7B 00 0C 9B 7A 01 02 02 11 instead. Note that the protocol the kernel's printk I added prints is the *outer* encap UDP protocol, not the inner UDP. ie. you can change the scapy.UDP to scapy.TCP in the 'inner_pkt =' assignment, and the warning still triggers. The resulting packet is: 02 00 00 00 FA 01 02 00 00 00 FA 00 08 00 45 00 00 50 00 01 00 00 40 11 66 8A 08 08 08 08 0A 00 FA 02 11 94 A7 EB 00 3C 33 E0 00 00 12 34 00 00 00 01 45 00 00 2C 00 01 00 00 40 06 66 B9 08 08 08 08 0A 00 FA 02 01 BB 7D 7B 00 00 00 00 00 00 00 00 50 02 20 00 F9 82 00 00 01 02 02 11 ie. the inner packet is IPv4/TCP: 00 00 00 00 00 00 00 00 00 00 00 00 08 00 45 00 00 2C 00 01 00 00 40 06 66 B9 08 08 08 08 0A 00 FA 02 01 BB 7D 7B 00 00 00 00 00 00 00 00 50 02 20 00 F9 82 00 00 01 02 02 11 > Are you able to reduce this to a set of "ip xfrm" commands that I > can use to reproduce this?
On Sat, Jul 1, 2023 at 2:27 PM Maciej Żenczykowski <maze@google.com> wrote: > On Sat, Jul 1, 2023 at 9:51 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > xfrm_prepare_input: XFRM_MODE_SKB_CB(skb)->protocol: 17 > > > xfrm_inner_mode_encap_remove: x->props.mode: 1 XFRM_MODE_SKB_SB(skb)->protocol:17 > > > > This seems to make no sense. UDP encapsulation is supposed to sit > > on the outside of ESP. So by the time we hit xfrm_input it should > > be lone gone. On the inside of the packet, as it's tunnel mode we > > should have either IPIP or IPV6, definitely not UDP. > > It's triggering in testIPv4UDPEncapRecvTunnel() in xfrm_test.py. > Specifically, it's the self.ReceivePacketOn(netid, input_pkt) a dozen > lines higher. > > The packet we end up writing into the tap fd is > 02 00 00 00 C8 01 02 00 00 00 C8 00 08 00 > 45 00 00 44 00 01 00 00 40 11 98 96 08 08 08 08 0A 00 C8 02 > 11 94 BF 12 00 30 1C D0 > 00 00 12 34 00 00 00 01 > 45 00 00 20 00 01 00 00 40 11 98 BA 08 08 08 08 0A 00 C8 02 > 01 BB 7D 7B 00 0C 9B 7A > 01 02 02 11 > > You can decode this with https://hpd.gasmi.net/ or https://packetor.com/ > > You can decode the inner packet (this is null esp crypto) by passing in > 00 00 00 00 00 00 00 00 00 00 00 00 08 00 > 45 00 00 20 00 01 00 00 40 11 98 BA 08 08 08 08 0A 00 C8 02 > 01 BB 7D 7B 00 0C 9B 7A > 01 02 02 11 > instead. > > Note that the protocol the kernel's printk I added prints is the > *outer* encap UDP protocol, not the inner UDP. > ie. you can change the scapy.UDP to scapy.TCP in the 'inner_pkt =' assignment, > and the warning still triggers. The resulting packet is: > 02 00 00 00 FA 01 02 00 00 00 FA 00 08 00 > 45 00 00 50 00 01 00 00 40 11 66 8A 08 08 08 08 0A 00 FA 02 > 11 94 A7 EB 00 3C 33 E0 > 00 00 12 34 00 00 00 01 > 45 00 00 2C 00 01 00 00 40 06 66 B9 08 08 08 08 0A 00 FA 02 > 01 BB 7D 7B 00 00 00 00 00 00 00 00 50 02 20 00 F9 82 00 00 > 01 02 02 11 > > ie. the inner packet is IPv4/TCP: > 00 00 00 00 00 00 00 00 00 00 00 00 08 00 > 45 00 00 2C 00 01 00 00 40 06 66 B9 08 08 08 08 0A 00 FA 02 > 01 BB 7D 7B 00 00 00 00 00 00 00 00 50 02 20 00 F9 82 00 00 > 01 02 02 11 It looks like the problem is that final '11', and thus the fix is in the python test itself: https://android-review.googlesource.com/c/kernel/tests/+/2647762 - data += xfrm_base.GetEspTrailer(len(data), IPPROTO_UDP) + data += xfrm_base.GetEspTrailer(len(data), {4: IPPROTO_IPIP, 6: IPPROTO_IPV6}[version]) I guess it's OK that this WARN_ON is remotely triggerable?
On Sat, Jul 01, 2023 at 02:39:36PM +0200, Maciej Żenczykowski wrote: > > I guess it's OK that this WARN_ON is remotely triggerable? That's a good point. We should audit all the WARN_ONs in net/xfrm and get rid of the ones that can be triggered by a remote peer. Cheers,
> That's a good point. > > We should audit all the WARN_ONs in net/xfrm and get rid of the > ones that can be triggered by a remote peer. While I'm looking at this code... I noticed the comment: if mode == xfrm.XFRM_MODE_TRANSPORT: # Due to a bug in the IPv6 UDP encap code, there must be at least 32 # bytes after the ESP header or the packet will be dropped. # 8 (UDP header) + 18 (payload) + 2 (ESP trailer) = 28, dropped # 8 (UDP header) + 19 (payload) + 4 (ESP trailer) = 32, received # There is a similar bug in IPv4 encap, but the minimum is only 12 bytes, # which is much less likely to occur. This doesn't affect tunnel mode # because IP headers are always at least 20 bytes long. data = 19 * b"a" datalen = len(data) data += xfrm_base.GetEspTrailer(len(data), IPPROTO_UDP) and indeed reducing the 19 to 18 results in the test failing. (I'm guessing 'encap' in the comment really should be 'decap'...) I guess this means short IPv6/ESP transport/UDP fail? I'll dig a little deeper later...
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index 815b38080401..bd10605b211d 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -348,6 +348,7 @@ xfrm_inner_mode_encap_remove(struct xfrm_state *x, } } + printk(KERN_WARNING "xfrm_inner_mode_encap_remove: x->props.mode: %d XFRM_MODE_SKB_SB(skb)->protocol:%d\n", x->props.mode, XFRM_MODE_SKB_CB(skb)->protocol); WARN_ON_ONCE(1); return -EOPNOTSUPP; } @@ -375,6 +376,7 @@ static int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb) skb->protocol = htons(ETH_P_IPV6); break; default: + printk(KERN_WARNING "xfrm_prepare_input: XFRM_MODE_SKB_CB(skb)->protocol: %d\n", XFRM_MODE_SKB_CB(skb)->protocol); WARN_ON_ONCE(1); break; }