[PATCH net 0/2] connector/cn_proc: fix listener count and socket state cleanup

[PATCH net 0/2] connector/cn_proc: fix listener count and socket state cleanup

[Cen Zhang (Microsoft)]

Two independent bugs in the process events connector, both reachable
by an unprivileged user:

1. proc_event_num_listeners can go negative because cn_proc_mcast_ctl()
   does not validate mcast_op before storing it as socket state. All
   process events are then suppressed for every subscriber.

2. sk_user_data leaks and the listener count stays too high because
   cn_release() frees the state only for multicast group members and
   never drops the count.

Both were found and fixed with the assistance of an LLM-based code
analysis tool and verified by the author.

Cen Zhang (Microsoft) (2):
  connector/cn_proc: reject invalid mcast_op before touching socket
    state
  connector/cn_proc: release socket state and listener count on close

 drivers/connector/cn_proc.c   | 23 ++++++++++++++++++++---
 drivers/connector/connector.c |  7 +++----
 include/linux/cn_proc.h       |  6 ++++++
 3 files changed, 29 insertions(+), 7 deletions(-)


base-commit: 94cea342e8915812b45f93c56e134041c7fdb4d7
-- 
2.55.0

[PATCH net 1/2] connector/cn_proc: reject invalid mcast_op before touching socket state

[Cen Zhang (Microsoft)]

cn_proc_mcast_ctl() does not validate mcast_op before storing it in the
per-socket proc_input, so an invalid value is kept as the socket's
state. The IGNORE case only tests prev_mc_op != PROC_CN_MCAST_IGNORE, so
a following PROC_CN_MCAST_IGNORE decrements proc_event_num_listeners for
a socket that never did LISTEN.

Since commit bfdfdc2f3b70
("connector/cn_proc: Allow non-root users access") any user in the initial
namespaces can send these messages. Alternating an invalid operation and
IGNORE drives the count negative; every proc_*_connector() then returns
early and no process events are delivered to any subscriber, including
root-owned monitors.

Fix by rejecting an operation other than LISTEN or IGNORE before any
per-socket state is created or modified. The default case of the switch
becomes unreachable and is removed.

Fixes: 2aa1f7a1f47c ("connector/cn_proc: Add filtering to fix some bugs")
Cc: stable@vger.kernel.org
Cc: AutonomousCodeSecurity@microsoft.com
Signed-off-by: Cen Zhang (Microsoft) <cenzhang@linux.microsoft.com>
---
The bug was found and this fix was developed with the assistance of an
LLM-based code analysis tool. The analysis, the patch and its testing
were reviewed and verified by the author.

 drivers/connector/cn_proc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 0056ab81fbc3..4f613c0648ae 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -421,6 +421,12 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
 		return;
 	}
 
+	if (mc_op != PROC_CN_MCAST_LISTEN &&
+	    mc_op != PROC_CN_MCAST_IGNORE) {
+		err = EINVAL;
+		goto out;
+	}
+
 	ev_type = valid_event((enum proc_cn_event)ev_type);
 
 	if (ev_type == PROC_EVENT_NONE)
@@ -455,9 +461,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
 		((struct proc_input *)(sk->sk_user_data))->event_type =
 			PROC_EVENT_NONE;
 		break;
-	default:
-		err = EINVAL;
-		break;
 	}
 
 out:
-- 
2.55.0

[PATCH net 2/2] connector/cn_proc: release socket state and listener count on close

[Cen Zhang (Microsoft)]

cn_release() is the netlink release callback for NETLINK_CONNECTOR
sockets. It frees the per-socket proc_input only if the socket is a
member of the CN_IDX_PROC multicast group, and it never touches
proc_event_num_listeners. Both are wrong: cn_proc_mcast_ctl() allocates
proc_input for any socket that sends a control message, and a socket
that is still in LISTEN state when it closes has incremented the count
and never decremented it.

This has two consequences. A socket that sends a control message
without joining the group leaks its proc_input on close; an
unprivileged user looping over socket(), one control message and
close() leaks 8 bytes of kmalloc-8 per iteration that no process exit
gives back, until the OOM killer fires:

  Out of memory: Killed process 93 (trigger) total-vm:1016kB, ... UID:1000

And a listener that exits without sending IGNORE leaves the count too
high, so the kernel keeps building process events that nobody receives.

Fix by moving the cleanup into cn_proc.c and running it for every
socket on close: free proc_input if present, and decrement the count if
the socket was still in LISTEN state.

