* [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices
@ 2022-05-12 10:48 Eyal Birger
2022-05-12 16:09 ` Nicolas Dichtel
0 siblings, 1 reply; 4+ messages in thread
From: Eyal Birger @ 2022-05-12 10:48 UTC (permalink / raw)
To: davem, yoshfuji, dsahern, edumazet, kuba, pabeni,
steffen.klassert, herbert
Cc: netdev, Eyal Birger, Shmulik Ladkani
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(-)
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;
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices
2022-05-12 10:48 [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices Eyal Birger
@ 2022-05-12 16:09 ` Nicolas Dichtel
2022-05-12 16:29 ` Eyal Birger
0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Dichtel @ 2022-05-12 16:09 UTC (permalink / raw)
To: Eyal Birger, davem, yoshfuji, dsahern, edumazet, kuba, pabeni,
steffen.klassert, herbert
Cc: netdev, Shmulik Ladkani
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices
2022-05-12 16:09 ` Nicolas Dichtel
@ 2022-05-12 16:29 ` Eyal Birger
2022-05-13 15:12 ` Nicolas Dichtel
0 siblings, 1 reply; 4+ messages in thread
From: Eyal Birger @ 2022-05-12 16:29 UTC (permalink / raw)
To: nicolas.dichtel
Cc: davem, yoshfuji, dsahern, edumazet, kuba, pabeni,
steffen.klassert, herbert, netdev, Shmulik Ladkani
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices
2022-05-12 16:29 ` Eyal Birger
@ 2022-05-13 15:12 ` Nicolas Dichtel
0 siblings, 0 replies; 4+ messages in thread
From: Nicolas Dichtel @ 2022-05-13 15:12 UTC (permalink / raw)
To: Eyal Birger
Cc: davem, yoshfuji, dsahern, edumazet, kuba, pabeni,
steffen.klassert, herbert, netdev, Shmulik Ladkani
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-13 15:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 10:48 [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices Eyal Birger
2022-05-12 16:09 ` Nicolas Dichtel
2022-05-12 16:29 ` Eyal Birger
2022-05-13 15:12 ` Nicolas Dichtel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.