From: Kyle Zeng <kylebot@openai.com>

espintcp_sendmsg() builds a new message directly in ctx->partial. If
allocation fails, sk_stream_wait_memory() drops the socket lock while
the shared sk_msg remains unpublished with emsg->len equal to zero. A
concurrent sender can then reuse the same slot. If the first sender is
interrupted, its failure path frees state now owned by the second sender
while TCP may still be consuming it, causing a use-after-free.

Construct the message in a call-local sk_msg instead. After allocation
and any lock-dropping wait, recheck that the shared partial slot is still
free, then transfer the completed message into it. Failure cleanup
consequently releases only state owned by the current call. The recheck
also covers packets submitted through the common IPv4 and IPv6
espintcp_push_skb() path.

Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6-sol
Signed-off-by: Kyle Zeng <kylebot@openai.com>
Signed-off-by: Bruno Produit <bruno.produit@trailofbits.com>
---
 net/xfrm/espintcp.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
index 674aedc..1642b34 100644
--- a/net/xfrm/espintcp.c
+++ b/net/xfrm/espintcp.c
@@ -311,6 +311,7 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	struct espintcp_msg *emsg = &ctx->partial;
 	struct iov_iter pfx_iter;
 	struct kvec pfx_iov = {};
+	struct sk_msg *skmsg;
 	size_t msglen = size + 2;
 	char buf[2] = {0};
 	int err, end;
@@ -324,6 +325,11 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	if (msg->msg_controllen)
 		return -EOPNOTSUPP;
 
+	skmsg = kmalloc_obj(*skmsg);
+	if (!skmsg)
+		return -ENOMEM;
+	sk_msg_init(skmsg);
+
 	lock_sock(sk);
 
 	err = espintcp_push_msgs(sk, msg->msg_flags & MSG_DONTWAIT);
@@ -337,10 +343,9 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 		goto unlock;
 	}
 
-	sk_msg_init(&emsg->skmsg);
 	while (1) {
 		/* only -ENOMEM is possible since we don't coalesce */
-		err = sk_msg_alloc(sk, &emsg->skmsg, msglen, 0);
+		err = sk_msg_alloc(sk, skmsg, msglen, 0);
 		if (!err)
 			break;
 
@@ -348,25 +353,30 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 		if (err)
 			goto fail;
 	}
+	if (emsg->len) {
+		err = -ENOBUFS;
+		goto fail;
+	}
 
 	*((__be16 *)buf) = cpu_to_be16(msglen);
 	pfx_iov.iov_base = buf;
 	pfx_iov.iov_len = sizeof(buf);
 	iov_iter_kvec(&pfx_iter, ITER_SOURCE, &pfx_iov, 1, pfx_iov.iov_len);
 
-	err = sk_msg_memcopy_from_iter(sk, &pfx_iter, &emsg->skmsg,
+	err = sk_msg_memcopy_from_iter(sk, &pfx_iter, skmsg,
 				       pfx_iov.iov_len);
 	if (err < 0)
 		goto fail;
 
-	err = sk_msg_memcopy_from_iter(sk, &msg->msg_iter, &emsg->skmsg, size);
+	err = sk_msg_memcopy_from_iter(sk, &msg->msg_iter, skmsg, size);
 	if (err < 0)
 		goto fail;
 
-	end = emsg->skmsg.sg.end;
-	emsg->len = size;
+	end = skmsg->sg.end;
 	sk_msg_iter_var_prev(end);
-	sg_mark_end(sk_msg_elem(&emsg->skmsg, end));
+	sg_mark_end(sk_msg_elem(skmsg, end));
+	sk_msg_xfer_full(&emsg->skmsg, skmsg);
+	emsg->len = size;
 
 	tcp_rate_check_app_limited(sk);
 
@@ -374,14 +384,15 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	/* this message could be partially sent, keep it */
 
 	release_sock(sk);
+	kfree(skmsg);
 
 	return size;
 
 fail:
-	sk_msg_free(sk, &emsg->skmsg);
-	memset(emsg, 0, sizeof(*emsg));
+	sk_msg_free(sk, skmsg);
 unlock:
 	release_sock(sk);
+	kfree(skmsg);
 	return err;
 }
 
-- 
2.53.0

2026-09-22, 16:53:35 +0200, Bruno Produit wrote:
> From: Kyle Zeng <kylebot@openai.com>
> 
> espintcp_sendmsg() builds a new message directly in ctx->partial. If
> allocation fails, sk_stream_wait_memory() drops the socket lock while
> the shared sk_msg remains unpublished with emsg->len equal to zero. A
> concurrent sender can then reuse the same slot. If the first sender is

Could we add an ->owned flag to emsg to make the other sender
wait/abort when the flag is set (whether the emsg has been fully set
up or not)?

If not, comments below.

> interrupted, its failure path frees state now owned by the second sender
> while TCP may still be consuming it, causing a use-after-free.
> 
> Construct the message in a call-local sk_msg instead.


> After allocation
> and any lock-dropping wait, recheck that the shared partial slot is still
> free, then transfer the completed message into it. Failure cleanup
> consequently releases only state owned by the current call.

Please don't describe what the patch does. We can read the code.

> The recheck
> also covers packets submitted through the common IPv4 and IPv6
> espintcp_push_skb() path.

I have no idea what this means.


> diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
> index 674aedc..1642b34 100644
> --- a/net/xfrm/espintcp.c
> +++ b/net/xfrm/espintcp.c
> @@ -311,6 +311,7 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  	struct espintcp_msg *emsg = &ctx->partial;
>  	struct iov_iter pfx_iter;
>  	struct kvec pfx_iov = {};
> +	struct sk_msg *skmsg;

nit: reverse xmas tree ordering

>  	size_t msglen = size + 2;
>  	char buf[2] = {0};
>  	int err, end;
> @@ -324,6 +325,11 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  	if (msg->msg_controllen)
>  		return -EOPNOTSUPP;
>  
> +	skmsg = kmalloc_obj(*skmsg);
> +	if (!skmsg)
> +		return -ENOMEM;
> +	sk_msg_init(skmsg);

Why do that before trying (and possibly failing) to push the pending
message?

>  	lock_sock(sk);
>  
>  	err = espintcp_push_msgs(sk, msg->msg_flags & MSG_DONTWAIT);
> @@ -337,10 +343,9 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		goto unlock;
>  	}
>  
> -	sk_msg_init(&emsg->skmsg);
>  	while (1) {
>  		/* only -ENOMEM is possible since we don't coalesce */
> -		err = sk_msg_alloc(sk, &emsg->skmsg, msglen, 0);
> +		err = sk_msg_alloc(sk, skmsg, msglen, 0);
>  		if (!err)
>  			break;
>  
> @@ -348,25 +353,30 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		if (err)
>  			goto fail;
>  	}
> +	if (emsg->len) {
> +		err = -ENOBUFS;
> +		goto fail;
> +	}

Do another espintcp_push_msgs before giving up?

And there should be a comment here to explain why we need to recheck
emsg->len even though we already did at the top (something like "we
may have dropped the lock in sk_stream_wait_memory, check if someone
else used the emsg").

-- 
Sabrina