[PATCH v1 0/2] Fix issues in xfrm_migrate

[PATCH v1 0/2] Fix issues in xfrm_migrate

[Yan Yan]

This patch series include two patches to fix two issues in xfrm_migrate.

PATCH 1/2 enables distinguishing SAs and SPs based on if_id during the
xfrm_migrate flow. It fixes the problem that when there are multiple
existing SPs with the same direction, the same xfrm_selector and
different endpoint addresses, xfrm_migrate might fail.

PATCH 2/2 enables xfrm_migrate to handle address family change by
breaking the original xfrm_state_clone method into two steps so as to
update the props.family before running xfrm_init_state.

Yan Yan (2):
  xfrm: Check if_id in xfrm_migrate
  xfrm: Fix xfrm migrate issues when address family changes

 include/net/xfrm.h     |  5 +++--
 net/key/af_key.c       |  2 +-
 net/xfrm/xfrm_policy.c | 14 ++++++++------
 net/xfrm/xfrm_state.c  | 40 ++++++++++++++++++++++++++++------------
 net/xfrm/xfrm_user.c   |  6 +++++-
 5 files changed, 45 insertions(+), 22 deletions(-)


base-commit: 18343b80691560f41c3339119a2e9314d4672c77
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v1 1/2] xfrm: Check if_id in xfrm_migrate

[Yan Yan]

This patch enables distinguishing SAs and SPs based on if_id during
the xfrm_migrate flow. This ensures support for xfrm interfaces
throughout the SA/SP lifecycle.

When there are multiple existing SPs with the same direction,
the same xfrm_selector and different endpoint addresses,
xfrm_migrate might fail with ENODATA.

Specifically, the code path for performing xfrm_migrate is:
  Stage 1: find policy to migrate with
    xfrm_migrate_policy_find(sel, dir, type, net)
  Stage 2: find and update state(s) with
    xfrm_migrate_state_find(mp, net)
  Stage 3: update endpoint address(es) of template(s) with
    xfrm_policy_migrate(pol, m, num_migrate)

Currently "Stage 1" always returns the first xfrm_policy that
matches, and "Stage 3" looks for the xfrm_tmpl that matches the
old endpoint address. Thus if there are multiple xfrm_policy
with same selector, direction, type and net, "Stage 1" might
rertun a wrong xfrm_policy and "Stage 3" will fail with ENODATA
because it cannot find a xfrm_tmpl with the matching endpoint
address.

The fix is to allow userspace to pass an if_id and add if_id
to the matching rule in Stage 1 and Stage 2 since if_id is a
unique ID for xfrm_policy and xfrm_state. For compatibility,
if_id will only be checked if the attribute is set.

Tested with additions to Android's kernel unit test suite:
https://android-review.googlesource.com/c/kernel/tests/+/1668886

Signed-off-by: Yan Yan <evitayan@google.com>
---
 include/net/xfrm.h     |  5 +++--
 net/key/af_key.c       |  2 +-
 net/xfrm/xfrm_policy.c | 14 ++++++++------
 net/xfrm/xfrm_state.c  |  7 ++++++-
 net/xfrm/xfrm_user.c   |  6 +++++-
 5 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 83b46da8873d..faee50259fd4 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1676,14 +1676,15 @@ int km_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 	       const struct xfrm_migrate *m, int num_bundles,
 	       const struct xfrm_kmaddress *k,
 	       const struct xfrm_encap_tmpl *encap);
-struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net);
+struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net,
+						u32 if_id);
 struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
 				      struct xfrm_migrate *m,
 				      struct xfrm_encap_tmpl *encap);
 int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		 struct xfrm_migrate *m, int num_bundles,
 		 struct xfrm_kmaddress *k, struct net *net,
