netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net: psp: do not inherit the Rx association on clone
@ 2026-08-29 16:56 Norbert Szetei
  2026-08-29 20:40 ` Daniel Zahka
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Norbert Szetei @ 2026-08-29 16:56 UTC (permalink / raw)
  To: netdev
  Cc: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
	David S. Miller, Jakub Kicinski, Simon Horman, Daniel Zahka,
	linux-kernel

sk->psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it into
every socket accepted from a listener without taking a reference, while
inet_sock_destruct() puts for every inet socket. psp_twsk_init() does
refcount_inc() for the timewait socket, so a child closing through
TIME_WAIT cancels its own put and leaves the association with one
reference and N timewait sockets holding the same pointer. Closing the
listener frees it, and the timewait timers then put freed memory.

Rejecting the association on a listening socket is not sufficient: a socket
can acquire one while established and then be turned back into a listener,
because tcp_disconnect() leaves sk->psp_assoc in place.

  BUG: KASAN: slab-use-after-free in psp_twsk_assoc_free+0x6f/0xf0
  Write of size 4 at addr ffff888110f9255c by task swapper/7/0
   psp_twsk_assoc_free+0x6f/0xf0
   inet_twsk_put+0xda/0x1b0
   call_timer_fn+0x53/0x2e0
   __run_timers+0x764/0xa80
  Freed by task 99:
   kfree+0x1a7/0x500
   process_one_work+0x7ec/0x1100

An association carries a per-connection SPI and key, so a child must not
inherit the parent's. Clear it on clone.

Fixes: 6b46ca260e22 ("net: psp: add socket security association code")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
---
Reproducer available on request.

 net/core/sock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/sock.c b/net/core/sock.c
index 1ad41904db25..fa60b7494c58 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2494,6 +2494,9 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
 #ifdef CONFIG_BPF_SYSCALL
 	RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL);
 #endif
+#if IS_ENABLED(CONFIG_INET_PSP)
+	RCU_INIT_POINTER(newsk->psp_assoc, NULL);
+#endif
 
 	/* SANITY */
 	if (likely(newsk->sk_net_refcnt)) {
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH net] net: psp: do not inherit the Rx association on clone
  2026-08-29 16:56 [PATCH net] net: psp: do not inherit the Rx association on clone Norbert Szetei
@ 2026-08-29 20:40 ` Daniel Zahka
  2026-08-30 18:07   ` Norbert Szetei
  2026-09-01 10:01 ` Paolo Abeni
  2026-09-01 13:20 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 7+ messages in thread
From: Daniel Zahka @ 2026-08-29 20:40 UTC (permalink / raw)
  To: Norbert Szetei, netdev
  Cc: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
	David S. Miller, Jakub Kicinski, Simon Horman, Daniel Zahka,
	linux-kernel

On Sat Aug 29, 2026 at 12:56 PM EDT, Norbert Szetei wrote:
> sk->psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it into
> every socket accepted from a listener without taking a reference, while
> inet_sock_destruct() puts for every inet socket. psp_twsk_init() does
> refcount_inc() for the timewait socket, so a child closing through
> TIME_WAIT cancels its own put and leaves the association with one
> reference and N timewait sockets holding the same pointer. Closing the
> listener frees it, and the timewait timers then put freed memory.
>
> Rejecting the association on a listening socket is not sufficient: a socket
> can acquire one while established and then be turned back into a listener,
> because tcp_disconnect() leaves sk->psp_assoc in place.
>
>   BUG: KASAN: slab-use-after-free in psp_twsk_assoc_free+0x6f/0xf0
>   Write of size 4 at addr ffff888110f9255c by task swapper/7/0
>    psp_twsk_assoc_free+0x6f/0xf0
>    inet_twsk_put+0xda/0x1b0
>    call_timer_fn+0x53/0x2e0
>    __run_timers+0x764/0xa80
>   Freed by task 99:
>    kfree+0x1a7/0x500
>    process_one_work+0x7ec/0x1100
>
> An association carries a per-connection SPI and key, so a child must not
> inherit the parent's. Clear it on clone.
>
> Fixes: 6b46ca260e22 ("net: psp: add socket security association code")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Norbert Szetei <norbert@doyensec.com>

