Re: [PATCH ipsec-next v10 1/3] xfrm: Add Direction to the SA in or out

[Sabrina Dubroca <sd@queasysnail.net>]

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.

-- 
Sabrina