[PATCH net v2] net: unix: reject negative max_dgram_qlen values

[PATCH net v2] net: unix: reject negative max_dgram_qlen values

[Yingjie Wang]

net.unix.max_dgram_qlen is parsed into a signed int but copied to the
u32 sock::sk_max_ack_backlog when an AF_UNIX socket is created.  As a
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.

The -1 convention in the listen(2) backlog path is separate from this
sysctl; max_dgram_qlen has no documented sentinel value.  Keep it a
non-negative queue length rather than relying on the signed-to-unsigned
conversion.

Using a signed min/max handler would reject negative values, but would
also restrict the maximum to INT_MAX even though both sk_max_ack_backlog
and sk_buff_head::qlen are u32.

Make the backing value unsigned and use proc_douintvec_minmax.  This
rejects negative input while letting userspace configure the full u32
range directly.  The default, zero, and existing nonnegative values
remain unchanged.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5
Signed-off-by: Yingjie Wang <1075151112@qq.com>
---
v2:
  - Change the backing value and handler to unsigned, preserving the full
    u32 range (Kuniyuki Iwashima).
  - Update the commit message to explain why a signed minimum would cap the
    interface at INT_MAX.
  - Document the sysctl as unsigned and clarify that the listen(2) -1
    convention is not a documented max_dgram_qlen sentinel.
v1: https://lore.kernel.org/netdev/tencent_4AD5738B48FDD2FC84E21FE59338C8DBAA05@qq.com/

 Documentation/networking/ip-sysctl.rst | 5 +++--
 include/net/netns/unix.h               | 2 +-
 net/unix/sysctl_net_unix.c             | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 208f469..2fe1ca8 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -3818,8 +3818,9 @@ l3mdev_accept - BOOLEAN
 ``/proc/sys/net/unix/*``
 ========================
 
-max_dgram_qlen - INTEGER
+max_dgram_qlen - UNSIGNED INTEGER
 	The maximum length of dgram socket receive queue
 
-	Default: 10
+	Negative values are rejected.
 
+	Default: 10
diff --git a/include/net/netns/unix.h b/include/net/netns/unix.h
index 9859d13..e233c42 100644
--- a/include/net/netns/unix.h
+++ b/include/net/netns/unix.h
@@ -15,7 +15,7 @@ struct unix_table {
 struct ctl_table_header;
 struct netns_unix {
 	struct unix_table	table;
-	int			sysctl_max_dgram_qlen;
+	unsigned int		sysctl_max_dgram_qlen;
 	struct ctl_table_header	*ctl;
 };
 
diff --git a/net/unix/sysctl_net_unix.c b/net/unix/sysctl_net_unix.c
index 47660d5..8f6f065 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,
 	},
 };
 
-- 
2.43.0

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

[netdev-bot+sashiko]

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