Thanks. Nit: I think the commit message overemphasizes the timewait
paths being part of reaching the bug. I suppose that is because the
included trace went that route, but I think anything that happens after
the copying of the socket's psp_assoc without taking a refcount will
reach the same issue.

The fix looks appropriate to me. I also think disallowing rx-assoc might
be something that we ought to do.

Reviewed-by: Daniel Zahka <daniel.zahka@gmail.com>

> ---
> Reproducer available on request.
>

Sure. Let's see it. Here's one with packetdrill and netdevsim:

cat gtests/net/psp/repro/psp-listen-assoc-uaf.pkt 
// Expected on an unfixed kernel:
//
//   BUG: KASAN: slab-use-after-free in psp_assoc_put.part.0+0x1b/0x60
//   Write of size 4 at addr ff11000112918a60 by task swapper/3/0
//    psp_assoc_put.part.0+0x1b/0x60
//    __sk_destruct+0x7b/0x630
//    rcu_core+0x508/0x1470
//   Allocated by task 258:
//    psp_assoc_create+0xb7/0x3f0
//    psp_nl_rx_assoc_doit+0x1cc/0xae0
//   Freed by task 77:
//    kfree+0x321/0x500
//    process_one_work+0x89f/0x18a0
//
//   refcount_t: underflow; use-after-free.

--psp_udp_port=1000

// Initialize a listening socket.
    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 psp_rx_assoc(3, [7]) = 0

// Complete a handshake. The child cloned here silently shares the
// listener's psp_assoc pointer with no reference of its own.
  +.1 < S 0:0(0) win 50000 <mss 1000,nop,wscale 0>
   +0 > S. 0:0(0) ack 1 <mss MSS,nop,wscale 8>
  +.1 < . 1:1(0) ack 1 win 50000
   +0 accept(3, ..., ...) = 4

// Reset the child so close() destroys it right away rather than parking
// it in FIN_WAIT/TIME_WAIT.
  +.1 < R. 1:1(0) ack 1 win 0

// First put drops the only refcnt
   +0 close(4) = 0

  +.5 `sleep 0.5`

// Second put: psp_assoc_put() touches freed memory
   +0 close(3) = 0

// Keep the VM alive long enough for the splat to reach the console.
  +.5 `sleep 1`

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] net: psp: do not inherit the Rx association on clone
  2026-08-29 20:40 ` Daniel Zahka
@ 2026-08-30 18:07   ` Norbert Szetei
  0 siblings, 0 replies; 7+ messages in thread
From: Norbert Szetei @ 2026-08-30 18:07 UTC (permalink / raw)
  To: Daniel Zahka
  Cc: netdev, Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni,
	Willem de Bruijn, David S. Miller, Jakub Kicinski, Simon Horman,
	linux-kernel

