| Message ID | 4b30e07300159db93ec0f6b31778aa0f6a41ef21.1698331320.git.antony.antony@secunet.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec-next,1/2] xfrm: introduce forwarding of ICMP Error messages | expand |
Hi Antony, kernel test robot noticed the following build warnings: [auto build test WARNING on klassert-ipsec-next/master] [also build test WARNING on klassert-ipsec/master net-next/main net/main linus/master v6.6-rc7 next-20231026] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Antony-Antony/xfrm-fix-source-address-in-icmp-error-generation-from-IPsec-gateway/20231026-234542 base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master patch link: https://lore.kernel.org/r/4b30e07300159db93ec0f6b31778aa0f6a41ef21.1698331320.git.antony.antony%40secunet.com patch subject: [PATCH ipsec-next 1/2] xfrm: introduce forwarding of ICMP Error messages config: csky-randconfig-002-20231027 (https://download.01.org/0day-ci/archive/20231027/202310270353.sobcrQay-lkp@intel.com/config) compiler: csky-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231027/202310270353.sobcrQay-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310270353.sobcrQay-lkp@intel.com/ All warnings (new ones prefixed by >>): net/xfrm/xfrm_policy.c: In function 'icmp_err_packet': >> net/xfrm/xfrm_policy.c:3490:30: warning: unused variable 'fl6' [-Wunused-variable] 3490 | const struct flowi6 *fl6 = &fl->u.ip6; | ^~~ vim +/fl6 +3490 net/xfrm/xfrm_policy.c 3487 3488 static bool icmp_err_packet(const struct flowi *fl, unsigned short family) 3489 { > 3490 const struct flowi6 *fl6 = &fl->u.ip6; 3491 const struct flowi4 *fl4 = &fl->u.ip4; 3492 3493 if (family == AF_INET && 3494 fl4->flowi4_proto == IPPROTO_ICMP && 3495 (fl4->fl4_icmp_type == ICMP_DEST_UNREACH || 3496 fl4->fl4_icmp_type == ICMP_TIME_EXCEEDED)) 3497 return true; 3498
Hello Antony Antony,
The patch 63b21caba17e: "xfrm: introduce forwarding of ICMP Error
messages" from Jan 19, 2024 (linux-next), leads to the following
Smatch static checker warning:
net/xfrm/xfrm_policy.c:3708 __xfrm_policy_check()
error: testing array offset 'dir' after use.
net/xfrm/xfrm_policy.c
3689
3690 pol = NULL;
3691 sk = sk_to_full_sk(sk);
3692 if (sk && sk->sk_policy[dir]) {
^^^^^^^^^^^^^^^^
If dir is XFRM_POLICY_FWD (2) then it is one element beyond the end of
the ->sk_policy[] array.
3693 pol = xfrm_sk_policy_lookup(sk, dir, &fl, family, if_id);
3694 if (IS_ERR(pol)) {
3695 XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLERROR);
3696 return 0;
3697 }
3698 }
3699
3700 if (!pol)
3701 pol = xfrm_policy_lookup(net, &fl, family, dir, if_id);
3702
3703 if (IS_ERR(pol)) {
3704 XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLERROR);
3705 return 0;
3706 }
3707
3708 if (!pol && dir == XFRM_POLICY_FWD)
^^^^^^^^^^^^^^^^^^^^^^
This assumes that dir can be 2.
3709 pol = xfrm_in_fwd_icmp(skb, &fl, family, if_id);
3710
3711 if (!pol) {
3712 if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) {
3713 XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
3714 return 0;
3715 }
3716
3717 if (sp && secpath_has_nontransport(sp, 0, &xerr_idx)) {
3718 xfrm_secpath_reject(xerr_idx, skb, &fl);
3719 XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
3720 return 0;
3721 }
3722 return 1;
regards,
dan carpenter
HI Dan, Thanks for reporting the warning. On Tue, Jan 30, 2024 at 01:36:28PM +0300, Dan Carpenter wrote: > > Hello Antony Antony, > > The patch 63b21caba17e: "xfrm: introduce forwarding of ICMP Error > messages" from Jan 19, 2024 (linux-next), leads to the following > Smatch static checker warning: > > net/xfrm/xfrm_policy.c:3708 __xfrm_policy_check() > error: testing array offset 'dir' after use. > > net/xfrm/xfrm_policy.c > 3689 > 3690 pol = NULL; > 3691 sk = sk_to_full_sk(sk); > 3692 if (sk && sk->sk_policy[dir]) { > ^^^^^^^^^^^^^^^^ > If dir is XFRM_POLICY_FWD (2) then it is one element beyond the end of > the ->sk_policy[] array. Yes, that's correct. However, for this patch, it's necessary that sk != NULL at the same time. As far as I know, there isn't any code that would call dir = XFRM_POLICY_FWD with sk != NULL. What am I missing? Did Smatch give any hints for such a code path? > > 3693 pol = xfrm_sk_policy_lookup(sk, dir, &fl, family, if_id); > 3694 if (IS_ERR(pol)) { > 3695 XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLERROR); > 3696 return 0; > 3697 } > 3698 } > 3699 > 3700 if (!pol) > 3701 pol = xfrm_policy_lookup(net, &fl, family, dir, if_id); > 3702 > 3703 if (IS_ERR(pol)) { > 3704 XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLERROR); > 3705 return 0; > 3706 } > 3707 > 3708 if (!pol && dir == XFRM_POLICY_FWD) > ^^^^^^^^^^^^^^^^^^^^^^ > This assumes that dir can be 2 Yes that is correct. However, this patch does not need sk != NULL at the same time. > 3709 pol = xfrm_in_fwd_icmp(skb, &fl, family, if_id); > 3710 > 3711 if (!pol) { > 3712 if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) { > 3713 XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS); > 3714 return 0; > 3715 } > 3716 > 3717 if (sp && secpath_has_nontransport(sp, 0, &xerr_idx)) { > 3718 xfrm_secpath_reject(xerr_idx, skb, &fl); > 3719 XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS); > 3720 return 0; > 3721 } > 3722 return 1; > > regards, > dan carpenter >
On Wed, Jan 31, 2024 at 08:38:51PM +0100, Antony Antony wrote: > HI Dan, > > Thanks for reporting the warning. > > On Tue, Jan 30, 2024 at 01:36:28PM +0300, Dan Carpenter wrote: > > > > Hello Antony Antony, > > > > The patch 63b21caba17e: "xfrm: introduce forwarding of ICMP Error > > messages" from Jan 19, 2024 (linux-next), leads to the following > > Smatch static checker warning: > > > > net/xfrm/xfrm_policy.c:3708 __xfrm_policy_check() > > error: testing array offset 'dir' after use. > > > > > net/xfrm/xfrm_policy.c > > 3689 > > 3690 pol = NULL; > > 3691 sk = sk_to_full_sk(sk); > > 3692 if (sk && sk->sk_policy[dir]) { > > ^^^^^^^^^^^^^^^^ > > If dir is XFRM_POLICY_FWD (2) then it is one element beyond the end of > > the ->sk_policy[] array. > > Yes, that's correct. However, for this patch, it's necessary that sk != NULL > at the same time. As far as I know, there isn't any code that would call dir > = XFRM_POLICY_FWD with sk != NULL. What am I missing? Did Smatch give any > hints for such a code path? > I wondered if that might be the case. The truth is that this sort of dependency is too compicated for any static analysis tools that currently exist. Smatch tries to track the relationship between "dir" and "sk" as they are passed in, but it will look the relationship information when we re-assign sk. "sk = sk_to_full_sk(sk);". So what we do in this case, is we just ignore the warning and if anyone has questions about it they will look up this conversation on lore.kernel.org to find the explanation. No need to worry about trying to silence the checker... regards, dan carpenter
On Wed, Jan 31, 2024 at 10:48:02PM +0300, Dan Carpenter wrote: > On Wed, Jan 31, 2024 at 08:38:51PM +0100, Antony Antony wrote: > > HI Dan, > > > > Thanks for reporting the warning. > > > > On Tue, Jan 30, 2024 at 01:36:28PM +0300, Dan Carpenter wrote: > > > > > > Hello Antony Antony, > > > > > > The patch 63b21caba17e: "xfrm: introduce forwarding of ICMP Error > > > messages" from Jan 19, 2024 (linux-next), leads to the following > > > Smatch static checker warning: > > > > > > net/xfrm/xfrm_policy.c:3708 __xfrm_policy_check() > > > error: testing array offset 'dir' after use. > > > > > > > > net/xfrm/xfrm_policy.c > > > 3689 > > > 3690 pol = NULL; > > > 3691 sk = sk_to_full_sk(sk); > > > 3692 if (sk && sk->sk_policy[dir]) { > > > ^^^^^^^^^^^^^^^^ > > > If dir is XFRM_POLICY_FWD (2) then it is one element beyond the end of > > > the ->sk_policy[] array. > > > > Yes, that's correct. However, for this patch, it's necessary that sk != NULL > > at the same time. As far as I know, there isn't any code that would call dir > > = XFRM_POLICY_FWD with sk != NULL. What am I missing? Did Smatch give any > > hints for such a code path? > > > > I wondered if that might be the case. The truth is that this sort of > dependency is too compicated for any static analysis tools that > currently exist. Smatch tries to track the relationship between > "dir" and "sk" as they are passed in, but it will look the relationship > information when we re-assign sk. "sk = sk_to_full_sk(sk);". s/look/lose/. I'm tired. I should go to bed. regards, dan carpenter
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index e8c406eba11b..683acc4ad94b 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -29,6 +29,7 @@ #include <linux/audit.h> #include <linux/rhashtable.h> #include <linux/if_tunnel.h> +#include <linux/icmp.h> #include <net/dst.h> #include <net/flow.h> #include <net/inet_ecn.h> @@ -3498,6 +3499,132 @@ static inline int secpath_has_nontransport(const struct sec_path *sp, int k, int return 0; } +static bool icmp_err_packet(const struct flowi *fl, unsigned short family) +{ + const struct flowi6 *fl6 = &fl->u.ip6; + const struct flowi4 *fl4 = &fl->u.ip4; + + if (family == AF_INET && + fl4->flowi4_proto == IPPROTO_ICMP && + (fl4->fl4_icmp_type == ICMP_DEST_UNREACH || + fl4->fl4_icmp_type == ICMP_TIME_EXCEEDED)) + return true; + +#if IS_ENABLED(CONFIG_IPV6) + if (family == AF_INET6 && + fl6->flowi6_proto == IPPROTO_ICMPV6 && + (fl6->fl6_icmp_type == ICMPV6_DEST_UNREACH || + fl6->fl6_icmp_type == ICMPV6_PKT_TOOBIG || + fl6->fl6_icmp_type == ICMPV6_TIME_EXCEED)) + return true; +#endif + return false; +} + +static struct sk_buff *xfrm_icmp_flow_decode(struct sk_buff *skb, + unsigned short family, + struct flowi *fl, + struct flowi *fl1) +{ + struct net *net = dev_net(skb->dev); + struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC); + int hl = family == AF_INET ? (sizeof(struct iphdr) + sizeof(struct icmphdr)) : + (sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr)); + skb_reset_network_header(newskb); + + if (!pskb_pull(newskb, hl)) + return NULL; + skb_reset_network_header(newskb); + + if (xfrm_decode_session_reverse(net, newskb, fl1, family) < 0) { + kfree_skb(newskb); + XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR); + return NULL; + } + + fl1->flowi_oif = fl->flowi_oif; + fl1->flowi_mark = fl->flowi_mark; + fl1->flowi_tos = fl->flowi_tos; + nf_nat_decode_session(newskb, fl1, family); + + return newskb; +} + +static bool xfrm_sa_icmp_flow(struct sk_buff *skb, + unsigned short family, const struct xfrm_selector *sel, + struct flowi *fl) +{ + bool ret = false; + + if (icmp_err_packet(fl, family)) { + struct flowi fl1; + struct sk_buff *newskb = xfrm_icmp_flow_decode(skb, family, fl, &fl1); + + if (!newskb) + return ret; + + ret = xfrm_selector_match(sel, &fl1, family); + kfree_skb(newskb); + } + + return ret; +} + +static inline struct +xfrm_policy *xfrm_in_fwd_icmp(struct sk_buff *skb, + struct flowi *fl, unsigned short family, + u32 if_id) +{ + struct xfrm_policy *pol = NULL; + + if (icmp_err_packet(fl, family)) { + struct flowi fl1; + struct net *net = dev_net(skb->dev); + struct sk_buff *newskb = xfrm_icmp_flow_decode(skb, family, fl, &fl1); + + if (!newskb) + return pol; + pol = xfrm_policy_lookup(net, &fl1, family, XFRM_POLICY_FWD, if_id); + + kfree_skb(newskb); + } + + return pol; +} + +static inline struct +dst_entry *xfrm_out_fwd_icmp(struct sk_buff *skb, struct flowi *fl, + unsigned short family, struct dst_entry *dst) +{ + if (icmp_err_packet(fl, family)) { + struct net *net = dev_net(skb->dev); + struct dst_entry *dst2; + struct flowi fl1; + struct sk_buff *newskb = xfrm_icmp_flow_decode(skb, family, fl, &fl1); + + if (!newskb) + return dst; + + dst_hold(dst); + + dst2 = xfrm_lookup(net, dst, &fl1, NULL, (XFRM_LOOKUP_QUEUE | XFRM_LOOKUP_ICMP)); + + kfree_skb(newskb); + + if (IS_ERR(dst2)) + return dst; + + if (dst2->xfrm) { + dst_release(dst); + dst = dst2; + } else { + dst_release(dst2); + } + } + + return dst; +} + int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, unsigned short family) { @@ -3544,9 +3671,17 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, for (i = sp->len - 1; i >= 0; i--) { struct xfrm_state *x = sp->xvec[i]; + int ret = 0; + if (!xfrm_selector_match(&x->sel, &fl, family)) { - XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMISMATCH); - return 0; + ret = true; + if (x->props.flags & XFRM_STATE_ICMP && + xfrm_sa_icmp_flow(skb, family, &x->sel, &fl)) + ret = false; + if (ret) { + XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMISMATCH); + return 0; + } } } } @@ -3569,6 +3704,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, return 0; } + if (!pol && dir == XFRM_POLICY_FWD) + pol = xfrm_in_fwd_icmp(skb, &fl, family, if_id); + if (!pol) { if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) { XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS); @@ -3702,6 +3840,10 @@ int __xfrm_route_forward(struct sk_buff *skb, unsigned short family) res = 0; dst = NULL; } + + if (dst && !dst->xfrm) + dst = xfrm_out_fwd_icmp(skb, &fl, family, dst); + skb_dst_set(skb, dst); return res; }
This commit aligns with RFC 4301, Section 6, and addresses the requirement to forward unauthenticated ICMP error messages that do not match any xfrm policies. It utilizes the ICMP payload as an skb and performs a reverse lookup. If a policy match is found, forward the packet. The ICMP payload typically contains a partial IP packet that is likely responsible for the error message. The following error types will be forwarded: - IPv4 ICMP error types: ICMP_DEST_UNREACH & ICMP_TIME_EXCEEDED - IPv6 ICMPv6 error types: ICMPV6_DEST_UNREACH, ICMPV6_PKT_TOOBIG, ICMPV6_TIME_EXCEED To implement this feature, a reverse lookup has been added to the xfrm forward path, making use of the ICMP payload as the skb. To enable this functionality from user space, the XFRM_POLICY_ICMP flag should be added to the outgoing and forward policies, and the XFRM_STATE_ICMP flag should be set on incoming states. e.g. ip xfrm policy add flag icmp tmpl ip xfrm policy src 192.0.2.0/24 dst 192.0.1.0/25 dir out priority 2084302 ptype main flag icmp ip xfrm state add ...flag icmp ip xfrm state root@west:~#ip x s src 192.1.2.23 dst 192.1.2.45 proto esp spi 0xa7b76872 reqid 16389 mode tunnel replay-window 32 flag icmp af-unspec Signed-off-by: Antony Antony <antony.antony@secunet.com> --- net/xfrm/xfrm_policy.c | 146 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 144 insertions(+), 2 deletions(-)