Fixes: 2aa1f7a1f47c ("connector/cn_proc: Add filtering to fix some bugs")
Cc: stable@vger.kernel.org
Cc: AutonomousCodeSecurity@microsoft.com
Signed-off-by: Cen Zhang (Microsoft) <cenzhang@linux.microsoft.com>
---
The bug was found and this fix was developed with the assistance of an
LLM-based code analysis tool. The analysis, the patch and its testing
were reviewed and verified by the author.

 drivers/connector/cn_proc.c   | 14 ++++++++++++++
 drivers/connector/connector.c |  7 +++----
 include/linux/cn_proc.h       |  6 ++++++
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 4f613c0648ae..a4b62998095e 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -387,6 +387,20 @@ static void cn_proc_ack(int err, int rcvd_seq, int rcvd_ack)
 	send_msg(msg);
 }
 
+/* Called when a NETLINK_CONNECTOR socket is released. */
+void cn_proc_release(struct sock *sk)
+{
+	struct proc_input *input = sk->sk_user_data;
+
+	if (!input)
+		return;
+
+	if (input->mcast_op == PROC_CN_MCAST_LISTEN)
+		atomic_dec(&proc_event_num_listeners);
+	kfree(input);
+	sk->sk_user_data = NULL;
+}
+
 /**
  * cn_proc_mcast_ctl
  * @msg: message sent from userspace via the connector
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 4028e8eeba82..f31ee8962f7c 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -14,6 +14,7 @@
 #include <net/netlink.h>
 #include <linux/moduleparam.h>
 #include <linux/connector.h>
+#include <linux/cn_proc.h>
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/proc_fs.h>
@@ -182,12 +183,10 @@ static int cn_bind(struct net *net, int group)
 	return -EPERM;
 }
 
+/* sk_user_data of a connector socket belongs to cn_proc. */
 static void cn_release(struct sock *sk, unsigned long *groups)
 {
-	if (groups && test_bit(CN_IDX_PROC - 1, groups)) {
-		kfree(sk->sk_user_data);
-		sk->sk_user_data = NULL;
-	}
+	cn_proc_release(sk);
 }
 
 /*
diff --git a/include/linux/cn_proc.h b/include/linux/cn_proc.h
index 1d5b02a96c46..8de921d731ce 100644
--- a/include/linux/cn_proc.h
+++ b/include/linux/cn_proc.h
@@ -19,6 +19,8 @@
 
 #include <uapi/linux/cn_proc.h>
 
+struct sock;
+
 #ifdef CONFIG_PROC_EVENTS
 void proc_fork_connector(struct task_struct *task);
 void proc_exec_connector(struct task_struct *task);
@@ -28,6 +30,7 @@ void proc_ptrace_connector(struct task_struct *task, int which_id);
 void proc_comm_connector(struct task_struct *task);
 void proc_coredump_connector(struct task_struct *task);
 void proc_exit_connector(struct task_struct *task);
+void cn_proc_release(struct sock *sk);
 #else
 static inline void proc_fork_connector(struct task_struct *task)
 {}
@@ -54,5 +57,8 @@ static inline void proc_coredump_connector(struct task_struct *task)
 
 static inline void proc_exit_connector(struct task_struct *task)
 {}
+
+static inline void cn_proc_release(struct sock *sk)
+{}
 #endif	/* CONFIG_PROC_EVENTS */
 #endif	/* CN_PROC_H */
-- 
2.55.0

Re: [PATCH net 1/2] connector/cn_proc: reject invalid mcast_op before touching socket state

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] cn_proc_mcast_ctl() changes per-socket state and the global
  proc_event_num_listeners without any serialization, so an unprivileged…

Pre-existing issues:
- [High] At this commit, the per-socket struct proc_input that
  cn_proc_mcast_ctl() allocates (cn_proc.c:438) whenever…

--

connector/cn_proc: reject invalid mcast_op before touching socket state

With this commit, cn_proc_mcast_ctl() rejects any mcast_op other than
PROC_CN_MCAST_LISTEN or PROC_CN_MCAST_IGNORE before per-socket state is
created or modified. It also drops the default case of the switch, which
can no longer be reached. This keeps an invalid op followed by IGNORE from
decrementing proc_event_num_listeners for a socket that never listened.

> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index 0056ab81fbc39..4f613c0648ae4 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -421,6 +421,12 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>  		return;
>  	}
>  
> +	if (mc_op != PROC_CN_MCAST_LISTEN &&
> +	    mc_op != PROC_CN_MCAST_IGNORE) {
> +		err = EINVAL;
> +		goto out;
> +	}
> +
>  	ev_type = valid_event((enum proc_cn_event)ev_type);
>  
>  	if (ev_type == PROC_EVENT_NONE)

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can the
proc_input allocated a little further down in cn_proc_mcast_ctl() leak?