On Aug 29, 2026, at 22:40, Daniel Zahka <daniel.zahka@gmail.com> wrote:
> 
> On Sat Aug 29, 2026 at 12:56 PM EDT, Norbert Szetei wrote:
>> sk->psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it into
>> every socket accepted from a listener without taking a reference, while
>> inet_sock_destruct() puts for every inet socket. psp_twsk_init() does
>> refcount_inc() for the timewait socket, so a child closing through
>> TIME_WAIT cancels its own put and leaves the association with one
>> reference and N timewait sockets holding the same pointer. Closing the
>> listener frees it, and the timewait timers then put freed memory.
>> 
>> Rejecting the association on a listening socket is not sufficient: a socket
>> can acquire one while established and then be turned back into a listener,
>> because tcp_disconnect() leaves sk->psp_assoc in place.
>> 
>>  BUG: KASAN: slab-use-after-free in psp_twsk_assoc_free+0x6f/0xf0
>>  Write of size 4 at addr ffff888110f9255c by task swapper/7/0
>>   psp_twsk_assoc_free+0x6f/0xf0
>>   inet_twsk_put+0xda/0x1b0
>>   call_timer_fn+0x53/0x2e0
>>   __run_timers+0x764/0xa80
>>  Freed by task 99:
>>   kfree+0x1a7/0x500
>>   process_one_work+0x7ec/0x1100
>> 
>> An association carries a per-connection SPI and key, so a child must not
>> inherit the parent's. Clear it on clone.
>> 
>> Fixes: 6b46ca260e22 ("net: psp: add socket security association code")
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Norbert Szetei <norbert@doyensec.com>
> 
> Thanks. Nit: I think the commit message overemphasizes the timewait
> paths being part of reaching the bug. I suppose that is because the
> included trace went that route, but I think anything that happens after
> the copying of the socket's psp_assoc without taking a refcount will
> reach the same issue.

Agreed, and your packetdrill test shows it better than my commit log does.
I can drop or change that paragraph in a v2 if you or the maintainers would 
prefer it, otherwise I will leave the patch as is so as not to reset your 
review.

> The fix looks appropriate to me. I also think disallowing rx-assoc might
> be something that we ought to do.

That was my first attempt, and it is how I ended up with this patch
instead. I rejected rx-assoc unless sk_state was TCP_ESTABLISHED, in
psp_sock_assoc_set_rx(). That closes the obvious route, but a socket can
still pick up an association while established and then be turned back
into a listener:

  connect()            -> ESTABLISHED
  rx-assoc             -> allowed, the socket is established
  connect(AF_UNSPEC)   -> tcp_disconnect() has no psp handling, so
                          sk->psp_assoc survives, sk_state is TCP_CLOSE
  listen()             -> __inet_listen_sk() accepts TCPF_CLOSE

which is a listener holding an association again. I still think it is worth 
doing on top of this, though.

> Reviewed-by: Daniel Zahka <daniel.zahka@gmail.com>
> 
>> ---
>> Reproducer available on request.
>> 
> 
> Sure. Let's see it. Here's one with packetdrill and netdevsim:
> 
> cat gtests/net/psp/repro/psp-listen-assoc-uaf.pkt 
> // Expected on an unfixed kernel:
> //
> //   BUG: KASAN: slab-use-after-free in psp_assoc_put.part.0+0x1b/0x60
> //   Write of size 4 at addr ff11000112918a60 by task swapper/3/0
> //    psp_assoc_put.part.0+0x1b/0x60
> //    __sk_destruct+0x7b/0x630
> //    rcu_core+0x508/0x1470
> //   Allocated by task 258:
> //    psp_assoc_create+0xb7/0x3f0
> //    psp_nl_rx_assoc_doit+0x1cc/0xae0
> //   Freed by task 77:
> //    kfree+0x321/0x500
> //    process_one_work+0x89f/0x18a0
> //
> //   refcount_t: underflow; use-after-free.
> 
> --psp_udp_port=1000
> 
> // Initialize a listening socket.
>    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
>   +0 bind(3, ..., ...) = 0
>   +0 listen(3, 1) = 0
> 
>   +0 psp_rx_assoc(3, [7]) = 0
> 
> // Complete a handshake. The child cloned here silently shares the
> // listener's psp_assoc pointer with no reference of its own.
>  +.1 < S 0:0(0) win 50000 <mss 1000,nop,wscale 0>
>   +0 > S. 0:0(0) ack 1 <mss MSS,nop,wscale 8>
>  +.1 < . 1:1(0) ack 1 win 50000
>   +0 accept(3, ..., ...) = 4
> 
> // Reset the child so close() destroys it right away rather than parking
> // it in FIN_WAIT/TIME_WAIT.
>  +.1 < R. 1:1(0) ack 1 win 0
> 
> // First put drops the only refcnt
>   +0 close(4) = 0
> 
>  +.5 `sleep 0.5`
> 
> // Second put: psp_assoc_put() touches freed memory
>   +0 close(3) = 0
> 
> // Keep the VM alive long enough for the splat to reach the console.
>  +.5 `sleep 1`

