| Message ID | 20231026144610.26347-1-fw@strlen.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec-next,v2] xfrm: policy: fix layer 4 flowi decoding | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Thu, Oct 26, 2023 at 04:45:42PM +0200, Florian Westphal wrote: > The commit shipped with two bugs: > fl4->fl4_icmp_type = flkeys->icmp.type; > fl4->fl4_icmp_type = flkeys->icmp.code; > ~~~~ should have been "code". > > But the more severe bug is that I got fooled by flowi member defines: > fl4_icmp_type, fl4_gre_key and fl4_dport share the same union/address. > > Fix typo and make gre/icmp key setting depend on the l4 protocol. > > Fixes: 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector") > Reported-and-tested-by: Antony Antony <antony@phenome.org> > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > v2: decode_session6 must use IPPROTO_ICMPV6, not IPPROTO_ICMP. > > I normally do not resend immediately but in this case it seems like the > lesser evil. This was indeed the better way to do it. Applied, thanks a lot!
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 6aea8b2f45e0..d2dddc570f4f 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -3400,11 +3400,18 @@ decode_session4(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl4->fl4_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl4->fl4_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMP: + fl4->fl4_icmp_type = flkeys->icmp.type; + fl4->fl4_icmp_code = flkeys->icmp.code; + break; + } + fl4->flowi4_proto = flkeys->basic.ip_proto; fl4->flowi4_tos = flkeys->ip.tos; - fl4->fl4_icmp_type = flkeys->icmp.type; - fl4->fl4_icmp_type = flkeys->icmp.code; - fl4->fl4_gre_key = flkeys->gre.keyid; } #if IS_ENABLED(CONFIG_IPV6) @@ -3427,10 +3434,17 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl6->fl6_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl6->fl6_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMPV6: + fl6->fl6_icmp_type = flkeys->icmp.type; + fl6->fl6_icmp_code = flkeys->icmp.code; + break; + } + fl6->flowi6_proto = flkeys->basic.ip_proto; - fl6->fl6_icmp_type = flkeys->icmp.type; - fl6->fl6_icmp_type = flkeys->icmp.code; - fl6->fl6_gre_key = flkeys->gre.keyid; } #endif
The commit shipped with two bugs: fl4->fl4_icmp_type = flkeys->icmp.type; fl4->fl4_icmp_type = flkeys->icmp.code; ~~~~ should have been "code". But the more severe bug is that I got fooled by flowi member defines: fl4_icmp_type, fl4_gre_key and fl4_dport share the same union/address. Fix typo and make gre/icmp key setting depend on the l4 protocol. Fixes: 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector") Reported-and-tested-by: Antony Antony <antony@phenome.org> Signed-off-by: Florian Westphal <fw@strlen.de> --- v2: decode_session6 must use IPPROTO_ICMPV6, not IPPROTO_ICMP. I normally do not resend immediately but in this case it seems like the lesser evil. Alternative is to defer this until after -next merges have completed and then handle it via ipsec.git. net/xfrm/xfrm_policy.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)