| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zh0b3gfnr99ddaYM@hog>
Date: Mon, 15 Apr 2024 14:21:50 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antony Antony <antony.antony@...unet.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>,
Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
devel@...ux-ipsec.org, Leon Romanovsky <leon@...nel.org>,
Eyal Birger <eyal.birger@...il.com>,
Nicolas Dichtel <nicolas.dichtel@...nd.com>
Subject: Re: [PATCH ipsec-next v10 1/3] xfrm: Add Direction to the SA in or
out
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
Powered by blists - more mailing lists