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

[netdev-bot+sashiko@kernel.org]

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