netdev - Re: [PATCH 1/2] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <willemdebruijn.kernel.164466b751181@gmail.com>
Date: Thu, 18 Dec 2025 16:15:09 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jens Axboe <axboe@...nel.dk>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 netdev@...r.kernel.org
Cc: io-uring@...r.kernel.org, 
 kuba@...nel.org, 
 kuniyu@...gle.com, 
 willemb@...gle.com, 
 stable@...r.kernel.org, 
 Julian Orth <ju.orth@...il.com>
Subject: Re: [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly
 asked for

Jens Axboe wrote:
> On 12/18/25 1:35 PM, Willem de Bruijn wrote:
> > 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 cmsg if u->recvmsg_inq is set.
> >>
> >> 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>
> >> ---
> >>  net/unix/af_unix.c | 10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >> index 55cdebfa0da0..110d716087b5 100644
> >> --- a/net/unix/af_unix.c
> >> +++ b/net/unix/af_unix.c
> >> @@ -3086,12 +3086,16 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> >>  
> >>  	mutex_unlock(&u->iolock);
> >>  	if (msg) {
> >> +		bool do_cmsg;
> >> +
> >>  		scm_recv_unix(sock, msg, &scm, flags);
> >>  
> >> -		if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
> >> +		do_cmsg = READ_ONCE(u->recvmsg_inq);
> >> +		if (do_cmsg || msg->msg_get_inq) {
> >>  			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);
> > 
> > Is it intentional that msg_inq is set also if msg_get_inq is not set,
> > but do_cmsg is?
> 
> It doesn't really matter, what matters is the actual cmsg posting be
> guarded. The msg_inq should only be used for a successful return anyway,
> I think we're better off reading it unconditionally than having multiple
> branches.
> 
> Not really important, if you prefer to keep them consistent, that's fine
> with me too.
> 
> > 
> > It just seems a bit surprising behavior.
> > 
> > That is an entangling of two separate things.
> > - msg_get_inq sets msg_inq, and
> > - cmsg_flags & TCP_CMSG_INQ inserts TCP_CM_INQ cmsg
> > 
> > The original TCP patch also entangles them, but in another way.
> > The cmsg is written only if msg_get_inq is requested.
> 
> The cmsg is written iff TCP_CMSG_INQ is set, not if ->msg_get_inq is the
> only thing set. That part is important.
> 
> But yes, both need the data left.

I see, writing msg_inq if not requested is benign indeed. The inverse
is not true.

Ok. I do think it would be good to have the protocols consistent.
Simpler to reason about the behavior and intent long term.
 
> -- 
> Jens Axboe