netdev - Re: [PATCH v2] af_unix: don't post cmsg for SO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <willemdebruijn.kernel.18e89ba05fbac@gmail.com>
Date: Fri, 19 Dec 2025 14:02:32 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jens Axboe <axboe@...nel.dk>, 
 netdev <netdev@...r.kernel.org>
Cc: io-uring <io-uring@...r.kernel.org>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Kuniyuki Iwashima <kuniyu@...gle.com>, 
 Julian Orth <ju.orth@...il.com>
Subject: Re: [PATCH v2] af_unix: don't post cmsg for SO_INQ unless explicitly
 asked for

Jens Axboe wrote:
> A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it
> posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is
> incorrect, as ->msg_get_inq is just the caller asking for the remainder
> to be passed back in msg->msg_inq, it has nothing to do with cmsg. The
> original commit states that this is done to make sockets
> io_uring-friendly", but it's actually incorrect as io_uring doesn't use
> cmsg headers internally at all, and it's actively wrong as this means
> that cmsg's are always posted if someone does recvmsg via io_uring.
> 
> Fix that up by only posting a cmsg if u->recvmsg_inq is set.
> 
> Additionally, mirror how TCP handles inquiry handling in that it should
> only be done for a successful return. This makes the logic for the two
> identical.
> 
> Cc: stable@...r.kernel.org
> Fixes: df30285b3670 ("af_unix: Introduce SO_INQ.")
> Reported-by: Julian Orth <ju.orth@...il.com>
> Link: https://github.com/axboe/liburing/issues/1509
> Signed-off-by: Jens Axboe <axboe@...nel.dk>
> 
> ---
> 
> V2:
> - Unify logic with tcp
> - Squash the two patches into one
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 55cdebfa0da0..a7ca74653d94 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>  	unsigned int last_len;
>  	struct unix_sock *u;
>  	int copied = 0;
> +	bool do_cmsg;
>  	int err = 0;
>  	long timeo;
>  	int target;
> @@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>  
>  	u = unix_sk(sk);
>  
> +	do_cmsg = READ_ONCE(u->recvmsg_inq);
> +	if (do_cmsg)
> +		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way
that applications can even get confused by recvmsg setting msg_flags.
I've seen multiple reports of applications failing to scrub that field
inbetween calls.

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

>  redo:
>  	/* Lock the socket to prevent queue disordering
>  	 * while sleeps in memcpy_tomsg
> @@ -3088,10 +3092,11 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>  	if (msg) {
>  		scm_recv_unix(sock, msg, &scm, flags);
>  
> -		if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
> +		if (msg->msg_get_inq && (copied ?: err) >= 0) {
>  			msg->msg_inq = READ_ONCE(u->inq_len);
> -			put_cmsg(msg, SOL_SOCKET, SCM_INQ,
> -				 sizeof(msg->msg_inq), &msg->msg_inq);
> +			if (do_cmsg)
> +				put_cmsg(msg, SOL_SOCKET, SCM_INQ,
> +					 sizeof(msg->msg_inq), &msg->msg_inq);
>  		}
>  	} else {
>  		scm_destroy(&scm);
> -- 
> Jens Axboe
> 
>