Re: [net,v2] net: unix: reject negative max_dgram_qlen values

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Incomplete mitigation: the patch rejects negative writes to
  net.unix.max_dgram_qlen but supplies no .extra1/.extra2 bounds to…
- [Medium] Commit message / Fixes:+stable justification does not match
  what the code does.

--

net: unix: reject negative max_dgram_qlen values

This changes netns_unix::sysctl_max_dgram_qlen from int to unsigned int
and switches the net.unix.max_dgram_qlen handler from proc_dointvec to
proc_douintvec_minmax, so negative writes are rejected while the full u32
range stays writable.  The documentation entry is updated accordingly.

Two questions, one on the changelog and one on the sysctl table entry.

First the changelog.  It describes a concrete malfunction:

>     result, negative values are accepted and converted to large queue limits;
>     for example, -1 becomes UINT_MAX.  A nonblocking sender can then continue
>     enqueueing instead of receiving EAGAIN at a finite limit.

and it carries:

>     Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>     Cc: stable@vger.kernel.org

[Severity: Medium]

Is that description still accurate after this patch?  The message also
says the intent is to "let userspace configure the full u32 range
directly", and with no .extra1/.extra2 in the table entry a write of
4294967295 is still accepted, so sk_max_ack_backlog can still end up at
UINT_MAX by way of unix_create1().

If the sk_max_ack_backlog == UINT_MAX state remains reachable, then the
only behavioural delta is stricter parsing of a namespace-writable knob:
writes of negative values now return -EINVAL where they previously
succeeded.

Would it be clearer to describe this as an input validation and typing
change, and to reconsider the Fixes: and Cc: stable tags, given that a
backporter reading the message would expect a queue-limit malfunction to
be closed and would accept the user-visible tightening on that basis?

[ ... ]

> diff --git a/net/unix/sysctl_net_unix.c b/net/unix/sysctl_net_unix.c
> index 47660d5726bb..8f6f0657495a 100644
> --- a/net/unix/sysctl_net_unix.c
> +++ b/net/unix/sysctl_net_unix.c
> @@ -17,9 +17,9 @@ static const struct ctl_table unix_table[] = {
>  	{
>  		.procname	= "max_dgram_qlen",
>  		.data		= &init_net.unx.sysctl_max_dgram_qlen,
> -		.maxlen		= sizeof(int),
> +		.maxlen		= sizeof(unsigned int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec
> +		.proc_handler	= proc_douintvec_minmax,
>  	},
>  };

[Severity: High]

Does the min/max handler do any bounding here without .extra1/.extra2?
In kernel/sysctl.c, do_proc_uint_conv_minmax() calls proc_uint_conv()
with the range check enabled, but the check itself is conditional on the
extra pointers:

kernel/sysctl.c:proc_uint_conv() {
	...
		if ((tbl->extra1 &&
		     *(uint *)tbl->extra1 > tmp_k) ||
		    (tbl->extra2 &&
		     *(uint *)tbl->extra2 < tmp_k))
			return -ERANGE;
	...
}

and the conversion only rejects values above UINT_MAX:

kernel/sysctl.c:proc_uint_u2k_conv_uop() {
	...
	if (u > UINT_MAX)
		return -EINVAL;
	WRITE_ONCE(*k_ptr, u);
	...
}

So does echo 4294967295 > /proc/sys/net/unix/max_dgram_qlen still store
UINT_MAX?  That value reaches every new AF_UNIX socket:

net/unix/af_unix.c:unix_create1() {
	...
	sk->sk_max_ack_backlog	= READ_ONCE(net->unx.sysctl_max_dgram_qlen);
	...
}

and the queue-full test compares a __u32 queue length against it:

net/unix/af_unix.c:unix_recvq_full_lockless() {
	return skb_queue_len_lockless(&sk->sk_receive_queue) > sk->sk_max_ack_backlog;
}

Since sk_buff_head::qlen is itself __u32, can this comparison ever
evaluate true when the limit is UINT_MAX?  If it cannot, the gate in
unix_dgram_sendmsg():

	if (other != sk &&
	    unlikely(unix_peer(other) != sk &&
	    unix_recvq_full_lockless(other))) {
		if (timeo) {
	...

never trips, and the sender falls through to
skb_queue_tail(&other->sk_receive_queue, skb) rather than returning
EAGAIN, which is the same end state that -1 produced before this patch.

If so, would adding a sane .extra2 cap (or, alternatively, documenting
that very large values intentionally disable the limit) be the more
complete change here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_9FDB4FAAFE9F3EF38210AC963C73897A2E09%40qq.com