| Message ID | 20220512104831.976553-1-eyal.birger@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices | expand |
Le 12/05/2022 à 12:48, Eyal Birger a écrit : > In IPv4 setting the "disable_policy" flag on a device means no policy > should be enforced for traffic originating from the device. This was > implemented by seting the DST_NOPOLICY flag in the dst based on the > originating device. > > However, dsts are cached in nexthops regardless of the originating > devices, in which case, the DST_NOPOLICY flag value may be incorrect. > > Consider the following setup: > > +------------------------------+ > | ROUTER | > +-------------+ | +-----------------+ | > | ipsec src |----|-|ipsec0 | | > +-------------+ | |disable_policy=0 | +----+ | > | +-----------------+ |eth1|-|----- > +-------------+ | +-----------------+ +----+ | > | noipsec src |----|-|eth0 | | > +-------------+ | |disable_policy=1 | | > | +-----------------+ | > +------------------------------+ > > Where ROUTER has a default route towards eth1. > > dst entries for traffic arriving from eth0 would have DST_NOPOLICY > and would be cached and therefore can be reused by traffic originating > from ipsec0, skipping policy check. > > Fix by setting a IPSKB_NOPOLICY flag in IPCB and observing it instead > of the DST in IN/FWD IPv4 policy checks. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Reported-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> > Signed-off-by: Eyal Birger <eyal.birger@gmail.com> > --- [snip] > @@ -1852,8 +1856,7 @@ static int __mkroute_input(struct sk_buff *skb, > } > } > > - rth = rt_dst_alloc(out_dev->dev, 0, res->type, > - IN_DEV_ORCONF(in_dev, NOPOLICY), > + rth = rt_dst_alloc(out_dev->dev, 0, res->type, no_policy,> IN_DEV_ORCONF(out_dev, NOXFRM)); no_policy / DST_NOPOLICY is still needed in the dst entry after this patch?
On Thu, May 12, 2022 at 7:09 PM Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > > > Le 12/05/2022 à 12:48, Eyal Birger a écrit : > > In IPv4 setting the "disable_policy" flag on a device means no policy > > should be enforced for traffic originating from the device. This was > > implemented by seting the DST_NOPOLICY flag in the dst based on the > > originating device. > > > > However, dsts are cached in nexthops regardless of the originating > > devices, in which case, the DST_NOPOLICY flag value may be incorrect. > > > > Consider the following setup: > > > > +------------------------------+ > > | ROUTER | > > +-------------+ | +-----------------+ | > > | ipsec src |----|-|ipsec0 | | > > +-------------+ | |disable_policy=0 | +----+ | > > | +-----------------+ |eth1|-|----- > > +-------------+ | +-----------------+ +----+ | > > | noipsec src |----|-|eth0 | | > > +-------------+ | |disable_policy=1 | | > > | +-----------------+ | > > +------------------------------+ > > > > Where ROUTER has a default route towards eth1. > > > > dst entries for traffic arriving from eth0 would have DST_NOPOLICY > > and would be cached and therefore can be reused by traffic originating > > from ipsec0, skipping policy check. > > > > Fix by setting a IPSKB_NOPOLICY flag in IPCB and observing it instead > > of the DST in IN/FWD IPv4 policy checks. > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Reported-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> > > Signed-off-by: Eyal Birger <eyal.birger@gmail.com> > > --- > > [snip] > > > @@ -1852,8 +1856,7 @@ static int __mkroute_input(struct sk_buff *skb, > > } > > } > > > > - rth = rt_dst_alloc(out_dev->dev, 0, res->type, > > - IN_DEV_ORCONF(in_dev, NOPOLICY), > > + rth = rt_dst_alloc(out_dev->dev, 0, res->type, no_policy,> IN_DEV_ORCONF(out_dev, NOXFRM)); > no_policy / DST_NOPOLICY is still needed in the dst entry after this patch? I see it's being set in the outbound direction in IPv4 - though I don't see where it's actually used in that direction. Maybe it could be cleaned up as a follow up, but I wanted to scope this patch to the bugfix. Eyal.
Le 12/05/2022 à 18:29, Eyal Birger a écrit : > On Thu, May 12, 2022 at 7:09 PM Nicolas Dichtel > <nicolas.dichtel@6wind.com> wrote: >> >> >> Le 12/05/2022 à 12:48, Eyal Birger a écrit : >>> In IPv4 setting the "disable_policy" flag on a device means no policy >>> should be enforced for traffic originating from the device. This was >>> implemented by seting the DST_NOPOLICY flag in the dst based on the >>> originating device. >>> >>> However, dsts are cached in nexthops regardless of the originating >>> devices, in which case, the DST_NOPOLICY flag value may be incorrect. >>> >>> Consider the following setup: >>> >>> +------------------------------+ >>> | ROUTER | >>> +-------------+ | +-----------------+ | >>> | ipsec src |----|-|ipsec0 | | >>> +-------------+ | |disable_policy=0 | +----+ | >>> | +-----------------+ |eth1|-|----- >>> +-------------+ | +-----------------+ +----+ | >>> | noipsec src |----|-|eth0 | | >>> +-------------+ | |disable_policy=1 | | >>> | +-----------------+ | >>> +------------------------------+ >>> >>> Where ROUTER has a default route towards eth1. >>> >>> dst entries for traffic arriving from eth0 would have DST_NOPOLICY >>> and would be cached and therefore can be reused by traffic originating >>> from ipsec0, skipping policy check. >>> >>> Fix by setting a IPSKB_NOPOLICY flag in IPCB and observing it instead >>> of the DST in IN/FWD IPv4 policy checks. >>> >>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >>> Reported-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> >>> Signed-off-by: Eyal Birger <eyal.birger@gmail.com> >>> --- >> >> [snip] >> >>> @@ -1852,8 +1856,7 @@ static int __mkroute_input(struct sk_buff *skb, >>> } >>> } >>> >>> - rth = rt_dst_alloc(out_dev->dev, 0, res->type, >>> - IN_DEV_ORCONF(in_dev, NOPOLICY), >>> + rth = rt_dst_alloc(out_dev->dev, 0, res->type, no_policy,> IN_DEV_ORCONF(out_dev, NOXFRM)); >> no_policy / DST_NOPOLICY is still needed in the dst entry after this patch? > > I see it's being set in the outbound direction in IPv4 - though I don't see > where it's actually used in that direction. > > Maybe it could be cleaned up as a follow up, but I wanted to scope this > patch to the bugfix. Sure, the cleanup should be put in a separate patch. Removing the flag for ipv4 could help to avoid ambiguity. Thank you, Nicolas
diff --git a/include/net/ip.h b/include/net/ip.h index 3984f2c39c4b..0161137914cf 100644 --- a/include/net/ip.h +++ b/include/net/ip.h @@ -56,6 +56,7 @@ struct inet_skb_parm { #define IPSKB_DOREDIRECT BIT(5) #define IPSKB_FRAG_PMTU BIT(6) #define IPSKB_L3SLAVE BIT(7) +#define IPSKB_NOPOLICY BIT(8) u16 frag_max_size; }; diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 6fb899ff5afc..d2efddce65d4 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1093,6 +1093,18 @@ static inline bool __xfrm_check_nopolicy(struct net *net, struct sk_buff *skb, return false; } +static inline bool __xfrm_check_dev_nopolicy(struct sk_buff *skb, + int dir, unsigned short family) +{ + if (dir != XFRM_POLICY_OUT && family == AF_INET) { + /* same dst may be used for traffic originating from + * devices with different policy settings. + */ + return IPCB(skb)->flags & IPSKB_NOPOLICY; + } + return skb_dst(skb) && (skb_dst(skb)->flags & DST_NOPOLICY); +} + static inline int __xfrm_policy_check2(struct sock *sk, int dir, struct sk_buff *skb, unsigned int family, int reverse) @@ -1104,7 +1116,7 @@ static inline int __xfrm_policy_check2(struct sock *sk, int dir, return __xfrm_policy_check(sk, ndir, skb, family); return __xfrm_check_nopolicy(net, skb, dir) || - (skb_dst(skb) && (skb_dst(skb)->flags & DST_NOPOLICY)) || + __xfrm_check_dev_nopolicy(skb, dir, family) || __xfrm_policy_check(sk, ndir, skb, family); } diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 98c6f3429593..ea81e93c8c20 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -1795,7 +1795,7 @@ static int __mkroute_input(struct sk_buff *skb, struct rtable *rth; int err; struct in_device *out_dev; - bool do_cache; + bool do_cache, no_policy; u32 itag = 0; /* get a working reference to the output device */ @@ -1840,6 +1840,10 @@ static int __mkroute_input(struct sk_buff *skb, } } + no_policy = IN_DEV_ORCONF(in_dev, NOPOLICY); + if (no_policy) + IPCB(skb)->flags |= IPSKB_NOPOLICY; + fnhe = find_exception(nhc, daddr); if (do_cache) { if (fnhe) @@ -1852,8 +1856,7 @@ static int __mkroute_input(struct sk_buff *skb, } } - rth = rt_dst_alloc(out_dev->dev, 0, res->type, - IN_DEV_ORCONF(in_dev, NOPOLICY), + rth = rt_dst_alloc(out_dev->dev, 0, res->type, no_policy, IN_DEV_ORCONF(out_dev, NOXFRM)); if (!rth) { err = -ENOBUFS; @@ -2228,6 +2231,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, struct rtable *rth; struct flowi4 fl4; bool do_cache = true; + bool no_policy; /* IP on this device is disabled. */ @@ -2346,6 +2350,10 @@ out: return err; RT_CACHE_STAT_INC(in_brd); local_input: + no_policy = IN_DEV_ORCONF(in_dev, NOPOLICY); + if (no_policy) + IPCB(skb)->flags |= IPSKB_NOPOLICY; + do_cache &= res->fi && !itag; if (do_cache) { struct fib_nh_common *nhc = FIB_RES_NHC(*res); @@ -2360,7 +2368,7 @@ out: return err; rth = rt_dst_alloc(ip_rt_get_dev(net, res), flags | RTCF_LOCAL, res->type, - IN_DEV_ORCONF(in_dev, NOPOLICY), false); + no_policy, false); if (!rth) goto e_nobufs;
In IPv4 setting the "disable_policy" flag on a device means no policy should be enforced for traffic originating from the device. This was implemented by seting the DST_NOPOLICY flag in the dst based on the originating device. However, dsts are cached in nexthops regardless of the originating devices, in which case, the DST_NOPOLICY flag value may be incorrect. Consider the following setup: +------------------------------+ | ROUTER | +-------------+ | +-----------------+ | | ipsec src |----|-|ipsec0 | | +-------------+ | |disable_policy=0 | +----+ | | +-----------------+ |eth1|-|----- +-------------+ | +-----------------+ +----+ | | noipsec src |----|-|eth0 | | +-------------+ | |disable_policy=1 | | | +-----------------+ | +------------------------------+ Where ROUTER has a default route towards eth1. dst entries for traffic arriving from eth0 would have DST_NOPOLICY and would be cached and therefore can be reused by traffic originating from ipsec0, skipping policy check. Fix by setting a IPSKB_NOPOLICY flag in IPCB and observing it instead of the DST in IN/FWD IPv4 policy checks. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> Signed-off-by: Eyal Birger <eyal.birger@gmail.com> --- include/net/ip.h | 1 + include/net/xfrm.h | 14 +++++++++++++- net/ipv4/route.c | 16 ++++++++++++---- 3 files changed, 26 insertions(+), 5 deletions(-)