-		 struct xfrm_encap_tmpl *encap);
+		 struct xfrm_encap_tmpl *encap, u32 if_id);
 #endif
 
 int km_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, __be16 sport);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index de24a7d474df..9bf52a09b5ff 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2623,7 +2623,7 @@ static int pfkey_migrate(struct sock *sk, struct sk_buff *skb,
 	}
 
 	return xfrm_migrate(&sel, dir, XFRM_POLICY_TYPE_MAIN, m, i,
-			    kma ? &k : NULL, net, NULL);
+			    kma ? &k : NULL, net, NULL, 0);
 
  out:
 	return err;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 1a06585022ab..cd656b4ec5b9 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4235,7 +4235,7 @@ static bool xfrm_migrate_selector_match(const struct xfrm_selector *sel_cmp,
 }
 
 static struct xfrm_policy *xfrm_migrate_policy_find(const struct xfrm_selector *sel,
-						    u8 dir, u8 type, struct net *net)
+						    u8 dir, u8 type, struct net *net, u32 if_id)
 {
 	struct xfrm_policy *pol, *ret = NULL;
 	struct hlist_head *chain;
@@ -4244,7 +4244,8 @@ static struct xfrm_policy *xfrm_migrate_policy_find(const struct xfrm_selector *
 	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
 	chain = policy_hash_direct(net, &sel->daddr, &sel->saddr, sel->family, dir);
 	hlist_for_each_entry(pol, chain, bydst) {
-		if (xfrm_migrate_selector_match(sel, &pol->selector) &&
+		if ((if_id == 0 || pol->if_id == if_id) &&
+		    xfrm_migrate_selector_match(sel, &pol->selector) &&
 		    pol->type == type) {
 			ret = pol;
 			priority = ret->priority;
@@ -4256,7 +4257,8 @@ static struct xfrm_policy *xfrm_migrate_policy_find(const struct xfrm_selector *
 		if ((pol->priority >= priority) && ret)
 			break;
 
-		if (xfrm_migrate_selector_match(sel, &pol->selector) &&
+		if ((if_id == 0 || pol->if_id == if_id) &&
+		    xfrm_migrate_selector_match(sel, &pol->selector) &&
 		    pol->type == type) {
 			ret = pol;
 			break;
@@ -4372,7 +4374,7 @@ static int xfrm_migrate_check(const struct xfrm_migrate *m, int num_migrate)
 int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		 struct xfrm_migrate *m, int num_migrate,
 		 struct xfrm_kmaddress *k, struct net *net,
-		 struct xfrm_encap_tmpl *encap)
+		 struct xfrm_encap_tmpl *encap, u32 if_id)
 {
 	int i, err, nx_cur = 0, nx_new = 0;
 	struct xfrm_policy *pol = NULL;
@@ -4391,14 +4393,14 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 	}
 
 	/* Stage 1 - find policy */
-	if ((pol = xfrm_migrate_policy_find(sel, dir, type, net)) == NULL) {
+	if ((pol = xfrm_migrate_policy_find(sel, dir, type, net, if_id)) == NULL) {
 		err = -ENOENT;
 		goto out;
 	}
 
 	/* Stage 2 - find and update state(s) */
 	for (i = 0, mp = m; i < num_migrate; i++, mp++) {
-		if ((x = xfrm_migrate_state_find(mp, net))) {
+		if (x = xfrm_migrate_state_find(mp, net, if_id)) {
 			x_cur[nx_cur] = x;
 			nx_cur++;
 			xc = xfrm_state_migrate(x, mp, encap);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 0407272a990c..f52767685b13 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1603,7 +1603,8 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
 	return NULL;
 }
 
-struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net)
+struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net,
+						u32 if_id)
 {
 	unsigned int h;
 	struct xfrm_state *x = NULL;
@@ -1619,6 +1620,8 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 				continue;
 			if (m->reqid && x->props.reqid != m->reqid)
 				continue;
+			if (if_id != 0 && x->if_id != if_id)
+				continue;
 			if (!xfrm_addr_equal(&x->id.daddr, &m->old_daddr,
 					     m->old_family) ||
 			    !xfrm_addr_equal(&x->props.saddr, &m->old_saddr,
@@ -1634,6 +1637,8 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 			if (x->props.mode != m->mode ||
 			    x->id.proto != m->proto)
 				continue;
+			if (if_id != 0 && x->if_id != if_id)
+				continue;
 			if (!xfrm_addr_equal(&x->id.daddr, &m->old_daddr,
 					     m->old_family) ||
 			    !xfrm_addr_equal(&x->props.saddr, &m->old_saddr,
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index e3e26f4da6c2..eda02c5544ae 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2580,6 +2580,7 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
 	int n = 0;
 	struct net *net = sock_net(skb->sk);
 	struct xfrm_encap_tmpl  *encap = NULL;
+	u32 if_id = 0;
 
 	if (attrs[XFRMA_MIGRATE] == NULL)
 		return -EINVAL;
@@ -2604,7 +2605,10 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
 			return -ENOMEM;
 	}
 
-	err = xfrm_migrate(&pi->sel, pi->dir, type, m, n, kmp, net, encap);
+	if (attrs[XFRMA_IF_ID])
+		if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
+
+	err = xfrm_migrate(&pi->sel, pi->dir, type, m, n, kmp, net, encap, if_id);
 
 	kfree(encap);
 
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v1 2/2] xfrm: Fix xfrm migrate issues when address family changes

[Yan Yan]

xfrm_migrate cannot handle address family change of an xfrm_state.
The symptons are the xfrm_state will be migrated to a wrong address,
and sending as well as receiving packets wil be broken.

This commit fixes it by breaking the original xfrm_state_clone
method into two steps so as to update the props.family before
running xfrm_init_state. As the result, xfrm_state's inner mode,
outer mode, type and IP header length in xfrm_state_migrate can
be updated with the new address family.

Tested with additions to Android's kernel unit test suite:
https://android-review.googlesource.com/c/kernel/tests/+/1885354

Signed-off-by: Yan Yan <evitayan@google.com>
---
 net/xfrm/xfrm_state.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index f52767685b13..cac212039bf9 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1504,8 +1504,8 @@ static inline int clone_security(struct xfrm_state *x, struct xfrm_sec_ctx *secu
 	return 0;
 }
 
-static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
-					   struct xfrm_encap_tmpl *encap)
+static struct xfrm_state *xfrm_state_clone1(struct xfrm_state *orig,
+					    struct xfrm_encap_tmpl *encap)
 {
 	struct net *net = xs_net(orig);
 	struct xfrm_state *x = xfrm_state_alloc(net);
@@ -1579,8 +1579,20 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
 	memcpy(&x->mark, &orig->mark, sizeof(x->mark));
 	memcpy(&x->props.smark, &orig->props.smark, sizeof(x->props.smark));
 
-	if (xfrm_init_state(x) < 0)
-		goto error;
+	return x;
+
+ error:
+	xfrm_state_put(x);
+out:
+	return NULL;
+}
+
+static int xfrm_state_clone2(struct xfrm_state *orig, struct xfrm_state *x)
+{
+	int err = xfrm_init_state(x);
+
+	if (err < 0)
+		return err;
 
 	x->props.flags = orig->props.flags;
 	x->props.extra_flags = orig->props.extra_flags;
@@ -1595,12 +1607,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
 	x->replay = orig->replay;
 	x->preplay = orig->preplay;
 
-	return x;
-
- error:
-	xfrm_state_put(x);
-out:
-	return NULL;
+	return 0;
 }
 
 struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net,
@@ -1661,10 +1668,14 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
 {
 	struct xfrm_state *xc;
 
-	xc = xfrm_state_clone(x, encap);
+	xc = xfrm_state_clone1(x, encap);
 	if (!xc)
 		return NULL;
 
+	xc->props.family = m->new_family;
+	if (xfrm_state_clone2(x, xc) < 0)
+		goto error;
+
 	memcpy(&xc->id.daddr, &m->new_daddr, sizeof(xc->id.daddr));
 	memcpy(&xc->props.saddr, &m->new_saddr, sizeof(xc->props.saddr));
 
-- 
2.34.1.448.ga2b2bfdf31-goog

Re: [PATCH v1 2/2] xfrm: Fix xfrm migrate issues when address family changes

[Steffen Klassert]

On Wed, Jan 05, 2022 at 04:52:51PM -0800, Yan Yan wrote:

...

> -static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> -					   struct xfrm_encap_tmpl *encap)
> +static struct xfrm_state *xfrm_state_clone1(struct xfrm_state *orig,
> +					    struct xfrm_encap_tmpl *encap)
>  {
>  	struct net *net = xs_net(orig);
>  	struct xfrm_state *x = xfrm_state_alloc(net);
> @@ -1579,8 +1579,20 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
>  	memcpy(&x->mark, &orig->mark, sizeof(x->mark));
>  	memcpy(&x->props.smark, &orig->props.smark, sizeof(x->props.smark));
>  
> -	if (xfrm_init_state(x) < 0)
> -		goto error;
> +	return x;
> +
> + error:
> +	xfrm_state_put(x);
> +out:
> +	return NULL;
> +}
> +
> +static int xfrm_state_clone2(struct xfrm_state *orig, struct xfrm_state *x)

I'm not a frind of numbering function names, this just invites to
create xfrm_state_clone3 :)

> +{
> +	int err = xfrm_init_state(x);
> +
> +	if (err < 0)
> +		return err;
>  
>  	x->props.flags = orig->props.flags;
>  	x->props.extra_flags = orig->props.extra_flags;
> @@ -1595,12 +1607,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
>  	x->replay = orig->replay;
>  	x->preplay = orig->preplay;
>  
> -	return x;
> -
> - error:
> -	xfrm_state_put(x);
> -out:
> -	return NULL;
> +	return 0;
>  }
>  
>  struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net,
> @@ -1661,10 +1668,14 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
>  {
>  	struct xfrm_state *xc;
>  
> -	xc = xfrm_state_clone(x, encap);
> +	xc = xfrm_state_clone1(x, encap);
>  	if (!xc)
>  		return NULL;
>  
> +	xc->props.family = m->new_family;
> +	if (xfrm_state_clone2(x, xc) < 0)
> +		goto error;

xfrm_state_migrate() is the only function that calls xfrm_state_clone().
Wouldn't it be better to move xfrm_init_state() out of xfrm_state_clone()
and call it afterwards?

This would also fix the replay window initialization on ESN because
currently x->props.flags (which holds XFRM_STATE_ESN) is initialized
after xfrm_init_state().

Re: [PATCH v1 2/2] xfrm: Fix xfrm migrate issues when address family changes

[Yan Yan]

Thanks Steffen, I will pull out xfrm_init_state() as you suggested.

On Tue, Jan 11, 2022 at 11:57 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Wed, Jan 05, 2022 at 04:52:51PM -0800, Yan Yan wrote:
>
> ...
>
> > -static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> > -                                        struct xfrm_encap_tmpl *encap)
> > +static struct xfrm_state *xfrm_state_clone1(struct xfrm_state *orig,
> > +                                         struct xfrm_encap_tmpl *encap)
> >  {
> >       struct net *net = xs_net(orig);
> >       struct xfrm_state *x = xfrm_state_alloc(net);
> > @@ -1579,8 +1579,20 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> >       memcpy(&x->mark, &orig->mark, sizeof(x->mark));
> >       memcpy(&x->props.smark, &orig->props.smark, sizeof(x->props.smark));
> >
> > -     if (xfrm_init_state(x) < 0)
> > -             goto error;
> > +     return x;
> > +
> > + error:
> > +     xfrm_state_put(x);
> > +out:
> > +     return NULL;
> > +}
> > +
> > +static int xfrm_state_clone2(struct xfrm_state *orig, struct xfrm_state *x)
>
> I'm not a frind of numbering function names, this just invites to
> create xfrm_state_clone3 :)
>
> > +{
> > +     int err = xfrm_init_state(x);
> > +
> > +     if (err < 0)
> > +             return err;
> >
> >       x->props.flags = orig->props.flags;
> >       x->props.extra_flags = orig->props.extra_flags;
> > @@ -1595,12 +1607,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> >       x->replay = orig->replay;
> >       x->preplay = orig->preplay;
> >
> > -     return x;
> > -
> > - error:
> > -     xfrm_state_put(x);
> > -out:
> > -     return NULL;
> > +     return 0;
> >  }
> >
> >  struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net,
> > @@ -1661,10 +1668,14 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
> >  {
> >       struct xfrm_state *xc;
> >
> > -     xc = xfrm_state_clone(x, encap);
> > +     xc = xfrm_state_clone1(x, encap);
> >       if (!xc)
> >               return NULL;
> >
> > +     xc->props.family = m->new_family;
> > +     if (xfrm_state_clone2(x, xc) < 0)
> > +             goto error;
>
> xfrm_state_migrate() is the only function that calls xfrm_state_clone().
> Wouldn't it be better to move xfrm_init_state() out of xfrm_state_clone()
> and call it afterwards?
>
> This would also fix the replay window initialization on ESN because
> currently x->props.flags (which holds XFRM_STATE_ESN) is initialized
> after xfrm_init_state().
>


-- 
--
Best,
Yan

[PATCH v1 0/2] Fix issues in xfrm_migrate

[Yan Yan]

This patch series include two patches to fix two issues in xfrm_migrate.

PATCH 1/2 enables distinguishing SAs and SPs based on if_id during the
xfrm_migrate flow. It fixes the problem that when there are multiple
existing SPs with the same direction, the same xfrm_selector and
different endpoint addresses, xfrm_migrate might fail.

PATCH 2/2 enables xfrm_migrate to handle address family change by
breaking the original xfrm_state_clone method into two steps so as to
update the props.family before running xfrm_init_state.

Yan Yan (2):
  xfrm: Check if_id in xfrm_migrate
  xfrm: Fix xfrm migrate issues when address family changes

 include/net/xfrm.h     |  5 +++--
 net/xfrm/xfrm_policy.c | 14 ++++++++------
 net/xfrm/xfrm_state.c  | 38 +++++++++++++++++++++++++++-----------
 net/xfrm/xfrm_user.c   |  6 +++++-
 4 files changed, 43 insertions(+), 20 deletions(-)

-- 
2.34.1.307.g9b7440fafd-goog