drivers/connector/cn_proc.c:cn_proc_mcast_ctl() {
    ...
		if (sk->sk_user_data == NULL) {
			sk->sk_user_data = kzalloc_obj(struct proc_input);
    ...
}

This allocation happens for any socket that sends LISTEN or IGNORE to
portid 0, whether or not it has joined CN_IDX_PROC. At this commit the only
free is in cn_release(), and it runs only if the socket is a group member
at close:

drivers/connector/connector.c:cn_release() {
	if (groups && test_bit(CN_IDX_PROC - 1, groups)) {
		kfree(sk->sk_user_data);
		sk->sk_user_data = NULL;
	}
}

netlink_sock_destruct() does not free sk_user_data either. A socket that
never joins the group, or leaves it before close, keeps its proc_input.
A user in the initial namespaces could repeat socket(), send and close to
leak kmalloc-8 objects without limit.

A later patch in the series appears to resolve this: "connector/cn_proc:
release socket state and listener count on close" makes cn_release() call
cn_proc_release() unconditionally, and cn_proc_release() frees
sk_user_data.

[ ... ]

> @@ -455,9 +461,6 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>  		((struct proc_input *)(sk->sk_user_data))->event_type =
>  			PROC_EVENT_NONE;
>  		break;
> -	default:
> -		err = EINVAL;
> -		break;
>  	}
>  
>  out:

[Severity: High]
Even with the new mcast_op check, can concurrent senders on the same
socket still drive proc_event_num_listeners negative?

The commit message describes this outcome:

  "drives the count negative; every proc_*_connector() then returns
  early and no process events are delivered to any subscriber"

As far as I can see, nothing serializes cn_proc_mcast_ctl() for a given
socket:

- netlink_sendmsg() does not take the socket lock.
- netlink_unicast_kernel() calls nlk->netlink_rcv() (cn_rx_skb) in the
  sender's context.
- cn_call_callback() drops queue_lock before it runs the callback:

drivers/connector/connector.c:cn_call_callback() {
    ...
	spin_unlock_bh(&dev->cbdev->queue_lock);

	if (cbq != NULL) {
		cbq->callback(msg, nsp);
    ...
}

Inside cn_proc_mcast_ctl(), reading prev_mc_op, storing the new mcast_op
and doing the inc/dec are separate unlocked steps:

		} else {
			prev_mc_op =
			((struct proc_input *)(sk->sk_user_data))->mcast_op;
		}
		...
		((struct proc_input *)(sk->sk_user_data))->mcast_op = mc_op;
	...
	case PROC_CN_MCAST_IGNORE:
		if (!initial && (prev_mc_op != PROC_CN_MCAST_IGNORE))
			atomic_dec(&proc_event_num_listeners);

Take two threads sharing one NETLINK_CONNECTOR fd, after a single LISTEN:

thread A                          thread B
IGNORE: prev_mc_op = LISTEN
                                  IGNORE: prev_mc_op = LISTEN
mcast_op = IGNORE
                                  mcast_op = IGNORE
atomic_dec()
                                  atomic_dec()

That is a net -1 from this socket. Another LISTEN followed by another
racing pair repeats it. The window is narrow, but it can be retried
indefinitely. Once the count is below 1, every proc_*_connector() returns
early, and no subscriber gets events, including root-owned monitors.

Two racing first messages can also both see sk_user_data == NULL. Both then
kzalloc and both set initial = 1. One proc_input is overwritten and leaked,
and two LISTENs add +2 for a single socket.

The later "connector/cn_proc: release socket state and listener count on
close" patch in the series adds another path. Suppose a LISTEN races an
IGNORE that saw prev_mc_op == LISTEN, and the LISTEN's store of mcast_op
lands last. The IGNORE has already decremented, and cn_proc_release()
decrements again on close:

	if (input->mcast_op == PROC_CN_MCAST_LISTEN)
		atomic_dec(&proc_event_num_listeners);

Would it make sense to serialize the per-socket update? One option is a
lock around the allocation and the prev/new update. Another is xchg() on
mcast_op, with the inc/dec chosen from the returned old value.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102130922.75629-1-cenzhang%40linux.microsoft.com

Re: [PATCH net 2/2] connector/cn_proc: release socket state and listener count on close

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch makes cn_proc_release() (drivers/connector/cn_proc.c)
  the single place that cleans up when a NETLINK_CONNECTOR socket closes.

--

connector/cn_proc: release socket state and listener count on close

This moves the NETLINK_CONNECTOR release cleanup into a new
cn_proc_release() helper, which now runs for every socket on close. The
helper frees sk->sk_user_data and decrements proc_event_num_listeners
when the stored mcast_op is PROC_CN_MCAST_LISTEN. The aim is to fix the
proc_input leak and the listener count imbalance.

> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index 4f613c0648ae4..a4b62998095ea 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -387,6 +387,20 @@ static void cn_proc_ack(int err, int rcvd_seq, int rcvd_ack)
>  	send_msg(msg);
>  }
>  
> +/* Called when a NETLINK_CONNECTOR socket is released. */
> +void cn_proc_release(struct sock *sk)
> +{
> +	struct proc_input *input = sk->sk_user_data;
> +
> +	if (!input)
> +		return;
> +
> +	if (input->mcast_op == PROC_CN_MCAST_LISTEN)
> +		atomic_dec(&proc_event_num_listeners);
> +	kfree(input);
> +	sk->sk_user_data = NULL;
> +}

[Severity: High]
This cleanup assumes two things. Each socket owns at most one
proc_input, and proc_event_num_listeners holds exactly one increment for
each socket whose stored mcast_op is LISTEN. Does cn_proc_mcast_ctl()
actually guarantee either of these?

Nothing on the send path serialises it per socket. In
netlink_sendmsg()->netlink_unicast()->netlink_unicast_kernel(),
nlk->netlink_rcv() is called with no lock_sock(). cn_call_callback()
holds queue_lock only for the lookup:

drivers/connector/connector.c:cn_call_callback() {
    ...
	spin_unlock_bh(&dev->cbdev->queue_lock);

	if (cbq != NULL) {
		cbq->callback(msg, nsp);
    ...
}

As a result, two threads sharing one fd can run this section of
cn_proc_mcast_ctl() at the same time:

drivers/connector/cn_proc.c:cn_proc_mcast_ctl() {
    ...
		if (sk->sk_user_data == NULL) {
			sk->sk_user_data = kzalloc_obj(struct proc_input);
			...
			initial = 1;
		} else {
			prev_mc_op =
			((struct proc_input *)(sk->sk_user_data))->mcast_op;
		}
		...
		((struct proc_input *)(sk->sk_user_data))->mcast_op = mc_op;
	...
	case PROC_CN_MCAST_LISTEN:
		if (initial || (prev_mc_op != PROC_CN_MCAST_LISTEN))
			atomic_inc(&proc_event_num_listeners);
		break;
	case PROC_CN_MCAST_IGNORE:
		if (!initial && (prev_mc_op != PROC_CN_MCAST_IGNORE))
			atomic_dec(&proc_event_num_listeners);
    ...
}

Can this still leak the proc_input? If both threads see sk_user_data as
NULL, each calls kzalloc and the second store overwrites the first
pointer. cn_proc_release() frees only the pointer that survives. A loop
of socket(), two concurrent control messages and close() would then
still leak kmalloc-8, much like the OOM reproducer in the commit
message.

Can the count still be left too high? Both threads can take the
increment path, for example when both are initial LISTENs, or when the
second thread reads the freshly published zeroed mcast_op (0, which is
not PROC_CN_MCAST_LISTEN) before the first thread stores LISTEN.
cn_proc_release() decrements only once.

There is also an interleaving where the new release-time decrement
seems to push the count too low:

thread A (LISTEN, fresh socket)
    sk->sk_user_data = kzalloc_obj(...)   /* mcast_op == 0 */
    initial = 1

thread B (IGNORE)
    prev_mc_op = 0
    atomic_dec(&proc_event_num_listeners)
    mcast_op = PROC_CN_MCAST_IGNORE

thread A
    mcast_op = PROC_CN_MCAST_LISTEN
    atomic_inc(&proc_event_num_listeners)

close()
    cn_proc_release()
        atomic_dec(&proc_event_num_listeners)

Before this patch that sequence left the counter balanced. With this
patch, each round lowers proc_event_num_listeners by one.

Once the count drops below 1, proc_fork_connector() and the other
proc_*_connector() hooks, as well as cn_proc_ack(), return early. Would
that stop process events for every listener on the system? The
reachability checks are cn_bind() for CN_IDX_PROC and the init user/pid
namespace test in cn_proc_mcast_ctl(), so an unprivileged user can
trigger this.

Would it make sense to serialise cn_proc_mcast_ctl() per socket, for
example with lock_sock(sk) or a mutex around the sk_user_data install
and the mcast_op transition plus counter update? Another option is a
cmpxchg() install of sk_user_data together with an xchg() of mcast_op,
using the old value to pick the inc or dec.

>  /**
>   * cn_proc_mcast_ctl
>   * @msg: message sent from userspace via the connector

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102130922.75629-1-cenzhang%40linux.microsoft.com