| Message ID | a53333717022906933e9113980304fa4717118e9.1712320696.git.antony.antony@secunet.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec-next,v6] xfrm: Add Direction to the SA in or out | expand |
Le 05/04/2024 à 14:40, Antony Antony a écrit : > This patch introduces the 'dir' attribute, 'in' or 'out', to the > xfrm_state, SA, enhancing usability by delineating the scope of values > based on direction. An input SA will now exclusively encompass values > pertinent to input, effectively segregating them from output-related > values. This change aims to streamline the configuration process and > improve the overall clarity of SA attributes. > > This feature sets the groundwork for future patches, including > the upcoming IP-TFS patch. > > Currently, dir is only allowed when HW OFFLOAD is set. > > --- > v5->v6: > - XFRMA_SA_DIR only allowed with HW OFFLOAD > > v4->v5: > - add details to commit message > > v3->v4: > - improve HW OFFLOAD DIR check check other direction > > v2->v3: > - delete redundant XFRM_SA_DIR_USE > - use u8 for "dir" > - fix HW OFFLOAD DIR check > > v1->v2: > - use .strict_start_type in struct nla_policy xfrma_policy > - delete redundant XFRM_SA_DIR_MAX enum > --- > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Hi Antony, 2024-04-05, 14:40:07 +0200, Antony Antony wrote: > This patch introduces the 'dir' attribute, 'in' or 'out', to the > xfrm_state, SA, enhancing usability by delineating the scope of values > based on direction. An input SA will now exclusively encompass values > pertinent to input, effectively segregating them from output-related > values. But this patch isn't doing that for existing properties (I'm thinking of replay window, not sure if any others are relevant [1]). Why not? [1] that should include values passed via xfrm_usersa_info too, not just XFRMA_* attributes Adding these checks should be safe (wrt breakage of API): Old software would not be passing XFRMA_SA_DIR, so adding checks when it is provided would not break anything there. Only new software using the attribute would benefit from having directed SAs and restriction on which attributes can be used (and that's fine). Right now the new attribute is 100% duplicate of the existing offload direction, so I don't see much point. > This change aims to streamline the configuration process and > improve the overall clarity of SA attributes. > > This feature sets the groundwork for future patches, including > the upcoming IP-TFS patch. > > Currently, dir is only allowed when HW OFFLOAD is set. > > --- BTW, everything after this '---' will get cut, including your sign-off. > v5->v6: > - XFRMA_SA_DIR only allowed with HW OFFLOAD > > v4->v5: > - add details to commit message > > v3->v4: > - improve HW OFFLOAD DIR check check other direction > > v2->v3: > - delete redundant XFRM_SA_DIR_USE > - use u8 for "dir" > - fix HW OFFLOAD DIR check > > v1->v2: > - use .strict_start_type in struct nla_policy xfrma_policy > - delete redundant XFRM_SA_DIR_MAX enum > --- > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> nit: If I'm making non-trivial changes to the contents of the patch, I typically drop the review (and test) tags I got on previous versions, since they may no longer apply.
> On Apr 5, 2024, at 17:56, Sabrina Dubroca via Devel <devel@linux-ipsec.org> wrote: > > Hi Antony, > > 2024-04-05, 14:40:07 +0200, Antony Antony wrote: >> This patch introduces the 'dir' attribute, 'in' or 'out', to the >> xfrm_state, SA, enhancing usability by delineating the scope of values >> based on direction. An input SA will now exclusively encompass values >> pertinent to input, effectively segregating them from output-related >> values. > > But this patch isn't doing that for existing properties (I'm thinking > of replay window, not sure if any others are relevant [1]). Why not? > > [1] that should include values passed via xfrm_usersa_info too, > not just XFRMA_* attributes > > Adding these checks should be safe (wrt breakage of API): Old software > would not be passing XFRMA_SA_DIR, so adding checks when it is provided > would not break anything there. Only new software using the attribute > would benefit from having directed SAs and restriction on which attributes > can be used (and that's fine). > > Right now the new attribute is 100% duplicate of the existing offload > direction, so I don't see much point. A subset of the ipsec has requested that IPTFS utilize this new directional attribute as most of IPTFS config is send-only. So IPTFS is the planned first user and is also now blocked on this patch. Chris. > >> This change aims to streamline the configuration process and >> improve the overall clarity of SA attributes. >> >> This feature sets the groundwork for future patches, including >> the upcoming IP-TFS patch. >> >> Currently, dir is only allowed when HW OFFLOAD is set. >> >> --- > > BTW, everything after this '---' will get cut, including your sign-off. > >> v5->v6: >> - XFRMA_SA_DIR only allowed with HW OFFLOAD >> >> v4->v5: >> - add details to commit message >> >> v3->v4: >> - improve HW OFFLOAD DIR check check other direction >> >> v2->v3: >> - delete redundant XFRM_SA_DIR_USE >> - use u8 for "dir" >> - fix HW OFFLOAD DIR check >> >> v1->v2: >> - use .strict_start_type in struct nla_policy xfrma_policy >> - delete redundant XFRM_SA_DIR_MAX enum >> --- >> >> Signed-off-by: Antony Antony <antony.antony@secunet.com> >> Reviewed-by: Leon Romanovsky <leonro@nvidia.com> > > nit: If I'm making non-trivial changes to the contents of the patch, I > typically drop the review (and test) tags I got on previous versions, > since they may no longer apply. > > -- > Sabrina > > -- > Devel mailing list > Devel@linux-ipsec.org > https://linux-ipsec.org/mailman/listinfo/devel >
Hi Sabrina, On Fri, Apr 05, 2024 at 11:56:00PM +0200, Sabrina Dubroca via Devel wrote: > Hi Antony, > > 2024-04-05, 14:40:07 +0200, Antony Antony wrote: > > This patch introduces the 'dir' attribute, 'in' or 'out', to the > > xfrm_state, SA, enhancing usability by delineating the scope of values > > based on direction. An input SA will now exclusively encompass values > > pertinent to input, effectively segregating them from output-related > > values. > > But this patch isn't doing that for existing properties (I'm thinking > of replay window, not sure if any others are relevant [1]). Why not? Thank you for raising this point. I thought that introducing a patch for the replay window check could stir more controversy, which might delay the acceptance of the essential 'dir' feature. My primary goal at this stage is to get this basic feature in and to convince Chris to integrate the "dir" attribute into IP-TFS. This patch has partly contributed to the delays in IP-TFS's development. Given your input, I'm curious about the specific conditions you have in mind. See the attached patch. For non-ESN scenarios, the outbound SA should have a replay window set to 0? And for ESN 1? non-ESN ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 \ mode tunnel dir out aead 'rfc4106(gcm(aes))' \ 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10 Error: Replay-window too big > 0 for OUT SA. The current impelementation does not replay window 0 with ESN. Even though disabling replay window with ESN is a desired feature. ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 mode tunnel dir out flag esn aead 'rfc4106(gcm(aes))' \ 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10 Error: ESN replay-window too big > 1 for OUT SA.ww I wonder would the attached patch get accepted quickly. > > [1] that should include values passed via xfrm_usersa_info too, > not just XFRMA_* attributes > > Adding these checks should be safe (wrt breakage of API): Old software > would not be passing XFRMA_SA_DIR, so adding checks when it is provided > would not break anything there. Only new software using the attribute > would benefit from having directed SAs and restriction on which attributes > can be used (and that's fine). > > Right now the new attribute is 100% duplicate of the existing offload > direction, so I don't see much point. IP-TFS and Chris alreay expressed it. It started with this e-mail. https://lore.kernel.org/netdev/ZV0BSBzNh3UIqueZ@Antony2201.local I am trying to convince Chris to use "dir". Without direction I found IP-TFS confusing without direction. > > This change aims to streamline the configuration process and > > improve the overall clarity of SA attributes. > > > > This feature sets the groundwork for future patches, including > > the upcoming IP-TFS patch. > > > > Currently, dir is only allowed when HW OFFLOAD is set. > > > > --- > > BTW, everything after this '---' will get cut, including your sign-off. thanks for spotting it. I will send new version. > > > v5->v6: > > - XFRMA_SA_DIR only allowed with HW OFFLOAD > > > > v4->v5: > > - add details to commit message > > > > v3->v4: > > - improve HW OFFLOAD DIR check check other direction > > > > v2->v3: > > - delete redundant XFRM_SA_DIR_USE > > - use u8 for "dir" > > - fix HW OFFLOAD DIR check > > > > v1->v2: > > - use .strict_start_type in struct nla_policy xfrma_policy > > - delete redundant XFRM_SA_DIR_MAX enum > > --- > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> > > nit: If I'm making non-trivial changes to the contents of the patch, I > typically drop the review (and test) tags I got on previous versions, > since they may no longer apply. I agree. -antony >From c4b7a7232aab0adefd138170391cbe0532216642 Mon Sep 17 00:00:00 2001 From: Antony Antony <antony.antony@secunet.com> Date: Fri, 5 Apr 2024 14:40:07 +0200 Subject: [PATCH ipsec-next v7] xfrm: Add Direction to the SA in or out This patch introduces the 'dir' attribute, 'in' or 'out', to the xfrm_state, SA, enhancing usability by delineating the scope of values based on direction. An input SA will now exclusively encompass values pertinent to input, effectively segregating them from output-related values. This change aims to streamline the configuration process and improve the overall clarity of SA attributes. This feature sets the groundwork for future patches, including the upcoming IP-TFS patch. Signed-off-by: Antony Antony <antony.antony@secunet.com> --- v6->v7: - add replay-window check non-esn 0 and ESN 1. - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD v5->v6: - XFRMA_SA_DIR only allowed with HW OFFLOAD v4->v5: - add details to commit message v3->v4: - improve HW OFFLOAD DIR check add the other direction v2->v3: - delete redundant XFRM_SA_DIR_USE - use u8 for "dir" - fix HW OFFLOAD DIR check v1->v2: - use .strict_start_type in struct nla_policy xfrma_policy - delete redundant XFRM_SA_DIR_MAX enum --- include/net/xfrm.h | 1 + include/uapi/linux/xfrm.h | 6 ++++ net/xfrm/xfrm_compat.c | 7 +++-- net/xfrm/xfrm_device.c | 6 ++++ net/xfrm/xfrm_state.c | 1 + net/xfrm/xfrm_user.c | 66 ++++++++++++++++++++++++++++++++++++--- 6 files changed, 81 insertions(+), 6 deletions(-) diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 57c743b7e4fe..7c9be06f8302 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -291,6 +291,7 @@ struct xfrm_state { /* Private data of this transformer, format is opaque, * interpreted by xfrm_type methods. */ void *data; + u8 dir; }; static inline struct net *xs_net(struct xfrm_state *x) diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h index 6a77328be114..18ceaba8486e 100644 --- a/include/uapi/linux/xfrm.h +++ b/include/uapi/linux/xfrm.h @@ -141,6 +141,11 @@ enum { XFRM_POLICY_MAX = 3 }; +enum xfrm_sa_dir { + XFRM_SA_DIR_IN = 1, + XFRM_SA_DIR_OUT = 2 +}; + enum { XFRM_SHARE_ANY, /* No limitations */ XFRM_SHARE_SESSION, /* For this session only */ @@ -315,6 +320,7 @@ enum xfrm_attr_type_t { XFRMA_SET_MARK_MASK, /* __u32 */ XFRMA_IF_ID, /* __u32 */ XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */ + XFRMA_SA_DIR, /* __u8 */ __XFRMA_MAX #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */ diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c index 655fe4ff8621..007dee03b1bc 100644 --- a/net/xfrm/xfrm_compat.c +++ b/net/xfrm/xfrm_compat.c @@ -98,6 +98,7 @@ static const int compat_msg_min[XFRM_NR_MSGTYPES] = { }; static const struct nla_policy compat_policy[XFRMA_MAX+1] = { + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR }, [XFRMA_SA] = { .len = XMSGSIZE(compat_xfrm_usersa_info)}, [XFRMA_POLICY] = { .len = XMSGSIZE(compat_xfrm_userpolicy_info)}, [XFRMA_LASTUSED] = { .type = NLA_U64}, @@ -129,6 +130,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = { [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 }, [XFRMA_IF_ID] = { .type = NLA_U32 }, [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 }, + [XFRMA_SA_DIR] = { .type = NLA_U8} }; static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb, @@ -277,9 +279,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src) case XFRMA_SET_MARK_MASK: case XFRMA_IF_ID: case XFRMA_MTIMER_THRESH: + case XFRMA_SA_DIR: return xfrm_nla_cpy(dst, src, nla_len(src)); default: - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH); + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR); pr_warn_once("unsupported nla_type %d\n", src->nla_type); return -EOPNOTSUPP; } @@ -434,7 +437,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla, int err; if (type > XFRMA_MAX) { - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH); + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR); NL_SET_ERR_MSG(extack, "Bad attribute"); return -EOPNOTSUPP; } diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index 6346690d5c69..2455a76a1cff 100644 --- a/net/xfrm/xfrm_device.c +++ b/net/xfrm/xfrm_device.c @@ -253,6 +253,12 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x, return -EINVAL; } + if ((xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir == XFRM_SA_DIR_OUT) || + (!(xuo->flags & XFRM_OFFLOAD_INBOUND) && x->dir == XFRM_SA_DIR_IN)) { + NL_SET_ERR_MSG(extack, "Mismatched SA and offload direction"); + return -EINVAL; + } + is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET; /* We don't yet support UDP encapsulation and TFC padding. */ diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 0c306473a79d..749c9cf41c06 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, x->lastused = orig->lastused; x->new_mapping = 0; x->new_mapping_sport = 0; + x->dir = orig->dir; return x; diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 810b520493f3..c86853ef9e59 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -360,6 +360,36 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, } } + if (attrs[XFRMA_SA_DIR]) { + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]); + + if (sa_dir != XFRM_SA_DIR_IN && sa_dir != XFRM_SA_DIR_OUT) { + NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range"); + err = -EINVAL; + goto out; + } + + if (sa_dir == XFRM_SA_DIR_OUT) { + if (p->replay_window > 0) { + NL_SET_ERR_MSG(extack, "Replay-window too big > 0 for OUT SA"); + err = -EINVAL; + goto out; + } + + if (attrs[XFRMA_REPLAY_ESN_VAL]) { + struct xfrm_replay_state_esn *esn; + + esn = nla_data(attrs[XFRMA_REPLAY_ESN_VAL]); + if (esn->replay_window > 1) { + NL_SET_ERR_MSG(extack, + "ESN replay-window too big > 1 for OUT SA"); + err = -EINVAL; + goto out; + } + } + } + } + out: return err; } @@ -627,6 +657,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs, struct nlattr *et = attrs[XFRMA_ETIMER_THRESH]; struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH]; struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH]; + struct nlattr *dir = attrs[XFRMA_SA_DIR]; if (re && x->replay_esn && x->preplay_esn) { struct xfrm_replay_state_esn *replay_esn; @@ -661,6 +692,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs, if (mt) x->mapping_maxage = nla_get_u32(mt); + + if (dir) + x->dir = nla_get_u8(dir); } static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m) @@ -1182,8 +1216,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x, if (ret) goto out; } - if (x->mapping_maxage) + if (x->mapping_maxage) { ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage); + if (ret) + goto out; + } + if (x->dir) + ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); out: return ret; } @@ -2402,7 +2441,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x) + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur)) + nla_total_size(sizeof(struct xfrm_mark)) + nla_total_size(4) /* XFRM_AE_RTHR */ - + nla_total_size(4); /* XFRM_AE_ETHR */ + + nla_total_size(4) /* XFRM_AE_ETHR */ + + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */ } static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c) @@ -2459,6 +2499,12 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct if (err) goto out_cancel; + if (x->dir) { + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); + if (err) + goto out_cancel; + } + nlmsg_end(skb, nlh); return 0; @@ -3018,6 +3064,7 @@ EXPORT_SYMBOL_GPL(xfrm_msg_min); #undef XMSGSIZE const struct nla_policy xfrma_policy[XFRMA_MAX+1] = { + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR }, [XFRMA_SA] = { .len = sizeof(struct xfrm_usersa_info)}, [XFRMA_POLICY] = { .len = sizeof(struct xfrm_userpolicy_info)}, [XFRMA_LASTUSED] = { .type = NLA_U64}, @@ -3049,6 +3096,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = { [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 }, [XFRMA_IF_ID] = { .type = NLA_U32 }, [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 }, + [XFRMA_SA_DIR] = { .type = NLA_U8 } }; EXPORT_SYMBOL_GPL(xfrma_policy); @@ -3189,8 +3237,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb) static inline unsigned int xfrm_expire_msgsize(void) { - return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) - + nla_total_size(sizeof(struct xfrm_mark)); + return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) + + nla_total_size(sizeof(struct xfrm_mark)) + + nla_total_size(sizeof_field(struct xfrm_state, dir)); } static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c) @@ -3217,6 +3266,12 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct if (err) return err; + if (x->dir) { + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); + if (err) + return err; + } + nlmsg_end(skb, nlh); return 0; } @@ -3324,6 +3379,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x) if (x->mapping_maxage) l += nla_total_size(sizeof(x->mapping_maxage)); + if (x->dir) + l += nla_total_size(sizeof(x->dir)); + return l; } -- 2.43.0
2024-04-07, 10:23:21 +0200, Antony Antony wrote: > Hi Sabrina, > > On Fri, Apr 05, 2024 at 11:56:00PM +0200, Sabrina Dubroca via Devel wrote: > > Hi Antony, > > > > 2024-04-05, 14:40:07 +0200, Antony Antony wrote: > > > This patch introduces the 'dir' attribute, 'in' or 'out', to the > > > xfrm_state, SA, enhancing usability by delineating the scope of values > > > based on direction. An input SA will now exclusively encompass values > > > pertinent to input, effectively segregating them from output-related > > > values. > > > > But this patch isn't doing that for existing properties (I'm thinking > > of replay window, not sure if any others are relevant [1]). Why not? > > Thank you for raising this point. I thought that introducing a patch for > the replay window check could stir more controversy, which might delay the > acceptance of the essential 'dir' feature. I'm not convinced it's *that* critical. People have someone managed to use IPsec without it for all those years. Is the intention to only allow setting up IPTFS SAs when this new 'dir' attribute is provided? If not, then this patch is not really blocking for IPTFS. And yes, people will sometimes make comments on patches that cause delays in getting the patches accepted. Some patches even end up getting rejected. The kernel is better thanks to that process, even if it can be annoying to the submitter (including me! it would be a lot more relaxing if my patches always just went in at v1 :)). Nicolas, since you were objecting to the informational nature of the attribute in v5: would you still object to the new attribute (and not just limited to offload cases) if it properly restricted attributes that don't match the direction? > My primary goal at this stage is > to get this basic feature in and to convince Chris to integrate the "dir" > attribute into IP-TFS. This patch has partly contributed to the delays in > IP-TFS's development. > > Given your input, I'm curious about the specific conditions you have in > mind. See the attached patch. I didn't look into details when I wrote that email. The rough idea was "Whatever makes the kernel do replay protection on incoming packets" (and if any attribute is output-only, skip those on input SAs). > For non-ESN scenarios, the outbound SA should have a replay window set to 0? Looks ok. > And for ESN 1? Why 1 and not 0? Does setting xfrm_replay_state_esn->{oseq,oseq_hi} on an input SA (and {seq,seq_hi} on output) make sense? And xfrm_state_update probably needs to check that the dir value matches? If we get this far we know the new state had matching direction and properties, but maybe the old one didn't? In XFRMA_SA_EXTRA_FLAGS, both XFRM_SA_XFLAG_DONT_ENCAP_DSCP and XFRM_SA_XFLAG_OSEQ_MAY_WRAP look like they're only used on output. A few of the flags defined with xfrm_usersa_info also seem to work only in one direction (XFRM_STATE_DECAP_DSCP, XFRM_STATE_NOPMTUDISC, XFRM_STATE_ICMP). > non-ESN > ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 \ > mode tunnel dir out aead 'rfc4106(gcm(aes))' \ > 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10 > Error: Replay-window too big > 0 for OUT SA. I'd probably change the string to "Replay window should not be set for OUT SA", that makes a bit more sense to me. "too big" implies that some values are valid, which isn't really the case. > The current impelementation does not replay window 0 with ESN. Even though > disabling replay window with ESN is a desired feature. > > ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 mode > tunnel dir out flag esn aead 'rfc4106(gcm(aes))' \ > 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10 > Error: ESN replay-window too big > 1 for OUT SA.ww > > I wonder would the attached patch get accepted quickly. I'm more interested in getting things right if we're going to introduce a new bit of API. IMO a new "dir" attribute that doesn't fully lock down the options on an SA is worse than no attribute (as Nicolas said previously, it's really confusing). And I'm not that familiar with all the API for xfrm SAs, so the properties I listed above may not be everything we should lock down (and maybe some are wrong). I think it would also make sense to only accept this attribute in xfrm_add_sa, and not for any of the other message types. Sharing xfrma_policy is convenient but not all attributes are valid for all requests. Old attributes can't be changed, but we should try to be more strict when we introduce new attributes. Sorry that I didn't notice this when you posted the previous versions.
On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote: > 2024-04-07, 10:23:21 +0200, Antony Antony wrote: > > Hi Sabrina, > > > > On Fri, Apr 05, 2024 at 11:56:00PM +0200, Sabrina Dubroca via Devel wrote: > > > Hi Antony, > > > > > > 2024-04-05, 14:40:07 +0200, Antony Antony wrote: > > > > This patch introduces the 'dir' attribute, 'in' or 'out', to the > > > > xfrm_state, SA, enhancing usability by delineating the scope of values > > > > based on direction. An input SA will now exclusively encompass values > > > > pertinent to input, effectively segregating them from output-related > > > > values. > > > > > > But this patch isn't doing that for existing properties (I'm thinking > > > of replay window, not sure if any others are relevant [1]). Why not? > > > > Thank you for raising this point. I thought that introducing a patch for > > the replay window check could stir more controversy, which might delay the > > acceptance of the essential 'dir' feature. > > I'm not convinced it's *that* critical. People have someone managed to Understood, but from a user's perspective, I've seen significant confusion around this issue. Labeling it as 'historical' and unchangeable ignores its real impact on usability. I feel adding "dir" would help a lot. > use IPsec without it for all those years. Is the intention to only > allow setting up IPTFS SAs when this new 'dir' attribute is provided? > If not, then this patch is not really blocking for IPTFS. > And yes, people will sometimes make comments on patches that cause > delays in getting the patches accepted. Some patches even end up > getting rejected. The kernel is better thanks to that process, even if > it can be annoying to the submitter (including me! it would be a lot > more relaxing if my patches always just went in at v1 :)). > > Nicolas, since you were objecting to the informational nature of the > attribute in v5: would you still object to the new attribute (and not > just limited to offload cases) if it properly restricted attributes > that don't match the direction? > > > My primary goal at this stage is > > to get this basic feature in and to convince Chris to integrate the "dir" > > attribute into IP-TFS. This patch has partly contributed to the delays in > > IP-TFS's development. > > > > Given your input, I'm curious about the specific conditions you have in > > mind. See the attached patch. > > I didn't look into details when I wrote that email. The rough idea was > "Whatever makes the kernel do replay protection on incoming packets" > (and if any attribute is output-only, skip those on input SAs). > > > For non-ESN scenarios, the outbound SA should have a replay window set to 0? > > Looks ok. > > > And for ESN 1? > > Why 1 and not 0? Current implemenation does not allow 0. Though supporting 0 is higly desired feature and probably a hard to implement feature in xfrm code. Supporting 0 is also a long standing argument:) https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/xfrm/xfrm_replay.c#n781 int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack) 781 if (replay_esn->replay_window == 0) { 782 NL_SET_ERR_MSG(extack, "ESN replay window must be > 0"); 783 return -EINVAL; 784 } > Does setting xfrm_replay_state_esn->{oseq,oseq_hi} on an input SA (and > {seq,seq_hi} on output) make sense? Good point. I will add {seq,seq_hi} validation. I don't think we add a for {oseq,oseq_hi} as it might be used by strongSwan with: ESN replay-window 1, and migrating an SA. > And xfrm_state_update probably needs to check that the dir value > matches? If we get this far we know the new state had matching > direction and properties, but maybe the old one didn't? Yes. I will add this too. > In XFRMA_SA_EXTRA_FLAGS, both XFRM_SA_XFLAG_DONT_ENCAP_DSCP and > XFRM_SA_XFLAG_OSEQ_MAY_WRAP look like they're only used on output. A > few of the flags defined with xfrm_usersa_info also seem to work only > in one direction (XFRM_STATE_DECAP_DSCP, XFRM_STATE_NOPMTUDISC, > XFRM_STATE_ICMP). I'm familiar with one flag, but my knowledge on the rest is limited, still I believe they aren't direction-specific. If anyone has more specific insight, please do share. Are any of these flags or x flags direction specific? - XFRM_STATE_ICMP should not be allowed on "out" SA. This is good point. I have seen users getting confused about this. > > non-ESN > > ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 \ > > mode tunnel dir out aead 'rfc4106(gcm(aes))' \ > > 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10 > > Error: Replay-window too big > 0 for OUT SA. > > I'd probably change the string to "Replay window should not be set for > OUT SA", that makes a bit more sense to me. "too big" implies that > some values are valid, which isn't really the case. > good point. fixed in v8. > > The current impelementation does not replay window 0 with ESN. Even though > > disabling replay window with ESN is a desired feature. > > > > ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 mode > > tunnel dir out flag esn aead 'rfc4106(gcm(aes))' \ > > 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10 > > Error: ESN replay-window too big > 1 for OUT SA.ww > > > > I wonder would the attached patch get accepted quickly. > > I'm more interested in getting things right if we're going to > introduce a new bit of API. IMO a new "dir" attribute that doesn't > fully lock down the options on an SA is worse than no attribute (as > Nicolas said previously, it's really confusing). > And I'm not that familiar with all the API for xfrm SAs, so the > properties I listed above may not be everything we should lock down > (and maybe some are wrong). > > I think it would also make sense to only accept this attribute in > xfrm_add_sa, and not for any of the other message types. Sharing > xfrma_policy is convenient but not all attributes are valid for all > requests. Old attributes can't be changed, but we should try to be > more strict when we introduce new attributes. To clarify your feedback, are you suggesting the API should not permit XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used equivalently to XFRM_MSG_NEWSA by *swan. > Sorry that I didn't notice this when you posted the previous versions. thanks your review. -antony
Le 08/04/2024 à 15:02, Sabrina Dubroca a écrit : [snip] > Nicolas, since you were objecting to the informational nature of the > attribute in v5: would you still object to the new attribute (and not > just limited to offload cases) if it properly restricted attributes > that don't match the direction? It's a good step, sure. Does this prevent an 'input' SA to be used in the output path? This is the case I'm objecting.
2024-04-10, 08:27:49 +0200, Nicolas Dichtel wrote: > Le 08/04/2024 à 15:02, Sabrina Dubroca a écrit : > [snip] > > Nicolas, since you were objecting to the informational nature of the > > attribute in v5: would you still object to the new attribute (and not > > just limited to offload cases) if it properly restricted attributes > > that don't match the direction? > It's a good step, sure. Does this prevent an 'input' SA to be used in the output > path? This is the case I'm objecting. Not in the latest version, what we were discussing here was only checking attributes that don't match the direction of the SA. Adding checks on the input and output patch to only look up and use SAs with the correct direction (or no direction set) should be doable, and probably has a negligible impact on performance. If we do this, we should maybe add a counter for direction mismatch (Xfrm{In,Out}StateDirMismatch?) to help admins. I agree that it would make more sense.
2024-04-09, 19:23:04 +0200, Antony Antony wrote: > On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote: > > 2024-04-07, 10:23:21 +0200, Antony Antony wrote: > > > Thank you for raising this point. I thought that introducing a patch for > > > the replay window check could stir more controversy, which might delay the > > > acceptance of the essential 'dir' feature. > > > > I'm not convinced it's *that* critical. People have someone managed to > > Understood, but from a user's perspective, I've seen significant confusion > around this issue. Labeling it as 'historical' and unchangeable ignores its > real impact on usability. I feel adding "dir" would help a lot. Sure. I meant that also in relation to IPTFS development: > > use IPsec without it for all those years. Is the intention to only > > allow setting up IPTFS SAs when this new 'dir' attribute is provided? > > If not, then this patch is not really blocking for IPTFS. [...] > > > For non-ESN scenarios, the outbound SA should have a replay window set to 0? > > > > Looks ok. > > > > > And for ESN 1? > > > > Why 1 and not 0? > > Current implemenation does not allow 0. So we have to pass a replay window even if we know the SA is for output? That's pretty bad. > Though supporting 0 is higly desired > feature and probably a hard to implement feature in xfrm code. Why would it be hard for outgoing SAs? The replay window should never be used on those. And xfrm_replay_check_esn and xfrm_replay_check_bmp already have checks for 0-sized replay window. > Supporting 0 > is also a long standing argument:) > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/xfrm/xfrm_replay.c#n781 > > int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack) > > 781 if (replay_esn->replay_window == 0) { > 782 NL_SET_ERR_MSG(extack, "ESN replay window must be > 0"); > 783 return -EINVAL; > 784 } > > > Does setting xfrm_replay_state_esn->{oseq,oseq_hi} on an input SA (and > > {seq,seq_hi} on output) make sense? > > Good point. I will add {seq,seq_hi} validation. I don't think we add a for > {oseq,oseq_hi} as it might be used by strongSwan with: ESN replay-window 1, > and migrating an SA. I'm not at all familiar with that. Can you explain the problem? Also, this is a new bit of API. We don't have to accept strange configs with it, userspace should adapt to the strict rules we require. > > And xfrm_state_update probably needs to check that the dir value > > matches? If we get this far we know the new state had matching > > direction and properties, but maybe the old one didn't? > > Yes. I will add this too. > > > In XFRMA_SA_EXTRA_FLAGS, both XFRM_SA_XFLAG_DONT_ENCAP_DSCP and > > XFRM_SA_XFLAG_OSEQ_MAY_WRAP look like they're only used on output. A > > few of the flags defined with xfrm_usersa_info also seem to work only > > in one direction (XFRM_STATE_DECAP_DSCP, XFRM_STATE_NOPMTUDISC, > > XFRM_STATE_ICMP). > > I'm familiar with one flag, but my knowledge on the rest is limited, still I > believe they aren't direction-specific. If anyone has more specific insight, > please do share. Are any of these flags or x flags direction specific? [I typically wait for answers to my questions before I post the next version of a patch. Otherwise, reviewers have to do more work, looking at each version.] BTW I just looked at all the flags defined in uapi, and asked cscope where they were used. For some, the function names were clearly only output path, for some just input, or both directions. [...] > > I think it would also make sense to only accept this attribute in > > xfrm_add_sa, and not for any of the other message types. Sharing > > > xfrma_policy is convenient but not all attributes are valid for all > > requests. Old attributes can't be changed, but we should try to be > > more strict when we introduce new attributes. > > To clarify your feedback, are you suggesting the API should not permit > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used > equivalently to XFRM_MSG_NEWSA by *swan. Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA and UPDSA should accept it, but I'm thinking none of the other operations should. It's a property of SAs, not of other xfrm objects. Thanks.
On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote: > 2024-04-09, 19:23:04 +0200, Antony Antony wrote: > > On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote: > > > 2024-04-07, 10:23:21 +0200, Antony Antony wrote: > > > > Thank you for raising this point. I thought that introducing a patch for > > > > the replay window check could stir more controversy, which might delay the > > > > acceptance of the essential 'dir' feature. > > > > > > I'm not convinced it's *that* critical. People have someone managed to > > > > Understood, but from a user's perspective, I've seen significant confusion > > around this issue. Labeling it as 'historical' and unchangeable ignores its > > real impact on usability. I feel adding "dir" would help a lot. > > Sure. I meant that also in relation to IPTFS development: > > > > use IPsec without it for all those years. Is the intention to only > > > allow setting up IPTFS SAs when this new 'dir' attribute is provided? > > > If not, then this patch is not really blocking for IPTFS. > > > [...] > > > > For non-ESN scenarios, the outbound SA should have a replay window set to 0? > > > > > > Looks ok. > > > > > > > And for ESN 1? > > > > > > Why 1 and not 0? > > > > Current implemenation does not allow 0. > > So we have to pass a replay window even if we know the SA is for > output? That's pretty bad. we can default to 1 with ESN and when no replay-window is specified. > > Though supporting 0 is higly desired > > feature and probably a hard to implement feature in xfrm code. > > Why would it be hard for outgoing SAs? The replay window should never > be used on those. And xfrm_replay_check_esn and xfrm_replay_check_bmp > already have checks for 0-sized replay window. That information comes from hall way talks with Steffen. I can't explain it:) May be he can elaborate why 0 is not allowed with ESN. > > Supporting 0 is also a long standing argument:) > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/xfrm/xfrm_replay.c#n781 > > > > int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack) > > > > 781 if (replay_esn->replay_window == 0) { > > 782 NL_SET_ERR_MSG(extack, "ESN replay window must be > 0"); > > 783 return -EINVAL; > > 784 } > > > > > Does setting xfrm_replay_state_esn->{oseq,oseq_hi} on an input SA (and > > > {seq,seq_hi} on output) make sense? > > > > Good point. I will add {seq,seq_hi} validation. I don't think we add a for > > {oseq,oseq_hi} as it might be used by strongSwan with: ESN replay-window 1, > > and migrating an SA. > > I'm not at all familiar with that. Can you explain the problem? strongSwan sets ESN and replay-window 1 on "out" SA. Then to migrgate, when IKEv2 mobike exchange succeds, it use GETSA read {oseq,oseq_hi} and the attributes, delete this SA. Then create a new SA, with a different end point, and with old SA's {oseq,oseq_hi} and other parameters(curlft..). While Libreswan and Android use XFRM_MSG_MIGRATE. > Also, this is a new bit of API. We don't have to accept strange > configs with it, userspace should adapt to the strict rules we > require. > > > > And xfrm_state_update probably needs to check that the dir value > > > matches? If we get this far we know the new state had matching > > > direction and properties, but maybe the old one didn't? > > > > Yes. I will add this too. > > > > > In XFRMA_SA_EXTRA_FLAGS, both XFRM_SA_XFLAG_DONT_ENCAP_DSCP and > > > XFRM_SA_XFLAG_OSEQ_MAY_WRAP look like they're only used on output. A > > > few of the flags defined with xfrm_usersa_info also seem to work only > > > in one direction (XFRM_STATE_DECAP_DSCP, XFRM_STATE_NOPMTUDISC, > > > XFRM_STATE_ICMP). > > > > I'm familiar with one flag, but my knowledge on the rest is limited, still I > > believe they aren't direction-specific. If anyone has more specific insight, > > please do share. Are any of these flags or x flags direction specific? > > [I typically wait for answers to my questions before I post the next > version of a patch. Otherwise, reviewers have to do more work, looking > at each version.] I will not post v10 yet. > BTW I just looked at all the flags defined in uapi, and asked cscope > where they were used. For some, the function names were clearly only > output path, for some just input, or both directions. I looked closer at the flags and I noticed several of them are direction specific. And I am proposing to valide them and a simple data path check for directions in v10. > [...] > > > I think it would also make sense to only accept this attribute in > > > xfrm_add_sa, and not for any of the other message types. Sharing > > > > > xfrma_policy is convenient but not all attributes are valid for all > > > requests. Old attributes can't be changed, but we should try to be > > > more strict when we introduce new attributes. > > > > To clarify your feedback, are you suggesting the API should not permit > > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for > > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used > > equivalently to XFRM_MSG_NEWSA by *swan. > > Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA > and UPDSA should accept it, but I'm thinking none of the other > operations should. It's a property of SAs, not of other xfrm objects. For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in DELSA, it would also be disregarded. Attempting to introduce validations for DELSA and other methods seems like an extensive cleanup task. Do we consider this level of validation within the scope of our current patch? It feels like we are going too far.
Antony Antony via Devel <devel@linux-ipsec.org> writes: > On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote: >> 2024-04-09, 19:23:04 +0200, Antony Antony wrote: >> > On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote: >> > > 2024-04-07, 10:23:21 +0200, Antony Antony wrote: >> > > I think it would also make sense to only accept this attribute in >> > > xfrm_add_sa, and not for any of the other message types. Sharing >> > >> > > xfrma_policy is convenient but not all attributes are valid for all >> > > requests. Old attributes can't be changed, but we should try to be >> > > more strict when we introduce new attributes. >> > >> > To clarify your feedback, are you suggesting the API should not permit >> > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for >> > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used >> > equivalently to XFRM_MSG_NEWSA by *swan. >> >> Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA >> and UPDSA should accept it, but I'm thinking none of the other >> operations should. It's a property of SAs, not of other xfrm objects. > > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in > DELSA, it would also be disregarded. Attempting to introduce validations for > DELSA and other methods seems like an extensive cleanup task. Do we consider > this level of validation within the scope of our current patch? It feels > like we are going too far. I think a general clean up feels like a distinct patch to me. Thanks, Chris.
On Wed, 10 Apr 2024, Antony Antony via Devel wrote: >>>>> And for ESN 1? >>>> >>>> Why 1 and not 0? >>> >>> Current implemenation does not allow 0. >> >> So we have to pass a replay window even if we know the SA is for >> output? That's pretty bad. > > we can default to 1 with ESN and when no replay-window is specified. > >>> Though supporting 0 is higly desired >>> feature and probably a hard to implement feature in xfrm code. >> >> Why would it be hard for outgoing SAs? The replay window should never >> be used on those. And xfrm_replay_check_esn and xfrm_replay_check_bmp >> already have checks for 0-sized replay window. > > That information comes from hall way talks with Steffen. I can't explain > it:) May be he can elaborate why 0 is not allowed with ESN. With ESN, you use a 64 bit number but only send a 32 bit number over the wire. So you need to "track" the parts not being sent to do the proper packet authentication that uses the full 64bit number. The authentication bit is needed for encrypting and decrypting, so on both the incoming and outgoing SA. AFAIK, this 64 bit number tracking is done using the replay-window code. That is why replay-window cannot be 0 when ESN is enabled in either direction of the SA. I have already poked Steffen it would be good to decouple ESN code from replay-window code, as often people want to benchmark highspeed links by disabling replay protection completely, but then they are also unwittingly disabling ESN and causing needing a rekey ever 2 minutes or so on a modern 100gbps ipsec link. > strongSwan sets ESN and replay-window 1 on "out" SA. It has to set a replay-window of non-zero or else ESN won't work. It is not related to migration AFAIK. > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in > DELSA, it would also be disregarded. Attempting to introduce validations for > DELSA and other methods seems like an extensive cleanup task. Do we consider > this level of validation within the scope of our current patch? It feels > like we are going too far. Is there a way where rate limited logging can be introduced, so that userlands will clean up their use and after a few years change the API to not allow setting bogus values? Paul
2024-04-10, 20:58:33 -0400, Paul Wouters wrote: > On Wed, 10 Apr 2024, Antony Antony via Devel wrote: > > > > Though supporting 0 is higly desired > > > > feature and probably a hard to implement feature in xfrm code. > > > > > > Why would it be hard for outgoing SAs? The replay window should never > > > be used on those. And xfrm_replay_check_esn and xfrm_replay_check_bmp > > > already have checks for 0-sized replay window. > > > > That information comes from hall way talks with Steffen. I can't explain > > it:) May be he can elaborate why 0 is not allowed with ESN. > > With ESN, you use a 64 bit number but only send a 32 bit number over the > wire. So you need to "track" the parts not being sent to do the proper > packet authentication that uses the full 64bit number. The > authentication bit is needed for encrypting and decrypting, so on both > the incoming and outgoing SA. > > AFAIK, this 64 bit number tracking is done using the replay-window code. > That is why replay-window cannot be 0 when ESN is enabled in either > direction of the SA. It's in the replay-window code, but AFAICT it doesn't use the replay_window variable at all (xfrm_output calls into the xfrm_replay_overflow_* functions which only look at oseq, xfrm_input calls the *check and *advance functions of xfrm_replay.c). So I think we could accept an unset replay_window for an output SA. > I have already poked Steffen it would be good to decouple ESN code from > replay-window code, as often people want to benchmark highspeed links > by disabling replay protection completely, but then they are also > unwittingly disabling ESN and causing needing a rekey ever 2 minutes > or so on a modern 100gbps ipsec link. > > > strongSwan sets ESN and replay-window 1 on "out" SA. > > It has to set a replay-window of non-zero or else ESN won't work. > It is not related to migration AFAIK. > > > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in > > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in > > DELSA, it would also be disregarded. Attempting to introduce validations for > > DELSA and other methods seems like an extensive cleanup task. Do we consider > > this level of validation within the scope of our current patch? It feels > > like we are going too far. > > Is there a way where rate limited logging can be introduced, so that > userlands will clean up their use and after a few years change the API > to not allow setting bogus values? Yes, this is doable. Steffen, does that seem reasonable? (for example, when XFRMA_REPLAY_THRESH is passed to NEWSA, or XFRMA_ALG_AEAD to DELSA, etc) (as part of a separate patchset of course)
2024-04-10, 18:59:00 +0200, Antony Antony wrote: > On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote: > > 2024-04-09, 19:23:04 +0200, Antony Antony wrote: > > > Good point. I will add {seq,seq_hi} validation. I don't think we add a for > > > {oseq,oseq_hi} as it might be used by strongSwan with: ESN replay-window 1, > > > and migrating an SA. > > > > I'm not at all familiar with that. Can you explain the problem? > > strongSwan sets ESN and replay-window 1 on "out" SA. Then to migrgate, when > IKEv2 mobike exchange succeds, it use GETSA read {oseq,oseq_hi} and the > attributes, delete this SA. Then create a new SA, with a different end > point, and with old SA's {oseq,oseq_hi} and other parameters(curlft..). > While Libreswan and Android use XFRM_MSG_MIGRATE. Ok, thanks. But that's still an output SA. Setting {oseq,oseq_hi} on an input SA is bogus I would think? > > > > xfrma_policy is convenient but not all attributes are valid for all > > > > requests. Old attributes can't be changed, but we should try to be > > > > more strict when we introduce new attributes. > > > > > > To clarify your feedback, are you suggesting the API should not permit > > > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for > > > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used > > > equivalently to XFRM_MSG_NEWSA by *swan. > > > > Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA > > and UPDSA should accept it, but I'm thinking none of the other > > operations should. It's a property of SAs, not of other xfrm objects. > > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in > DELSA, it would also be disregarded. Attempting to introduce validations for > DELSA and other methods seems like an extensive cleanup task. Do we consider > this level of validation within the scope of our current patch? It feels > like we are going too far. No, I wouldn't introduce validation of other attributes. It doesn't belong in this patch(set), and I'm not sure we can add it now as it might break userspace (I don't see why userspace would pass XFRMA_ALG_AEAD etc on a DELSA request, but if we never rejected it, they could). But rejecting this new attribute from messages that don't handle it would be good, and should be done in this patch/series.
On Thu, Apr 11, 2024 at 11:24:06AM +0200, Sabrina Dubroca wrote: > 2024-04-10, 18:59:00 +0200, Antony Antony wrote: > > On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote: > > > 2024-04-09, 19:23:04 +0200, Antony Antony wrote: > > > > Good point. I will add {seq,seq_hi} validation. I don't think we add a for > > > > {oseq,oseq_hi} as it might be used by strongSwan with: ESN replay-window 1, > > > > and migrating an SA. > > > > > > I'm not at all familiar with that. Can you explain the problem? > > > > strongSwan sets ESN and replay-window 1 on "out" SA. Then to migrgate, when > > IKEv2 mobike exchange succeds, it use GETSA read {oseq,oseq_hi} and the > > attributes, delete this SA. Then create a new SA, with a different end > > point, and with old SA's {oseq,oseq_hi} and other parameters(curlft..). > > While Libreswan and Android use XFRM_MSG_MIGRATE. > > Ok, thanks. But that's still an output SA. Setting {oseq,oseq_hi} on > an input SA is bogus I would think? Corrrect, It is not allowed in v10. > > > > > > xfrma_policy is convenient but not all attributes are valid for all > > > > > requests. Old attributes can't be changed, but we should try to be > > > > > more strict when we introduce new attributes. > > > > > > > > To clarify your feedback, are you suggesting the API should not permit > > > > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for > > > > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used > > > > equivalently to XFRM_MSG_NEWSA by *swan. > > > > > > Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA > > > and UPDSA should accept it, but I'm thinking none of the other > > > operations should. It's a property of SAs, not of other xfrm objects. > > > > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in > > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in > > DELSA, it would also be disregarded. Attempting to introduce validations for > > DELSA and other methods seems like an extensive cleanup task. Do we consider > > this level of validation within the scope of our current patch? It feels > > like we are going too far. > > No, I wouldn't introduce validation of other attributes. It doesn't > belong in this patch(set), and I'm not sure we can add it now as it > might break userspace (I don't see why userspace would pass > XFRMA_ALG_AEAD etc on a DELSA request, but if we never rejected it, > they could). > > But rejecting this new attribute from messages that don't handle it > would be good, and should be done in this patch/series. Definitely see the value in such feature in general, but it seems ambitious for this patch set. Currently, only NEWSA, UPDSA, and ALLOCSPI need XFRMA_SA_DIR. I am wondering how to reject this atrribute in remaining 20-22 messages. Is there a precedent or example in xfrm_user.c for this kind of validation, or maybe a Netlink feature that lets us restrict NL attributes for a specific messages like DELSA. If not, it feels like a seperate patch set for general API cleanup.
On Wed, Apr 10, 2024 at 06:59:00PM +0200, Antony Antony via Devel wrote: > On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote: > > 2024-04-09, 19:23:04 +0200, Antony Antony wrote: > > > On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote: > > > > 2024-04-07, 10:23:21 +0200, Antony Antony wrote: > > > > > > Current implemenation does not allow 0. > > > > So we have to pass a replay window even if we know the SA is for > > output? That's pretty bad. > > we can default to 1 with ESN and when no replay-window is specified. > > > > Though supporting 0 is higly desired > > > feature and probably a hard to implement feature in xfrm code. > > > > Why would it be hard for outgoing SAs? The replay window should never > > be used on those. And xfrm_replay_check_esn and xfrm_replay_check_bmp > > already have checks for 0-sized replay window. > > That information comes from hall way talks with Steffen. I can't explain > it:) May be he can elaborate why 0 is not allowed with ESN. That is because the algorithm on the receive side does not work with replay window 0. Once we have sepateted input and output SAs, thereplay window can be 0 on outout.
On Thu, Apr 11, 2024 at 11:23:36AM +0200, Sabrina Dubroca wrote: > 2024-04-10, 20:58:33 -0400, Paul Wouters wrote: > > On Wed, 10 Apr 2024, Antony Antony via Devel wrote: > > > > > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in > > > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in > > > DELSA, it would also be disregarded. Attempting to introduce validations for > > > DELSA and other methods seems like an extensive cleanup task. Do we consider > > > this level of validation within the scope of our current patch? It feels > > > like we are going too far. > > > > Is there a way where rate limited logging can be introduced, so that > > userlands will clean up their use and after a few years change the API > > to not allow setting bogus values? > > Yes, this is doable. Steffen, does that seem reasonable? (for example, > when XFRMA_REPLAY_THRESH is passed to NEWSA, or XFRMA_ALG_AEAD to > DELSA, etc) Yes, a cleanup would be very wellcome. But that might be a bit of work :-)
2024-04-11, 12:36:28 +0200, Antony Antony wrote: > On Thu, Apr 11, 2024 at 11:24:06AM +0200, Sabrina Dubroca wrote: > > 2024-04-10, 18:59:00 +0200, Antony Antony wrote: > > > On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote: > > > > 2024-04-09, 19:23:04 +0200, Antony Antony wrote: > > > > > > xfrma_policy is convenient but not all attributes are valid for all > > > > > > requests. Old attributes can't be changed, but we should try to be > > > > > > more strict when we introduce new attributes. > > > > > > > > > > To clarify your feedback, are you suggesting the API should not permit > > > > > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for > > > > > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used > > > > > equivalently to XFRM_MSG_NEWSA by *swan. > > > > > > > > Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA > > > > and UPDSA should accept it, but I'm thinking none of the other > > > > operations should. It's a property of SAs, not of other xfrm objects. > > > > > > For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in > > > DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in > > > DELSA, it would also be disregarded. Attempting to introduce validations for > > > DELSA and other methods seems like an extensive cleanup task. Do we consider > > > this level of validation within the scope of our current patch? It feels > > > like we are going too far. > > > > No, I wouldn't introduce validation of other attributes. It doesn't > > belong in this patch(set), and I'm not sure we can add it now as it > > might break userspace (I don't see why userspace would pass > > XFRMA_ALG_AEAD etc on a DELSA request, but if we never rejected it, > > they could). > > > > But rejecting this new attribute from messages that don't handle it > > would be good, and should be done in this patch/series. > > Definitely see the value in such feature in general, but it seems ambitious > for this patch set. I'm only talking about the new attribute here. Introducing validation for all other attributes, yes, that's a completely separate thing (and we can't do that immediately, we need to work toward it, see Paul's suggestion). > Currently, only NEWSA, UPDSA, and ALLOCSPI need > XFRMA_SA_DIR. I am wondering how to reject this atrribute in remaining 20-22 > messages. Is there a precedent or example in xfrm_user.c for this kind of > validation, or maybe a Netlink feature that lets us restrict NL attributes > for a specific messages like DELSA. I don't think there is, xfrm_user doesn't do that kind of validation yet. There's an example in rtnl_valid_dump_net_req and rtnl_net_valid_getid_req, where some attributes are rejected.
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 57c743b7e4fe..7c9be06f8302 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -291,6 +291,7 @@ struct xfrm_state { /* Private data of this transformer, format is opaque, * interpreted by xfrm_type methods. */ void *data; + u8 dir; }; static inline struct net *xs_net(struct xfrm_state *x) diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h index 6a77328be114..18ceaba8486e 100644 --- a/include/uapi/linux/xfrm.h +++ b/include/uapi/linux/xfrm.h @@ -141,6 +141,11 @@ enum { XFRM_POLICY_MAX = 3 }; +enum xfrm_sa_dir { + XFRM_SA_DIR_IN = 1, + XFRM_SA_DIR_OUT = 2 +}; + enum { XFRM_SHARE_ANY, /* No limitations */ XFRM_SHARE_SESSION, /* For this session only */ @@ -315,6 +320,7 @@ enum xfrm_attr_type_t { XFRMA_SET_MARK_MASK, /* __u32 */ XFRMA_IF_ID, /* __u32 */ XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */ + XFRMA_SA_DIR, /* __u8 */ __XFRMA_MAX #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */ diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c index 655fe4ff8621..007dee03b1bc 100644 --- a/net/xfrm/xfrm_compat.c +++ b/net/xfrm/xfrm_compat.c @@ -98,6 +98,7 @@ static const int compat_msg_min[XFRM_NR_MSGTYPES] = { }; static const struct nla_policy compat_policy[XFRMA_MAX+1] = { + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR }, [XFRMA_SA] = { .len = XMSGSIZE(compat_xfrm_usersa_info)}, [XFRMA_POLICY] = { .len = XMSGSIZE(compat_xfrm_userpolicy_info)}, [XFRMA_LASTUSED] = { .type = NLA_U64}, @@ -129,6 +130,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = { [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 }, [XFRMA_IF_ID] = { .type = NLA_U32 }, [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 }, + [XFRMA_SA_DIR] = { .type = NLA_U8} }; static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb, @@ -277,9 +279,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src) case XFRMA_SET_MARK_MASK: case XFRMA_IF_ID: case XFRMA_MTIMER_THRESH: + case XFRMA_SA_DIR: return xfrm_nla_cpy(dst, src, nla_len(src)); default: - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH); + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR); pr_warn_once("unsupported nla_type %d\n", src->nla_type); return -EOPNOTSUPP; } @@ -434,7 +437,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla, int err; if (type > XFRMA_MAX) { - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH); + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR); NL_SET_ERR_MSG(extack, "Bad attribute"); return -EOPNOTSUPP; } diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index 6346690d5c69..2455a76a1cff 100644 --- a/net/xfrm/xfrm_device.c +++ b/net/xfrm/xfrm_device.c @@ -253,6 +253,12 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x, return -EINVAL; } + if ((xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir == XFRM_SA_DIR_OUT) || + (!(xuo->flags & XFRM_OFFLOAD_INBOUND) && x->dir == XFRM_SA_DIR_IN)) { + NL_SET_ERR_MSG(extack, "Mismatched SA and offload direction"); + return -EINVAL; + } + is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET; /* We don't yet support UDP encapsulation and TFC padding. */ diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 0c306473a79d..749c9cf41c06 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, x->lastused = orig->lastused; x->new_mapping = 0; x->new_mapping_sport = 0; + x->dir = orig->dir; return x; diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 810b520493f3..c26f26a100a1 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -360,6 +360,22 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, } } + if (attrs[XFRMA_SA_DIR]) { + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]); + + if (sa_dir != XFRM_SA_DIR_IN && sa_dir != XFRM_SA_DIR_OUT) { + NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range"); + err = -EINVAL; + goto out; + } + + if (!attrs[XFRMA_OFFLOAD_DEV]) { + NL_SET_ERR_MSG(extack, "Missing required offload attribute for XFRMA_SA_DIR"); + err = -EINVAL; + goto out; + } + } + out: return err; } @@ -627,6 +643,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs, struct nlattr *et = attrs[XFRMA_ETIMER_THRESH]; struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH]; struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH]; + struct nlattr *dir = attrs[XFRMA_SA_DIR]; if (re && x->replay_esn && x->preplay_esn) { struct xfrm_replay_state_esn *replay_esn; @@ -661,6 +678,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs, if (mt) x->mapping_maxage = nla_get_u32(mt); + + if (dir) + x->dir = nla_get_u8(dir); } static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m) @@ -1182,8 +1202,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x, if (ret) goto out; } - if (x->mapping_maxage) + if (x->mapping_maxage) { ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage); + if (ret) + goto out; + } + if (x->dir) + ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); out: return ret; } @@ -2402,7 +2427,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x) + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur)) + nla_total_size(sizeof(struct xfrm_mark)) + nla_total_size(4) /* XFRM_AE_RTHR */ - + nla_total_size(4); /* XFRM_AE_ETHR */ + + nla_total_size(4) /* XFRM_AE_ETHR */ + + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */ } static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c) @@ -2459,6 +2485,12 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct if (err) goto out_cancel; + if (x->dir) { + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); + if (err) + goto out_cancel; + } + nlmsg_end(skb, nlh); return 0; @@ -3018,6 +3050,7 @@ EXPORT_SYMBOL_GPL(xfrm_msg_min); #undef XMSGSIZE const struct nla_policy xfrma_policy[XFRMA_MAX+1] = { + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR }, [XFRMA_SA] = { .len = sizeof(struct xfrm_usersa_info)}, [XFRMA_POLICY] = { .len = sizeof(struct xfrm_userpolicy_info)}, [XFRMA_LASTUSED] = { .type = NLA_U64}, @@ -3049,6 +3082,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = { [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 }, [XFRMA_IF_ID] = { .type = NLA_U32 }, [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 }, + [XFRMA_SA_DIR] = { .type = NLA_U8 } }; EXPORT_SYMBOL_GPL(xfrma_policy); @@ -3189,8 +3223,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb) static inline unsigned int xfrm_expire_msgsize(void) { - return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) - + nla_total_size(sizeof(struct xfrm_mark)); + return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) + + nla_total_size(sizeof(struct xfrm_mark)) + + nla_total_size(sizeof_field(struct xfrm_state, dir)); } static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c) @@ -3217,6 +3252,12 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct if (err) return err; + if (x->dir) { + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); + if (err) + return err; + } + nlmsg_end(skb, nlh); return 0; } @@ -3324,6 +3365,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x) if (x->mapping_maxage) l += nla_total_size(sizeof(x->mapping_maxage)); + if (x->dir) + l += nla_total_size(sizeof(x->dir)); + return l; }