netdev - Re: [PATCH ipsec-next v4 5/5] xfrm: add XFRM_MSG_MIGRATE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aXIA6JdfwK1CEMeX@secunet.com>
Date: Thu, 22 Jan 2026 11:50:16 +0100
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Antony Antony <antony.antony@...unet.com>
CC: 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>, Chiachang Wang
	<chiachangwang@...gle.com>, Yan Yan <evitayan@...gle.com>,
	<devel@...ux-ipsec.org>, Simon Horman <horms@...nel.org>, Paul Moore
	<paul@...l-moore.com>, Stephen Smalley <stephen.smalley.work@...il.com>,
	Ondrej Mosnacek <omosnace@...hat.com>, <linux-kernel@...r.kernel.org>,
	<selinux@...r.kernel.org>
Subject: Re: [PATCH ipsec-next v4 5/5] xfrm: add XFRM_MSG_MIGRATE_STATE for
 single SA migration

On Thu, Jan 22, 2026 at 11:33:42AM +0100, Steffen Klassert wrote:
> On Mon, Jan 19, 2026 at 09:54:55AM +0100, Antony Antony wrote:
> > +
> > +static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
> > +				 struct nlattr **attrs, struct netlink_ext_ack *extack)
> > +{
> > +	int err = -ESRCH;
> > +	struct xfrm_state *x;
> > +	struct net *net = sock_net(skb->sk);
> > +	struct xfrm_encap_tmpl *encap = NULL;
> > +	struct xfrm_user_offload *xuo = NULL;
> > +	struct xfrm_migrate m = { .old_saddr.a4 = 0,};
> > +	struct xfrm_user_migrate_state *um = nlmsg_data(nlh);
> > +
> > +	if (!um->id.spi) {
> > +		NL_SET_ERR_MSG(extack, "Invalid SPI 0x0");
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = copy_from_user_migrate_state(&m, um);
> > +	if (err)
> > +		return err;
> > +
> > +	x = xfrm_user_state_lookup(net, &um->id, attrs, &err);
> > +
> > +	if (x) {
> > +		struct xfrm_state *xc;
> > +
> > +		if (!x->dir) {
> > +			NL_SET_ERR_MSG(extack, "State direction is invalid");
> > +			err = -EINVAL;
> > +			goto error;
> > +		}
> > +
> > +		if (attrs[XFRMA_ENCAP]) {
> > +			encap = kmemdup(nla_data(attrs[XFRMA_ENCAP]),
> > +					sizeof(*encap), GFP_KERNEL);
> > +			if (!encap) {
> > +				err = -ENOMEM;
> > +				goto error;
> > +			}
> > +		}
> > +		if (attrs[XFRMA_OFFLOAD_DEV]) {
> > +			xuo = kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),
> > +				      sizeof(*xuo), GFP_KERNEL);
> > +			if (!xuo) {
> > +				err = -ENOMEM;
> > +				goto error;
> > +			}
> > +		}
> > +		xc = xfrm_state_migrate(x, &m, encap, net, xuo, extack);
> > +		if (xc) {
> > +			xfrm_state_delete(x);
> 
> This xfrm_state_delete() comes too late. You need to synchronize this
> with xfrm_state_clone_and_setup() by holding the state lock.
> Otherwise we might hand out the same sequence number etc. more than
> one time. Unfortunately xfrm_migrate() has the same problem, the
> error recovery mechanism in xfrm_migrate() looks broken.

The xfrm_migrate() problem is nasty, looks like we can't recover
if the state migration fails. We probably have to delete the
states and wait for the userspace to insert new ones.

Another thing, xfrm_user_state_lookup() returns a reference on x,
you should drop it somewhere.