| Message ID | 0e0d997e634261fcdf16cf9f07c97d97af7370b6.1712828282.git.antony.antony@secunet.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec-next,v10,1/3] xfrm: Add Direction to the SA in or out | expand |
On Thu, Apr 11, 2024 at 11:40:59AM +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. 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> > --- > v9->v10: > - add more direction specific validations > XFRM_STATE_NOPMTUDISC, XFRM_SA_XFLAG_DONT_ENCAP_DSCP > XFRMA_MTIMER_THRESH > - refactor validations into a fuction. > - add dir to ALLOCSPI to support strongSwan updating SPI for "in" state > v8->v9: > - add validation XFRM_STATE_ICMP not allowed on OUT SA. > > v7->v8: > - add extra validation check on replay window and seq > - XFRM_MSG_UPDSA old and new SA should match "dir" I asked it on one of the previous versions, but why do we need this limitation? Update SA is actually add and delete, so if user wants to update direction, it should be allowed. Thanks
On Apr 11, 2024, at 07:41, Leon Romanovsky via Devel <devel@linux-ipsec.org> wrote: > > > I asked it on one of the previous versions, but why do we need this limitation? > Update SA is actually add and delete, so if user wants to update > direction, it should be allowed. An SA can never change direction without dealing with updated SPIs. Logically, it makes no sense. Sure, if you treat SA’s as Linux lego bricks that can be turned into anything, then yeah why not. Why not turn the SA into block device or magic flute? If you keep to the RFC, an SA is uni directional. More things might apply too, eg the mode (transport vs tunnel) should not change, sequence numbers can only increase, etc. Paul
Paul Wouters via Devel <devel@linux-ipsec.org> writes: > On Apr 11, 2024, at 07:41, Leon Romanovsky via Devel <devel@linux-ipsec.org> wrote: >> >> >> I asked it on one of the previous versions, but why do we need this limitation? >> Update SA is actually add and delete, so if user wants to update >> direction, it should be allowed. > > An SA can never change direction without dealing with updated SPIs. Logically, > it makes no sense. Sure, if you treat SA’s as Linux lego bricks that can be > turned into anything, then yeah why not. Why not turn the SA into block device > or magic flute? > > If you keep to the RFC, an SA is uni directional. More things might apply too, > eg the mode (transport vs tunnel) should not change, sequence numbers can only > increase, etc. I have to agree. When we add new direction specific attributes (e.g., iptfs send-rate, pkt-size, etc, for "out", reorder window size, drop-time for "in") changing direction would mean removing invalidating these attributes and possibly setting new different ones, deleting old send state adding new receive state, etc.. this doesn't feel like an SA update IMO, it feels like a new SA. I say we should apply KISS and just require a new SA for a different direction attribute. Thanks, Chris. > > Paul
On Thu, Apr 11, 2024 at 12:20:31PM -0400, Paul Wouters wrote: > On Apr 11, 2024, at 07:41, Leon Romanovsky via Devel <devel@linux-ipsec.org> wrote: > > > > > > I asked it on one of the previous versions, but why do we need this limitation? > > Update SA is actually add and delete, so if user wants to update > > direction, it should be allowed. > > An SA can never change direction without dealing with updated SPIs. Logically, it makes no sense. Sure, if you treat SA’s as Linux lego bricks that can be turned into anything, then yeah why not. Why not turn the SA into block device or magic flute? > > If you keep to the RFC, an SA is uni directional. More things might apply too, eg the mode (transport vs tunnel) should not change, sequence numbers can only increase, etc. Right, and it looks like preventing change of direction is only small part of larger task - "improve validation of SA updates". Thanks > > Paul >
2024-04-11, 11:40:59 +0200, Antony Antony wrote: > 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} nit: <...> }, (space before } and , afterwards) See below for a comment on the policy itself. > 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; > + } It would be nice to set x->dir to match the flag, but then I guess the validation in xfrm_state_update would fail if userspaces tries an update without providing XFRMA_SA_DIR. (or not because we already went through this code by the time we get to xfrm_state_update?) > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > index 810b520493f3..df141edbe8d1 100644 > --- a/net/xfrm/xfrm_user.c > +++ b/net/xfrm/xfrm_user.c [...] > @@ -779,6 +793,77 @@ static struct xfrm_state *xfrm_state_construct(struct net *net, > return NULL; > } > > +static int verify_sa_dir(const struct xfrm_state *x, struct netlink_ext_ack *extack) > +{ > + if (x->dir == XFRM_SA_DIR_OUT) { > + if (x->props.replay_window > 0) { > + NL_SET_ERR_MSG(extack, "Replay window should not be set for OUT SA"); > + return -EINVAL; > + } > + > + if (x->replay.seq || x->replay.bitmap) { > + NL_SET_ERR_MSG(extack, > + "Replay seq, or bitmap should not be set for OUT SA with ESN"); I thought x->replay was for non-ESN, since we have x->replay_esn. > + return -EINVAL; > + } > + > + if (x->replay_esn) { > + if (x->replay_esn->replay_window > 1) { > + NL_SET_ERR_MSG(extack, > + "Replay window should be 1 for OUT SA with ESN"); I don't think that we should introduce something we know doesn't make sense (replay window = 1 on output). It will be API and we won't be able to fix it up later. We get a chance to make things nice and reasonable with this new attribute, let's not waste it. As I said, AFAICT replay_esn->replay_window isn't used on output, so unless I missed something, it should just be a matter of changing the validation. The additional checks in this version should guarantee we don't have dir==OUT SAs in the packet input path, so this should work. > + return -EINVAL; > + } > + > + if (x->replay_esn->seq || x->replay_esn->seq_hi || x->replay_esn->bmp_len) { > + NL_SET_ERR_MSG(extack, > + "Replay seq, seq_hi, bmp_len should not be set for OUT SA with ESN"); > + return -EINVAL; > + } > + } > + > + if (x->props.flags & XFRM_STATE_DECAP_DSCP) { > + NL_SET_ERR_MSG(extack, "Flag NDECAP_DSCP should not be set for OUT SA"); ^ extra N? > + return -EINVAL; > + } > + [...] > static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > struct nlattr **attrs, struct netlink_ext_ack *extack) > { > @@ -796,6 +881,16 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > if (!x) > return err; > > + if (x->dir) { > + err = verify_sa_dir(x, extack); > + if (err) { > + x->km.state = XFRM_STATE_DEAD; > + xfrm_dev_state_delete(x); > + xfrm_state_put(x); > + return err; That's not very nice. We're creating a state and just throwing it away immediately. How hard would it be to validate all that directly from verify_newsa_info instead? [...] > @@ -3018,6 +3137,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 +3169,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 } With .type = NLA_POLICY_RANGE(NLA_U8, XFRM_SA_DIR_IN, XFRM_SA_DIR_OUT) }, you wouldn't need to validate the attribute's values in verify_newsa_info and xfrm_alloc_userspi. And same for the xfrm_compat version of this. (also a nit on the formatting: a "," after the } would be nice, so that the next addition doesn't need to touch this line) And as we discussed, I'd really like XFRMA_SA_DIR to be rejected in commands that don't use its value. Thanks.
On Mon, Apr 15, 2024 at 02:21:50PM +0200, Sabrina Dubroca via Devel wrote: > 2024-04-11, 11:40:59 +0200, Antony Antony wrote: > > 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} > > nit: <...> }, > > (space before } and , afterwards) > > See below for a comment on the policy itself. fixed in v11. > > > > 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; > > + } > > It would be nice to set x->dir to match the flag, but then I guess the > validation in xfrm_state_update would fail if userspaces tries an > update without providing XFRMA_SA_DIR. (or not because we already went > through this code by the time we get to xfrm_state_update?) this code already executed from xfrm_state_construct. We could set the in flag in xuo when x->dir == XFRM_SA_DIR_IN, let me think again. May be we can do that later:) > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > > index 810b520493f3..df141edbe8d1 100644 > > --- a/net/xfrm/xfrm_user.c > > +++ b/net/xfrm/xfrm_user.c > [...] > > @@ -779,6 +793,77 @@ static struct xfrm_state *xfrm_state_construct(struct net *net, > > return NULL; > > } > > > > +static int verify_sa_dir(const struct xfrm_state *x, struct netlink_ext_ack *extack) > > +{ > > + if (x->dir == XFRM_SA_DIR_OUT) { > > + if (x->props.replay_window > 0) { > > + NL_SET_ERR_MSG(extack, "Replay window should not be set for OUT SA"); > > + return -EINVAL; > > + } > > + > > + if (x->replay.seq || x->replay.bitmap) { > > + NL_SET_ERR_MSG(extack, > > + "Replay seq, or bitmap should not be set for OUT SA with ESN"); > > I thought x->replay was for non-ESN, since we have x->replay_esn. > you are right. It is a wrong text due to copy paste. Fixed in v11. > > + return -EINVAL; > > + } > > + > > + if (x->replay_esn) { > > + if (x->replay_esn->replay_window > 1) { > > + NL_SET_ERR_MSG(extack, > > + "Replay window should be 1 for OUT SA with ESN"); > > I don't think that we should introduce something we know doesn't make > sense (replay window = 1 on output). It will be API and we won't be > able to fix it up later. We get a chance to make things nice and > reasonable with this new attribute, let's not waste it. > > As I said, AFAICT replay_esn->replay_window isn't used on output, so > unless I missed something, it should just be a matter of changing the > validation. The additional checks in this version should guarantee we > don't have dir==OUT SAs in the packet input path, so this should work. I agree. Your message and Steffen's message helped me figure out, how to allow replay-window zero for output SA; It is in v11. > > + return -EINVAL; > > + } > > + > > + if (x->replay_esn->seq || x->replay_esn->seq_hi || x->replay_esn->bmp_len) { > > + NL_SET_ERR_MSG(extack, > > + "Replay seq, seq_hi, bmp_len should not be set for OUT SA with ESN"); > > + return -EINVAL; > > + } > > + } > > + > > + if (x->props.flags & XFRM_STATE_DECAP_DSCP) { > > + NL_SET_ERR_MSG(extack, "Flag NDECAP_DSCP should not be set for OUT SA"); > > ^ extra N? > > > + return -EINVAL; > > + } > > + > > [...] > > static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > > struct nlattr **attrs, struct netlink_ext_ack *extack) > > { > > @@ -796,6 +881,16 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > > if (!x) > > return err; > > > > + if (x->dir) { > > + err = verify_sa_dir(x, extack); > > + if (err) { > > + x->km.state = XFRM_STATE_DEAD; > > + xfrm_dev_state_delete(x); > > + xfrm_state_put(x); > > + return err; > > That's not very nice. We're creating a state and just throwing it away > immediately. How hard would it be to validate all that directly from > verify_newsa_info instead? Your proposal would introduce redundant code, requiring accessing attributes in verify_newsa_info() and other functions. The way I propsed, a state x, xfrm_state, is created but it remains km.stae=XFRM_STATE_VOID. Newely added verify is before auditing and generating new genid changes, xfrm_state_add() or xfrm_state_update() would be called later. So deleteing a state just after xfrm_staet_constructi() is not bad! So I think the current code is cleaner, avoiding the need redundant code in verify_newsa_info(). > > [...] > > @@ -3018,6 +3137,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 +3169,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 } > > With > > .type = NLA_POLICY_RANGE(NLA_U8, XFRM_SA_DIR_IN, XFRM_SA_DIR_OUT) }, > > you wouldn't need to validate the attribute's values in > verify_newsa_info and xfrm_alloc_userspi. And same for the xfrm_compat > version of this. thanks, this is much better. > (also a nit on the formatting: a "," after the } would be nice, so > that the next addition doesn't need to touch this line) > > > And as we discussed, I'd really like XFRMA_SA_DIR to be rejected in > commands that don't use its value. I still don't see how to add such a check to about 20 functions. A burte force method would be 18-20 times copy code bellow, with different extack message. +++ b/net/xfrm/xfrm_user.c @@ -957,6 +957,11 @@ static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh, struct km_event c; struct xfrm_usersa_id *p = nlmsg_data(nlh); + if (attrs[XFRMA_SA_DIR]) { + NL_SET_ERR_MSG(extack, "Delete should not have dir attribute set"); + return -ESRCH; + } + I am still trying to figure out netlink examples, including the ones you pointed out : rtnl_valid_dump_net_req, rtnl_net_valid_getid_req. I wonder if there is a way to specifiy rejeced attributes per method. may be there is way to call nlmsg_parse_deprecated_strict() with .type = NLA_REJECT. And also this looks like a general cleanup up to me. I wonder how Steffen would add such a check for the upcoming PCPU attribute! Should that be prohibited DELSA or XFRM_MSG_FLUSHSA or DELSA? -antony
2024-04-16, 09:10:25 +0200, Antony Antony wrote: > On Mon, Apr 15, 2024 at 02:21:50PM +0200, Sabrina Dubroca via Devel wrote: > > 2024-04-11, 11:40:59 +0200, Antony Antony wrote: > > > 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; > > > + } > > > > It would be nice to set x->dir to match the flag, but then I guess the > > validation in xfrm_state_update would fail if userspaces tries an > > update without providing XFRMA_SA_DIR. (or not because we already went > > through this code by the time we get to xfrm_state_update?) > > this code already executed from xfrm_state_construct. > We could set the in flag in xuo when x->dir == XFRM_SA_DIR_IN, let me think > again. May be we can do that later:) I mean setting x->dir, not setting xuo, ie adding something like this to xfrm_dev_state_add: x->dir = xuo->flags & XFRM_OFFLOAD_INBOUND ? XFRM_SA_DIR_IN : XFRM_SA_DIR_OUT; xuo will already be set correctly when we're using offload, and won't be present if we're not. > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > > > index 810b520493f3..df141edbe8d1 100644 > > > --- a/net/xfrm/xfrm_user.c > > > +++ b/net/xfrm/xfrm_user.c > > > + return -EINVAL; > > > + } > > > + > > > + if (x->replay_esn) { > > > + if (x->replay_esn->replay_window > 1) { > > > + NL_SET_ERR_MSG(extack, > > > + "Replay window should be 1 for OUT SA with ESN"); > > > > I don't think that we should introduce something we know doesn't make > > sense (replay window = 1 on output). It will be API and we won't be > > able to fix it up later. We get a chance to make things nice and > > reasonable with this new attribute, let's not waste it. > > > > As I said, AFAICT replay_esn->replay_window isn't used on output, so > > unless I missed something, it should just be a matter of changing the > > validation. The additional checks in this version should guarantee we > > don't have dir==OUT SAs in the packet input path, so this should work. > > I agree. Your message and Steffen's message helped me figure out, > how to allow replay-window zero for output SA; > It is in v11. Nice, thanks. > > [...] > > > static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > > > struct nlattr **attrs, struct netlink_ext_ack *extack) > > > { > > > @@ -796,6 +881,16 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > > > if (!x) > > > return err; > > > > > > + if (x->dir) { > > > + err = verify_sa_dir(x, extack); > > > + if (err) { > > > + x->km.state = XFRM_STATE_DEAD; > > > + xfrm_dev_state_delete(x); > > > + xfrm_state_put(x); > > > + return err; > > > > That's not very nice. We're creating a state and just throwing it away > > immediately. How hard would it be to validate all that directly from > > verify_newsa_info instead? > > Your proposal would introduce redundant code, requiring accessing attributes > in verify_newsa_info() and other functions. > > The way I propsed, a state x, xfrm_state, is created but it remains > km.stae=XFRM_STATE_VOID. > Newely added verify is before auditing and generating new genid changes, > xfrm_state_add() or xfrm_state_update() would be called later. So deleteing > a state just after xfrm_staet_constructi() is not bad! > > So I think the current code is cleaner, avoiding the need redundant code in > verify_newsa_info(). Avoids a few easy accesses to the netlink attributes, but allocating a state and all its associated info just to throw it away almost immediately is not "cleaner" IMO. > > And as we discussed, I'd really like XFRMA_SA_DIR to be rejected in > > commands that don't use its value. > > I still don't see how to add such a check to about 20 functions. A burte > force method would be 18-20 times copy code bellow, with different extack > message. Yes, I think with the current netlink infrastructure and a single shared policy for all netlink message types, that's what we have to do. Doing it in the netlink core (or with help of the netlink core) seems difficult, as only the caller (xfrm_user) has all the information about which attributes are acceptable with each message type. > +++ b/net/xfrm/xfrm_user.c > @@ -957,6 +957,11 @@ static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > struct km_event c; > struct xfrm_usersa_id *p = nlmsg_data(nlh); > > + if (attrs[XFRMA_SA_DIR]) { > + NL_SET_ERR_MSG(extack, "Delete should not have dir attribute set"); > + return -ESRCH; > + } > + > > I am still trying to figure out netlink examples, including the ones you > pointed out : rtnl_valid_dump_net_req, rtnl_net_valid_getid_req. These do pretty much what you wrote. > I wonder if there is a way to specifiy rejeced attributes per method. > > may be there is way to call nlmsg_parse_deprecated_strict() > with .type = NLA_REJECT. For that, we'd have to separate the policies for each netlink command. Otherwise NLA_REJECT will reject the SA_DIR attribute for all commands, which is not what we want. > And also this looks like a general cleanup up to me. I wonder how Steffen > would add such a check for the upcoming PCPU attribute! Should that be > prohibited DELSA or XFRM_MSG_FLUSHSA or DELSA? IMO, new attributes should be rejected in any handler that doesn't use them. That's not a general cleanup because it's a new attribute, and the goal is to allow us to decide later if we want to use that attribute in DELSA etc. Maybe in one year, we want to make DELSA able to match on SA_DIR. If we don't reject SA_DIR from DELSA now, we won't be able to do that. That's why I'm insisting on this.
Hi Sabrina, On Tue, Apr 16, 2024 at 10:36:16AM +0200, Sabrina Dubroca wrote: > 2024-04-16, 09:10:25 +0200, Antony Antony wrote: > > On Mon, Apr 15, 2024 at 02:21:50PM +0200, Sabrina Dubroca via Devel wrote: > > > 2024-04-11, 11:40:59 +0200, Antony Antony wrote: > > > > 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; > > > > + } > > > > > > It would be nice to set x->dir to match the flag, but then I guess the > > > validation in xfrm_state_update would fail if userspaces tries an > > > update without providing XFRMA_SA_DIR. (or not because we already went > > > through this code by the time we get to xfrm_state_update?) > > > > this code already executed from xfrm_state_construct. > > We could set the in flag in xuo when x->dir == XFRM_SA_DIR_IN, let me think > > again. May be we can do that later:) > > I mean setting x->dir, not setting xuo, ie adding something like this > to xfrm_dev_state_add: > > x->dir = xuo->flags & XFRM_OFFLOAD_INBOUND ? XFRM_SA_DIR_IN : XFRM_SA_DIR_OUT; > > xuo will already be set correctly when we're using offload, and won't > be present if we're not. Updating with older tools may fail validation. For instance, if a user creates an SA using an older iproute2 with offload and XFRM_OFFLOAD_INBOUND flag set, the kernel sets x->dir = XFRM_SA_DIR_IN. Then, if the user wants to update this SA using the same older iproute2, which doesn't allow setting dir, the update will fail. However, as I proposed, if SA dir "in" and offload is enabled, the kernel could set xuo->flags &= XFRM_OFFLOAD_INBOUND to avoid double typing. I've considered this, but I'm unsure of any side effects. Also this could be added later, which is why I've ignored it for now:) > > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > > > > index 810b520493f3..df141edbe8d1 100644 > > > > --- a/net/xfrm/xfrm_user.c > > > > +++ b/net/xfrm/xfrm_user.c > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (x->replay_esn) { > > > > + if (x->replay_esn->replay_window > 1) { > > > > + NL_SET_ERR_MSG(extack, > > > > + "Replay window should be 1 for OUT SA with ESN"); > > > > > > I don't think that we should introduce something we know doesn't make > > > sense (replay window = 1 on output). It will be API and we won't be > > > able to fix it up later. We get a chance to make things nice and > > > reasonable with this new attribute, let's not waste it. > > > > > > As I said, AFAICT replay_esn->replay_window isn't used on output, so > > > unless I missed something, it should just be a matter of changing the > > > validation. The additional checks in this version should guarantee we > > > don't have dir==OUT SAs in the packet input path, so this should work. > > > > I agree. Your message and Steffen's message helped me figure out, > > how to allow replay-window zero for output SA; > > It is in v11. > > Nice, thanks. > > > > [...] > > > > static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > > > > struct nlattr **attrs, struct netlink_ext_ack *extack) > > > > { > > > > @@ -796,6 +881,16 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > > > > if (!x) > > > > return err; > > > > > > > > + if (x->dir) { > > > > + err = verify_sa_dir(x, extack); > > > > + if (err) { > > > > + x->km.state = XFRM_STATE_DEAD; > > > > + xfrm_dev_state_delete(x); > > > > + xfrm_state_put(x); > > > > + return err; > > > > > > That's not very nice. We're creating a state and just throwing it away > > > immediately. How hard would it be to validate all that directly from > > > verify_newsa_info instead? > > > > Your proposal would introduce redundant code, requiring accessing attributes > > in verify_newsa_info() and other functions. > > > > The way I propsed, a state x, xfrm_state, is created but it remains > > km.stae=XFRM_STATE_VOID. > > Newely added verify is before auditing and generating new genid changes, > > xfrm_state_add() or xfrm_state_update() would be called later. So deleteing > > a state just after xfrm_staet_constructi() is not bad! > > > > So I think the current code is cleaner, avoiding the need redundant code in > > verify_newsa_info(). > > Avoids a few easy accesses to the netlink attributes, but allocating a > state and all its associated info just to throw it away almost > immediately is not "cleaner" IMO. > > > And as we discussed, I'd really like XFRMA_SA_DIR to be rejected in > > > commands that don't use its value. > > > > I still don't see how to add such a check to about 20 functions. A burte > > force method would be 18-20 times copy code bellow, with different extack > > message. > > Yes, I think with the current netlink infrastructure and a single > shared policy for all netlink message types, that's what we have to > do. Doing it in the netlink core (or with help of the netlink core) > seems difficult, as only the caller (xfrm_user) has all the > information about which attributes are acceptable with each message > type. yes. If we use netlink infrastructure to reject attributes in some methods shared policy is not ideal. I tried NLA_POLICY_VALIDATE_FN() that function does not get message type as an arguent. So add a seperate function in v11. > > > +++ b/net/xfrm/xfrm_user.c > > @@ -957,6 +957,11 @@ static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh, > > struct km_event c; > > struct xfrm_usersa_id *p = nlmsg_data(nlh); > > > > + if (attrs[XFRMA_SA_DIR]) { > > + NL_SET_ERR_MSG(extack, "Delete should not have dir attribute set"); > > + return -ESRCH; > > + } > > + > > > > I am still trying to figure out netlink examples, including the ones you > > pointed out : rtnl_valid_dump_net_req, rtnl_net_valid_getid_req. > > These do pretty much what you wrote. > > > I wonder if there is a way to specifiy rejeced attributes per method. > > > > may be there is way to call nlmsg_parse_deprecated_strict() > > with .type = NLA_REJECT. > > For that, we'd have to separate the policies for each netlink > command. Otherwise NLA_REJECT will reject the SA_DIR attribute for all > commands, which is not what we want. > > > And also this looks like a general cleanup up to me. I wonder how Steffen > > would add such a check for the upcoming PCPU attribute! Should that be > > prohibited DELSA or XFRM_MSG_FLUSHSA or DELSA? > > IMO, new attributes should be rejected in any handler that doesn't use > them. That's not a general cleanup because it's a new attribute, and > the goal is to allow us to decide later if we want to use that > attribute in DELSA etc. Maybe in one year, we want to make DELSA able > to match on SA_DIR. If we don't reject SA_DIR from DELSA now, we won't > be able to do that. That's why I'm insisting on this. I have implemented a method to reject in v11, even though it is not my preference:) My argument xfrm has no precedence of limiting unused attributes in most types. We are not enforcing on all attributes such as upcoming PCPU. That is why I think it is a general clean up. I have also tweaked error messages. Removed ESN from it, in v11 verifications for ESN and non-ESN are similar. Since output SA with ESN allows replay window 0. -antony
2024-04-22, 00:04:55 +0200, Antony Antony wrote: > Hi Sabrina, > > On Tue, Apr 16, 2024 at 10:36:16AM +0200, Sabrina Dubroca wrote: > > 2024-04-16, 09:10:25 +0200, Antony Antony wrote: > > > On Mon, Apr 15, 2024 at 02:21:50PM +0200, Sabrina Dubroca via Devel wrote: > > > > 2024-04-11, 11:40:59 +0200, Antony Antony wrote: > > > > > 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; > > > > > + } > > > > > > > > It would be nice to set x->dir to match the flag, but then I guess the > > > > validation in xfrm_state_update would fail if userspaces tries an > > > > update without providing XFRMA_SA_DIR. (or not because we already went > > > > through this code by the time we get to xfrm_state_update?) > > > > > > this code already executed from xfrm_state_construct. > > > We could set the in flag in xuo when x->dir == XFRM_SA_DIR_IN, let me think > > > again. May be we can do that later:) > > > > I mean setting x->dir, not setting xuo, ie adding something like this > > to xfrm_dev_state_add: > > > > x->dir = xuo->flags & XFRM_OFFLOAD_INBOUND ? XFRM_SA_DIR_IN : XFRM_SA_DIR_OUT; > > > > xuo will already be set correctly when we're using offload, and won't > > be present if we're not. > > Updating with older tools may fail validation. For instance, if a user creates > an SA using an older iproute2 with offload and XFRM_OFFLOAD_INBOUND flag > set, the kernel sets x->dir = XFRM_SA_DIR_IN. Then, if the user wants to > update this SA using the same older iproute2, which doesn't allow setting > dir, the update will fail. I'm not sure it would, since as you said xfrm_state_construct would have set x->dir based on XFRM_OFFLOAD_INBOUND. But if that's the case, then that can be added later, because it would not change any behavior. > However, as I proposed, if SA dir "in" and offload is enabled, the kernel > could set xuo->flags &= XFRM_OFFLOAD_INBOUND to avoid double typing. Do you mean in iproute? On the kernel side, xuo has to be provided when offloading, and the meaning of (xuo->flags & XFRM_OFFLOAD_INBOUND) is well defined (0 = out, !0 = in). xuo->flags & XFRM_OFFLOAD_INBOUND == 0 with SA_DIR == IN must remain an invalid config. > > > And also this looks like a general cleanup up to me. I wonder how Steffen > > > would add such a check for the upcoming PCPU attribute! Should that be > > > prohibited DELSA or XFRM_MSG_FLUSHSA or DELSA? > > > > IMO, new attributes should be rejected in any handler that doesn't use > > them. That's not a general cleanup because it's a new attribute, and > > the goal is to allow us to decide later if we want to use that > > attribute in DELSA etc. Maybe in one year, we want to make DELSA able > > to match on SA_DIR. If we don't reject SA_DIR from DELSA now, we won't > > be able to do that. That's why I'm insisting on this. > > I have implemented a method to reject in v11, even though it is not my > preference:) My argument xfrm has no precedence of limiting unused > attributes in most types. We are not enforcing on all attributes such as > upcoming PCPU. I'll ask Steffen to enforce it there as well :) I think it's a mistake that old netlink APIs were too friendly to invalid input.
Le 22/04/2024 à 11:16, Sabrina Dubroca a écrit : [snip] >>>> And also this looks like a general cleanup up to me. I wonder how Steffen >>>> would add such a check for the upcoming PCPU attribute! Should that be >>>> prohibited DELSA or XFRM_MSG_FLUSHSA or DELSA? >>> >>> IMO, new attributes should be rejected in any handler that doesn't use >>> them. That's not a general cleanup because it's a new attribute, and >>> the goal is to allow us to decide later if we want to use that >>> attribute in DELSA etc. Maybe in one year, we want to make DELSA able >>> to match on SA_DIR. If we don't reject SA_DIR from DELSA now, we won't >>> be able to do that. That's why I'm insisting on this. >> >> I have implemented a method to reject in v11, even though it is not my >> preference:) My argument xfrm has no precedence of limiting unused >> attributes in most types. We are not enforcing on all attributes such as >> upcoming PCPU. > > I'll ask Steffen to enforce it there as well :) > I think it's a mistake that old netlink APIs were too friendly to invalid input. +1 This is an old problem in Netlink. There has been work during the last years to be more strict about new attributes. For example, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=56738f4608417
Hi Sabrina, On Mon, Apr 22, 2024 at 11:16:31AM +0200, Sabrina Dubroca via Devel wrote: > 2024-04-22, 00:04:55 +0200, Antony Antony wrote: > > Hi Sabrina, > > > > On Tue, Apr 16, 2024 at 10:36:16AM +0200, Sabrina Dubroca wrote: > > > 2024-04-16, 09:10:25 +0200, Antony Antony wrote: > > > > On Mon, Apr 15, 2024 at 02:21:50PM +0200, Sabrina Dubroca via Devel wrote: > > > > > 2024-04-11, 11:40:59 +0200, Antony Antony wrote: > > > > > > 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; > > > > > > + } > > > > > > > > > > It would be nice to set x->dir to match the flag, but then I guess the > > > > > validation in xfrm_state_update would fail if userspaces tries an > > > > > update without providing XFRMA_SA_DIR. (or not because we already went > > > > > through this code by the time we get to xfrm_state_update?) > > > > > > > > this code already executed from xfrm_state_construct. > > > > We could set the in flag in xuo when x->dir == XFRM_SA_DIR_IN, let me think > > > > again. May be we can do that later:) > > > > > > I mean setting x->dir, not setting xuo, ie adding something like this > > > to xfrm_dev_state_add: > > > > > > x->dir = xuo->flags & XFRM_OFFLOAD_INBOUND ? XFRM_SA_DIR_IN : XFRM_SA_DIR_OUT; > > > > > > xuo will already be set correctly when we're using offload, and won't > > > be present if we're not. > > > > Updating with older tools may fail validation. For instance, if a user creates > > an SA using an older iproute2 with offload and XFRM_OFFLOAD_INBOUND flag > > set, the kernel sets x->dir = XFRM_SA_DIR_IN. Then, if the user wants to > > update this SA using the same older iproute2, which doesn't allow setting > > dir, the update will fail. > > I'm not sure it would, since as you said xfrm_state_construct would > have set x->dir based on XFRM_OFFLOAD_INBOUND. But if that's the case, > then that can be added later, because it would not change any behavior. > > > However, as I proposed, if SA dir "in" and offload is enabled, the kernel > > could set xuo->flags &= XFRM_OFFLOAD_INBOUND to avoid double typing. > > Do you mean in iproute? I was thinking kernel. Then the API would be simple iproute2 or *swans. ip xfrm state .... dir in offload dev mlx0 dir in notice "dir in" twice. this can be tweaked later:) However, this would work only with newer kernels. > > On the kernel side, xuo has to be provided when offloading, and the > meaning of (xuo->flags & XFRM_OFFLOAD_INBOUND) is well defined (0 = > out, !0 = in). xuo->flags & XFRM_OFFLOAD_INBOUND == 0 with SA_DIR == > IN must remain an invalid config. yes I agree.
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..f7771a69ae2e 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; @@ -1857,6 +1858,9 @@ int xfrm_state_update(struct xfrm_state *x) if (!x1) goto out; + if (x1->dir != x->dir) + goto out; + if (xfrm_state_kern(x1)) { to_put = x1; err = -EEXIST; diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 810b520493f3..df141edbe8d1 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -360,6 +360,16 @@ 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; + } + } + out: return err; } @@ -627,6 +637,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 +672,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) @@ -779,6 +793,77 @@ static struct xfrm_state *xfrm_state_construct(struct net *net, return NULL; } +static int verify_sa_dir(const struct xfrm_state *x, struct netlink_ext_ack *extack) +{ + if (x->dir == XFRM_SA_DIR_OUT) { + if (x->props.replay_window > 0) { + NL_SET_ERR_MSG(extack, "Replay window should not be set for OUT SA"); + return -EINVAL; + } + + if (x->replay.seq || x->replay.bitmap) { + NL_SET_ERR_MSG(extack, + "Replay seq, or bitmap should not be set for OUT SA with ESN"); + return -EINVAL; + } + + if (x->replay_esn) { + if (x->replay_esn->replay_window > 1) { + NL_SET_ERR_MSG(extack, + "Replay window should be 1 for OUT SA with ESN"); + return -EINVAL; + } + + if (x->replay_esn->seq || x->replay_esn->seq_hi || x->replay_esn->bmp_len) { + NL_SET_ERR_MSG(extack, + "Replay seq, seq_hi, bmp_len should not be set for OUT SA with ESN"); + return -EINVAL; + } + } + + if (x->props.flags & XFRM_STATE_DECAP_DSCP) { + NL_SET_ERR_MSG(extack, "Flag NDECAP_DSCP should not be set for OUT SA"); + return -EINVAL; + } + + if (x->props.flags & XFRM_STATE_ICMP) { + NL_SET_ERR_MSG(extack, "Flag ICMP should not be set for OUT SA"); + return -EINVAL; + } + + if (x->mapping_maxage) { + NL_SET_ERR_MSG(extack, "MTIMER_THRESH should not be set for OUT SA"); + return -EINVAL; + } + } else { + if (x->replay.oseq) { + NL_SET_ERR_MSG(extack, + "Replay oseq should not be set for IN SA"); + return -EINVAL; + } + + if (x->replay_esn) { + if (x->replay_esn->oseq || x->replay_esn->oseq_hi) { + NL_SET_ERR_MSG(extack, + "Replay oseq and oseq_hi should not be set for IN SA with ESN"); + return -EINVAL; + } + } + + if (x->props.flags & XFRM_STATE_NOPMTUDISC) { + NL_SET_ERR_MSG(extack, "Flag NOPMTUDISC should not be set for IN SA"); + return -EINVAL; + } + + if (x->xflags & XFRM_SA_XFLAG_DONT_ENCAP_DSCP) { + NL_SET_ERR_MSG(extack, "Flag DONT_ENCAP_DSCP should not be set for IN SA"); + return -EINVAL; + } + } + + return 0; +} + static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, struct nlattr **attrs, struct netlink_ext_ack *extack) { @@ -796,6 +881,16 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh, if (!x) return err; + if (x->dir) { + err = verify_sa_dir(x, extack); + if (err) { + x->km.state = XFRM_STATE_DEAD; + xfrm_dev_state_delete(x); + xfrm_state_put(x); + return err; + } + } + xfrm_state_hold(x); if (nlh->nlmsg_type == XFRM_MSG_NEWSA) err = xfrm_state_add(x); @@ -1182,8 +1277,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; } @@ -1579,12 +1679,22 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh, u32 mark; struct xfrm_mark m; u32 if_id = 0; + u8 sa_dir = 0; p = nlmsg_data(nlh); err = verify_spi_info(p->info.id.proto, p->min, p->max, extack); if (err) goto out_noput; + if (attrs[XFRMA_SA_DIR]) { + 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_noput; + } + } + family = p->info.family; daddr = &p->info.id.daddr; @@ -1618,6 +1728,8 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh, if (err) goto out; + x->dir = sa_dir; + resp_skb = xfrm_state_netlink(skb, x, nlh->nlmsg_seq); if (IS_ERR(resp_skb)) { err = PTR_ERR(resp_skb); @@ -2402,7 +2514,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 +2572,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 +3137,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 +3169,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 +3310,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 +3339,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 +3452,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; }
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> --- v9->v10: - add more direction specific validations XFRM_STATE_NOPMTUDISC, XFRM_SA_XFLAG_DONT_ENCAP_DSCP XFRMA_MTIMER_THRESH - refactor validations into a fuction. - add dir to ALLOCSPI to support strongSwan updating SPI for "in" state v8->v9: - add validation XFRM_STATE_ICMP not allowed on OUT SA. v7->v8: - add extra validation check on replay window and seq - XFRM_MSG_UPDSA old and new SA should match "dir" 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 | 4 ++ net/xfrm/xfrm_user.c | 139 ++++++++++++++++++++++++++++++++++++-- 6 files changed, 157 insertions(+), 6 deletions(-) -- 2.30.2