netdev - Re: [PATCH ipsec-next 4/6] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aWZdTOXTn_YBKKhv@horms.kernel.org>
Date: Tue, 13 Jan 2026 14:57:16 +0000
From: Simon Horman <horms@...nel.org>
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
Subject: Re: [PATCH ipsec-next 4/6] xfrm: add XFRM_MSG_MIGRATE_STATE for
 single SA migration

On Fri, Jan 09, 2026 at 02:38:05PM +0100, Antony Antony wrote:
> Add a new netlink method to migrate a single xfrm_state.
> Unlike the existing migration mechanism (SA + policy), this
> supports migrating only the SA and allows changing the reqid.
> 
> Signed-off-by: Antony Antony <antony.antony@...unet.com>

...

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index ef832ce477b6..04c893e42bc1 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1967,7 +1967,8 @@ static inline int clone_security(struct xfrm_state *x, struct xfrm_sec_ctx *secu
>  
>  static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
>  					   struct xfrm_encap_tmpl *encap,
> -					   struct xfrm_migrate *m)
> +					   struct xfrm_migrate *m,

Hi Antony,

Not strictly related to this patch, but FWIIW, it seems that m could be
const in this call stack.  And, moreover, I think there would be some value
in constifying parameters throughout xfrm.

> +					   struct netlink_ext_ack *extack)
>  {
>  	struct net *net = xs_net(orig);
>  	struct xfrm_state *x = xfrm_state_alloc(net);
> @@ -1979,9 +1980,13 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
>  	memcpy(&x->lft, &orig->lft, sizeof(x->lft));
>  	x->props.mode = orig->props.mode;
>  	x->props.replay_window = orig->props.replay_window;
> -	x->props.reqid = orig->props.reqid;
>  	x->props.saddr = orig->props.saddr;
>  
> +	if (orig->props.reqid != m->new_reqid)
> +		x->props.reqid = m->new_reqid;
> +	else
> +		x->props.reqid = orig->props.reqid;
> +

Claude Code with Review Prompts [1] flags that until the next
patch of this series m->new_reqid is used uninitialised in the following
call stack:

xfrm_do_migrate -> xfrm_migrate -> xfrm_state_migrate -> xfrm_state_clone_and_setup

Also, while I could have missed something, it seems to me that it is
also uninitialised in this call stack:

pfkey_migrate -> xfrm_migrate -> xfrm_state_migrate -> xfrm_state_clone_and_setup

[1] https://github.com/masoncl/review-prompts/

>  	if (orig->aalg) {
>  		x->aalg = xfrm_algo_auth_clone(orig->aalg);
>  		if (!x->aalg)
> @@ -2059,7 +2064,6 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
>  			goto error;
>  	}
>  
> -

nit: this hunk doesn't seem related to the rest of the patch.

>  	x->props.family = m->new_family;
>  	memcpy(&x->id.daddr, &m->new_daddr, sizeof(x->id.daddr));
>  	memcpy(&x->props.saddr, &m->new_saddr, sizeof(x->props.saddr));

...

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c

...

> +static inline unsigned int xfrm_migrate_state_msgsize(bool with_encap, bool with_xuo)

Please don't use the inline keyword in .c files unless there is a
demonstrable - usually performance - reason to do so.
Rather, please let the compiler inline (or not) code as it sees fit.

> +{
> +	return NLMSG_ALIGN(sizeof(struct xfrm_user_migrate_state)) +
> +		(with_encap ? nla_total_size(sizeof(struct xfrm_encap_tmpl)) : 0) +
> +		(with_xuo ? nla_total_size(sizeof(struct xfrm_user_offload)) : 0);
> +}
> +

...

> +static int xfrm_send_migrate_state(const struct xfrm_user_migrate_state *um,
> +				   const struct xfrm_encap_tmpl *encap,
> +				   const struct xfrm_user_offload *xuo)
> +{
> +	int err;
> +	struct sk_buff *skb;
> +	struct net *net = &init_net;
> +
> +	skb = nlmsg_new(xfrm_migrate_state_msgsize(!!encap, !!xuo), GFP_ATOMIC);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	err = build_migrate_state(skb, um, encap, xuo);
> +	if (err < 0) {
> +		WARN_ON(1);
> +		return err;

skb seems to be leaked here.

Also flagged by Review Prompts.

> +	}
> +
> +	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
> +}

...