Mine is a standalone C program that takes the other route, children closed 
normally, so the UAF is triggered from the timewait timers. It uses 
netdevsim too, I ran `echo "1 1" > /sys/bus/netdevsim/new_device` first.

#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <linux/netlink.h>
#include <linux/genetlink.h>

/* from include/uapi/linux/psp.h */
#define PSP_CMD_DEV_GET		1
#define PSP_CMD_RX_ASSOC	8
#define PSP_A_DEV_ID		1
#define PSP_A_ASSOC_DEV_ID	1
#define PSP_A_ASSOC_VERSION	2
#define PSP_A_ASSOC_SOCK_FD	5

#define NR_CHILDREN		4

static int gfd;

static void put_u32(char *buf, int *len, int type, unsigned int val)
{
	struct nlattr *a = (struct nlattr *)(buf + NLMSG_ALIGN(*len));

	a->nla_type = type;
	a->nla_len = NLA_HDRLEN + 4;
	memcpy((char *)a + NLA_HDRLEN, &val, 4);
	*len = NLMSG_ALIGN(*len) + NLA_ALIGN(a->nla_len);
}

/* returns <0 on netlink error, else the reply length */
static int genl(int family, int cmd, int flags, const char *attrs, int alen,
		char *rbuf, int rlen)
{
	struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
	char buf[1024];
	struct nlmsghdr *n = (struct nlmsghdr *)buf;
	struct genlmsghdr *g = (struct genlmsghdr *)NLMSG_DATA(n);
	int len;

	memset(buf, 0, sizeof(buf));
	n->nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN);
	n->nlmsg_type = family;
	n->nlmsg_flags = NLM_F_REQUEST | flags;
	n->nlmsg_seq = 1;
	g->cmd = cmd;
	g->version = 1;
	if (alen) {
		memcpy(buf + NLMSG_ALIGN(n->nlmsg_len), attrs, alen);
		n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + alen;
	}
	if (sendto(gfd, buf, n->nlmsg_len, 0,
		   (struct sockaddr *)&sa, sizeof(sa)) < 0)
		return -errno;
	len = recv(gfd, rbuf, rlen, 0);
	if (len < 0)
		return -errno;
	n = (struct nlmsghdr *)rbuf;
	if (n->nlmsg_type == NLMSG_ERROR)
		return ((struct nlmsgerr *)NLMSG_DATA(n))->error;
	return len;
}

static int get_u32(char *msg, int len, int want, unsigned int *out)
{
	struct nlmsghdr *n = (struct nlmsghdr *)msg;
	struct nlattr *a;
	int alen;

	if (!NLMSG_OK(n, len))
		return -1;
	a = (struct nlattr *)((char *)NLMSG_DATA(n) + GENL_HDRLEN);
	alen = NLMSG_PAYLOAD(n, GENL_HDRLEN);
	for (; alen >= (int)NLA_HDRLEN && a->nla_len >= NLA_HDRLEN &&
	       a->nla_len <= alen;
	     alen -= NLA_ALIGN(a->nla_len),
	     a = (struct nlattr *)((char *)a + NLA_ALIGN(a->nla_len))) {
		if ((a->nla_type & NLA_TYPE_MASK) == want) {
			memcpy(out, (char *)a + NLA_HDRLEN, 4);
			return 0;
		}
	}
	return -1;
}

