linux-kernel - Re: [PATCH v2] fs: always return zero on success from replace

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: <20250804-rundum-anwalt-10c3b9c11f8e@brauner>
Date: Mon, 4 Aug 2025 14:33:13 +0200
From: Christian Brauner <brauner@...nel.org>
To: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>, 
	Sargun Dhillon <sargun@...gun.me>, Kees Cook <kees@...nel.org>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] fs: always return zero on success from replace_fd()

On Mon, Aug 04, 2025 at 10:40:17AM +0200, Thomas Weißschuh wrote:
> replace_fd() returns the number of the new file descriptor through the
> return value of do_dup2(). However its callers never care about the
> specific number. In fact the caller in receive_fd_replace() treats any

To be pedantic as stated this isn't true: While they don't care about it
in their error handling they very much care about what specific file
descriptor number is used. Try and assign unexpected fd numbers for
coredumping and see what CVEs you can get out of it...

I'll take the patch but I'll amend it to just use a guard:

diff --git a/fs/file.c b/fs/file.c
index 6d2275c3be9c..858a55dbd5d8 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1318,7 +1318,7 @@ __releases(&files->file_lock)
 int replace_fd(unsigned fd, struct file *file, unsigned flags)
 {
        int err;
-       struct files_struct *files = current->files;
+       struct files_struct *files;

        if (!file)
                return close_fd(fd);
@@ -1326,15 +1326,17 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
        if (fd >= rlimit(RLIMIT_NOFILE))
                return -EBADF;

-       spin_lock(&files->file_lock);
+       files = current->files;
+
+       guard(spinlock)(&files->file_lock);
        err = expand_files(files, fd);
        if (unlikely(err < 0))
-               goto out_unlock;
-       return do_dup2(files, file, fd, flags);
+               return err;
+       err = do_dup2(files, file, fd, flags);
+       if (err < 0)
+               return err;

-out_unlock:
-       spin_unlock(&files->file_lock);
-       return err;
+       return 0;
 }

 /**

scoped_guard() works too but bloats the patch.

> non-zero return value as an error and therefore never calls
> __receive_sock() for most file descriptors, which is a bug.
> 
> To fix the bug in receive_fd_replace() and to avoid the same issue
> happening in future callers, signal success through a plain zero.
> 
> Suggested-by: Al Viro <viro@...iv.linux.org.uk>
> Link: https://lore.kernel.org/lkml/20250801220215.GS222315@ZenIV/
> Fixes: 173817151b15 ("fs: Expand __receive_fd() to accept existing fd")
> Fixes: 42eb0d54c08a ("fs: split receive_fd_replace from __receive_fd")
> Cc: stable@...r.kernel.org
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> ---
> Changes in v2:
> - Move the fix to replace_fd() (Al)
> - Link to v1: https://lore.kernel.org/r/20250801-fix-receive_fd_replace-v1-1-d46d600c74d6@linutronix.de
> ---
> Untested, it stuck out while reading the code.
> ---
>  fs/file.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 6d2275c3be9c6967d16c75d1b6521f9b58980926..f8a271265913951d755a5db559938d589219c4f2 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1330,7 +1330,10 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>  	err = expand_files(files, fd);
>  	if (unlikely(err < 0))
>  		goto out_unlock;
> -	return do_dup2(files, file, fd, flags);
> +	err = do_dup2(files, file, fd, flags);
> +	if (err < 0)
> +		goto out_unlock;
> +	err = 0;
>  
>  out_unlock:
>  	spin_unlock(&files->file_lock);
> 
> ---
> base-commit: d2eedaa3909be9102d648a4a0a50ccf64f96c54f
> change-id: 20250801-fix-receive_fd_replace-7fdd5ce6532d
> 
> Best regards,
> -- 
> Thomas Weißschuh <thomas.weissschuh@...utronix.de>
>