[PATCH net 1/1] net: preserve socketpair output on setup failure

[PATCH net 1/1] net: preserve socketpair output on setup failure

[Steve Grubb]

socketpair() writes the reserved descriptor numbers to userspace before
creating the sockets and their file objects. A later failure therefore
leaves a modified caller's array despite returning an error, contrary to
POSIX. For example, socketpair(AF_UNIX, SOCK_STREAM, -1, fd) returns
EPROTONOSUPPORT but replaces the array's sentinel values with unused fd
numbers.

Defer the output stores until both file objects have been created, before
installing either descriptor. If a store faults, drop both file references
before releasing the reserved descriptor slots. Keep the early descriptor
reservation and the existing setup-failure cleanup paths.

In practice, I doubt anyone notices. But this aligns with the expected
behavior

Signed-off-by: Steve Grubb <sgrubb@redhat.com>
---
 net/socket.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index c05d86e63abf..4cb8662869f0 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1851,10 +1851,7 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec)
 	if (SOCK_NONBLOCK != O_NONBLOCK && (flags & SOCK_NONBLOCK))
 		flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
 
-	/*
-	 * reserve descriptors and make sure we won't fail
-	 * to return them to userland.
-	 */
+	/* Reserve both descriptors before creating the sockets. */
 	fd1 = get_unused_fd_flags(flags);
 	if (unlikely(fd1 < 0))
 		return fd1;
@@ -1865,14 +1862,6 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec)
 		return fd2;
 	}
 
-	err = put_user(fd1, &usockvec[0]);
-	if (err)
-		goto out;
-
-	err = put_user(fd2, &usockvec[1]);
-	if (err)
-		goto out;
-
 	/*
 	 * Obtain the first socket and check if the underlying protocol
 	 * supports the socketpair call.
@@ -1916,6 +1905,16 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec)
 		goto out;
 	}
 
+	/* Publish the descriptors now that it shouldn't fail. */
+	err = put_user(fd1, &usockvec[0]);
+	if (!err)
+		err = put_user(fd2, &usockvec[1]);
+	if (err) {
+		fput(newfile2);
+		fput(newfile1);
+		goto out;
+	}
+
 	audit_fd_pair(fd1, fd2);
 
 	fd_install(fd1, newfile1);
-- 
2.55.0

Re: [PATCH net 1/1] net: preserve socketpair output on setup failure

[netdev-bot+sashiko]

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

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

- [Medium] The new publication step in `__sys_socketpair()`
  (net/socket.c:1908-1916) still uses two independent `put_user()`…
- [Low] Undisclosed partial revert of commit 016a266bdfeda2
  ("socketpair(): allocate descriptors first") with no Fixes: tag and no…

--

net: preserve socketpair output on setup failure

This moves the two put_user() stores of the reserved descriptor numbers in
__sys_socketpair() from before socket creation to after both struct file
objects have been allocated, so a setup failure no longer leaves the
caller's array modified.  On a store failure both file references are
dropped before the reserved descriptor slots are released.

> diff --git a/net/socket.c b/net/socket.c
> index c05d86e63abf7..4cb8662869f09 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1851,10 +1851,7 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec)
>  	if (SOCK_NONBLOCK != O_NONBLOCK && (flags & SOCK_NONBLOCK))
>  		flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
>  
> -	/*
> -	 * reserve descriptors and make sure we won't fail
> -	 * to return them to userland.
> -	 */
> +	/* Reserve both descriptors before creating the sockets. */

[Severity: Low]
Should this change carry a Fixes: tag?

git blame on net/socket.c shows the early put_user() pair and the comment
being removed here both came from commit 016a266bdfeda2 ("socketpair():
allocate descriptors first"), which deliberately published the descriptor
numbers before socket and file creation so the store to userland could not
fail late, and used that to simplify the failure exits.

Moving the stores back after sock_alloc_file() and re-adding the
two-object failure exit reverts that part of 016a266bdfeda2, and the
replacement comment drops the recorded invariant.  Could the changelog
mention 016a266bdfeda2 as the origin of the ordering being changed, so
that backporters can see what is being partially reverted?

>  	fd1 = get_unused_fd_flags(flags);
>  	if (unlikely(fd1 < 0))
>  		return fd1;
> @@ -1865,14 +1862,6 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec)
>  		return fd2;
>  	}
>  
> -	err = put_user(fd1, &usockvec[0]);
> -	if (err)
> -		goto out;
> -
> -	err = put_user(fd2, &usockvec[1]);
> -	if (err)
> -		goto out;
> -
>  	/*
>  	 * Obtain the first socket and check if the underlying protocol
>  	 * supports the socketpair call.

[ ... ]

> @@ -1916,6 +1905,16 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec)
>  		goto out;
>  	}
>  
> +	/* Publish the descriptors now that it shouldn't fail. */
> +	err = put_user(fd1, &usockvec[0]);
> +	if (!err)
> +		err = put_user(fd2, &usockvec[1]);
> +	if (err) {
> +		fput(newfile2);
> +		fput(newfile1);
> +		goto out;
> +	}
> +

[Severity: Medium]
Can usockvec[0] still be left modified on an error return here?

The publication is two independent stores, so the first can commit while
the second faults, for instance when usockvec is placed with the first int
in a writable page and the second int in a PROT_NONE page, or when an
mprotect()/userfaultfd race lands between the two accesses:

	err = put_user(fd1, &usockvec[0]);	/* commits */
	if (!err)
		err = put_user(fd2, &usockvec[1]);	/* -EFAULT */

The error path then falls through to:

out:
	put_unused_fd(fd2);
	put_unused_fd(fd1);
	return err;

so -EFAULT is returned to the caller while usockvec[0] still holds fd1,
and fd1 has been released and will be handed back out by the next
open()/socket()/accept() in the process.

The commit message says the current code "leaves a modified caller's array
despite returning an error, contrary to POSIX".  Is that state still
reachable after this change for the partial-store case?

Also, the new comment "Publish the descriptors now that it shouldn't fail."
is immediately followed by the handler for that publication failing.
Could the comment be reworded, or the two stores replaced with a single
copy_to_user() of a two-element array as do_pipe2() does in fs/pipe.c?

	if (unlikely(copy_to_user(fildes, fd, sizeof(fd)))) {
		fput(files[0]);
		fput(files[1]);
		put_unused_fd(fd[0]);
		put_unused_fd(fd[1]);
		error = -EFAULT;

Note that even the do_pipe2() form can copy the first int before faulting
on the second, so a straddling buffer leaves a residual window there too.

For the record, the two-store publication is not new in this patch, the
baseline had the same pattern; the question is only whether the changelog
claim of removing this behaviour holds.

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