static int psp_family(void)
{
	struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
	struct { struct nlmsghdr n; struct genlmsghdr g; char b[128]; } req;
	char rbuf[8192];
	struct nlattr *a;
	unsigned int id;
	int len, l;

	memset(&req, 0, sizeof(req));
	req.n.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN);
	req.n.nlmsg_type = GENL_ID_CTRL;
	req.n.nlmsg_flags = NLM_F_REQUEST;
	req.n.nlmsg_seq = 1;
	req.g.cmd = CTRL_CMD_GETFAMILY;
	req.g.version = 1;
	l = req.n.nlmsg_len;
	a = (struct nlattr *)((char *)&req + NLMSG_ALIGN(l));
	a->nla_type = CTRL_ATTR_FAMILY_NAME;
	a->nla_len = NLA_HDRLEN + 4;
	memcpy((char *)a + NLA_HDRLEN, "psp", 4);
	req.n.nlmsg_len = NLMSG_ALIGN(l) + NLA_ALIGN(a->nla_len);

	if (sendto(gfd, &req, req.n.nlmsg_len, 0,
		   (struct sockaddr *)&sa, sizeof(sa)) < 0)
		return -errno;
	len = recv(gfd, rbuf, sizeof(rbuf), 0);
	if (len < 0 || get_u32(rbuf, len, CTRL_ATTR_FAMILY_ID, &id))
		return -ENOENT;
	return id;
}

int main() {
	struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
	struct sockaddr_in sin;
	char attrs[256], rbuf[8192];
	unsigned int devid;
	socklen_t slen;
	int fam, lfd, cfd, afd, alen, e, i;

	gfd = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
	if (gfd < 0) {
		perror("netlink socket");
		return 1;
	}
	bind(gfd, (struct sockaddr *)&sa, sizeof(sa));

	fam = psp_family();
	if (fam < 0) {
		fprintf(stderr, "no \"psp\" netlink family - CONFIG_INET_PSP=y?\n");
		return 1;
	}

	e = genl(fam, PSP_CMD_DEV_GET, NLM_F_DUMP, NULL, 0, rbuf, sizeof(rbuf));
	if (e < 0 || get_u32(rbuf, e, PSP_A_DEV_ID, &devid)) {
		fprintf(stderr, "no PSP device - create a netdevsim port first\n");
		return 1;
	}
	printf("psp device id %u\n", devid);

	lfd = socket(AF_INET, SOCK_STREAM, 0);
	memset(&sin, 0, sizeof(sin));
	sin.sin_family = AF_INET;
	sin.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
	if (bind(lfd, (struct sockaddr *)&sin, sizeof(sin)) ||
	    listen(lfd, NR_CHILDREN * 2)) {
		perror("bind/listen");
		return 1;
	}
	slen = sizeof(sin);
	getsockname(lfd, (struct sockaddr *)&sin, &slen);

	/* nothing checks sk_state, so this lands on a LISTENING socket */
	alen = 0;
	put_u32(attrs, &alen, PSP_A_ASSOC_SOCK_FD, lfd);
	put_u32(attrs, &alen, PSP_A_ASSOC_DEV_ID, devid);
	put_u32(attrs, &alen, PSP_A_ASSOC_VERSION, 0);
	e = genl(fam, PSP_CMD_RX_ASSOC, 0, attrs, alen, rbuf, sizeof(rbuf));
	if (e < 0) {
		fprintf(stderr, "rx-assoc on listener: %s\n", strerror(-e));
		return 1;
	}
	printf("rx-assoc installed on the listening socket (port %d)\n",
	       ntohs(sin.sin_port));

	/* every accepted child aliases the association without a reference */
	for (i = 0; i < NR_CHILDREN; i++) {
		cfd = socket(AF_INET, SOCK_STREAM, 0);
		if (connect(cfd, (struct sockaddr *)&sin, sizeof(sin))) {
			perror("connect");
			return 1;
		}
		afd = accept(lfd, NULL, NULL);
		if (afd < 0) {
			perror("accept");
			return 1;
		}
		close(afd);	/* -> TIME_WAIT, psp_twsk_init() takes a ref */
		close(cfd);
	}
	printf("accepted and closed %d children\n", NR_CHILDREN);

	close(lfd);		/* drops the last ref: association is freed */
	printf("listener closed; waiting ~75s for the TIME_WAIT timers\n");
	fflush(stdout);
	sleep(75);		/* timers fire -> put on freed memory */

	printf("done - check dmesg for the KASAN report\n");
	return 0;
}



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] net: psp: do not inherit the Rx association on clone
  2026-08-29 16:56 [PATCH net] net: psp: do not inherit the Rx association on clone Norbert Szetei
  2026-08-29 20:40 ` Daniel Zahka
@ 2026-09-01 10:01 ` Paolo Abeni
  2026-09-01 12:28   ` Daniel Zahka
  2026-09-01 13:20 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 7+ messages in thread
From: Paolo Abeni @ 2026-09-01 10:01 UTC (permalink / raw)
  To: Norbert Szetei, netdev
  Cc: Eric Dumazet, Kuniyuki Iwashima, Willem de Bruijn,
	David S. Miller, Jakub Kicinski, Simon Horman, Daniel Zahka,
	linux-kernel

On 8/29/26 6:56 PM, Norbert Szetei wrote:
> sk->psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it into
> every socket accepted from a listener without taking a reference, while
> inet_sock_destruct() puts for every inet socket. psp_twsk_init() does
> refcount_inc() for the timewait socket, so a child closing through
> TIME_WAIT cancels its own put and leaves the association with one
> reference and N timewait sockets holding the same pointer. Closing the
> listener frees it, and the timewait timers then put freed memory.
> 
> Rejecting the association on a listening socket is not sufficient: a socket
> can acquire one while established and then be turned back into a listener,
> because tcp_disconnect() leaves sk->psp_assoc in place.

So rejecting the association on listener, and clearing on disconnect
would be enough, right?

I think that would be preferable: it's a pity to add safeguard code to
the datapath due to a syscall (disconnect) used mostly by fuzzers.

/P


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] net: psp: do not inherit the Rx association on clone
  2026-09-01 10:01 ` Paolo Abeni
@ 2026-09-01 12:28   ` Daniel Zahka
  2026-09-01 13:10     ` Paolo Abeni
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Zahka @ 2026-09-01 12:28 UTC (permalink / raw)
  To: Paolo Abeni, Norbert Szetei, netdev
  Cc: Eric Dumazet, Kuniyuki Iwashima, Willem de Bruijn,
	David S. Miller, Jakub Kicinski, Simon Horman, Daniel Zahka,
	linux-kernel

On Tue Sep 1, 2026 at 6:01 AM EDT, Paolo Abeni wrote:
> On 8/29/26 6:56 PM, Norbert Szetei wrote:
>> sk->psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it into
>> every socket accepted from a listener without taking a reference, while
>> inet_sock_destruct() puts for every inet socket. psp_twsk_init() does
>> refcount_inc() for the timewait socket, so a child closing through
>> TIME_WAIT cancels its own put and leaves the association with one
>> reference and N timewait sockets holding the same pointer. Closing the
>> listener frees it, and the timewait timers then put freed memory.
>> 
>> Rejecting the association on a listening socket is not sufficient: a socket
>> can acquire one while established and then be turned back into a listener,
>> because tcp_disconnect() leaves sk->psp_assoc in place.
>
> So rejecting the association on listener, and clearing on disconnect
> would be enough, right?

I think that would solve this problem with sk_clone(), but clearing out
the psp_assoc from the sk anywhere other than the socket destructor
makes me nervous because of the risk of leaking cleartext to the
network, or admitting cleartext the receive queue.

Specifically about tcp_disconnect(), the write queue purge won't save us
from skbs already queued to the device. I suppose if we clear out the
sk_validate_xmit_skb hook we have for psp, those would at least get
dropped first. After that there is still a hazard of the psp_assoc
cleanup deleting a tx key handle, while tx descriptors in the driver
ring are still referencing that. That is something for which we rely
upon psp skbs being socket owned until the driver tx completion path.

All that to say, I think the fix here is the best option.

>
> I think that would be preferable: it's a pity to add safeguard code to
> the datapath due to a syscall (disconnect) used mostly by fuzzers.
>
> /P


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] net: psp: do not inherit the Rx association on clone
  2026-09-01 12:28   ` Daniel Zahka
@ 2026-09-01 13:10     ` Paolo Abeni
  0 siblings, 0 replies; 7+ messages in thread
From: Paolo Abeni @ 2026-09-01 13:10 UTC (permalink / raw)
  To: Daniel Zahka, Norbert Szetei, netdev
  Cc: Eric Dumazet, Kuniyuki Iwashima, Willem de Bruijn,
	David S. Miller, Jakub Kicinski, Simon Horman, linux-kernel

On 9/1/26 2:28 PM, Daniel Zahka wrote:
> On Tue Sep 1, 2026 at 6:01 AM EDT, Paolo Abeni wrote:
>> On 8/29/26 6:56 PM, Norbert Szetei wrote:
>>> sk->psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it into
>>> every socket accepted from a listener without taking a reference, while
>>> inet_sock_destruct() puts for every inet socket. psp_twsk_init() does
>>> refcount_inc() for the timewait socket, so a child closing through
>>> TIME_WAIT cancels its own put and leaves the association with one
>>> reference and N timewait sockets holding the same pointer. Closing the
>>> listener frees it, and the timewait timers then put freed memory.
>>>
>>> Rejecting the association on a listening socket is not sufficient: a socket
>>> can acquire one while established and then be turned back into a listener,
>>> because tcp_disconnect() leaves sk->psp_assoc in place.
>>
>> So rejecting the association on listener, and clearing on disconnect
>> would be enough, right?
> 
> I think that would solve this problem with sk_clone(), but clearing out
> the psp_assoc from the sk anywhere other than the socket destructor
> makes me nervous because of the risk of leaking cleartext to the
> network, or admitting cleartext the receive queue.
> 
> Specifically about tcp_disconnect(), the write queue purge won't save us
> from skbs already queued to the device. 

Ah, right, I did not take in account this path. Makes sense.

/P


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] net: psp: do not inherit the Rx association on clone
  2026-08-29 16:56 [PATCH net] net: psp: do not inherit the Rx association on clone Norbert Szetei
  2026-08-29 20:40 ` Daniel Zahka
  2026-09-01 10:01 ` Paolo Abeni
@ 2026-09-01 13:20 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-09-01 13:20 UTC (permalink / raw)
  To: Norbert Szetei
  Cc: netdev, edumazet, kuniyu, pabeni, willemb, davem, kuba, horms,
	daniel.zahka, linux-kernel

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Sat, 29 Aug 2026 18:56:18 +0200 you wrote:
> sk->psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it into
> every socket accepted from a listener without taking a reference, while
> inet_sock_destruct() puts for every inet socket. psp_twsk_init() does
> refcount_inc() for the timewait socket, so a child closing through
> TIME_WAIT cancels its own put and leaves the association with one
> reference and N timewait sockets holding the same pointer. Closing the
> listener frees it, and the timewait timers then put freed memory.
> 
> [...]

Here is the summary with links:
  - [net] net: psp: do not inherit the Rx association on clone
    https://git.kernel.org/netdev/net/c/1d2929d0850f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-09-01 13:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-29 16:56 [PATCH net] net: psp: do not inherit the Rx association on clone Norbert Szetei
2026-08-29 20:40 ` Daniel Zahka
2026-08-30 18:07   ` Norbert Szetei
2026-09-01 10:01 ` Paolo Abeni
2026-09-01 12:28   ` Daniel Zahka
2026-09-01 13:10     ` Paolo Abeni
2026-09-01 13:20 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).