linux-kernel - Re: fs: Expand __fd_install

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] [day] [month] [year] [list]

Message-ID: <202006220851.7B817663C7@keescook>
Date:   Mon, 22 Jun 2020 08:52:12 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Colin Ian King <colin.king@...onical.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Dmitry Kadashev <dkadashev@...il.com>,
        Jens Axboe <axboe@...nel.dk>, Arnd Bergmann <arnd@...db.de>,
        Sargun Dhillon <sargun@...gun.me>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: fs: Expand __fd_install_received() to accept fd

On Mon, Jun 22, 2020 at 04:11:30PM +0100, Colin Ian King wrote:
> Hi,
> 
> static analysis with Coverity has detected a potential issue with the
> following commit:
> 
> commit 8336af9ab8c5d64a309cbf72648054af61548899
> Author: Kees Cook <keescook@...omium.org>
> Date:   Wed Jun 10 08:46:58 2020 -0700
> 
>     fs: Expand __fd_install_received() to accept fd
> 
> Calling __fd_install_received() with fd >= 0 and ufd being non-null will
> cause a put_user of an uninitialized new_fd hence potentially leaking
> data on the stack back to the user.
> 
> static analysis is as follows:
> 
> 1050 int __fd_install_received(int fd, struct file *file, int __user *ufd,
> 1051                          unsigned int o_flags)
> 1052 {
> 1053        struct socket *sock;
> 
>     1. var_decl: Declaring variable new_fd without initializer.
> 
> 1054        int new_fd;
> 1055        int error;
> 1056
> 1057        error = security_file_receive(file);
> 
>     2. Condition error, taking false branch.
> 
> 1058        if (error)
> 1059                return error;
> 1060
> 
>     3. Condition fd < 0, taking false branch.
> 
> 1061        if (fd < 0) {
> 1062                new_fd = get_unused_fd_flags(o_flags);
> 1063                if (new_fd < 0)
> 1064                        return new_fd;
> 1065        }
> 1066
> 
>     4. Condition ufd, taking true branch.
> 1067        if (ufd) {
> 
> CID: Uninitialized scalar variable (UNINIT)5. uninit_use: Using
> uninitialized value new_fd.
> 
> 1068                error = put_user(new_fd, ufd);
> 
> Colin

Eek. Thank you. Fixed with:


diff --git a/fs/file.c b/fs/file.c
index 9568bcfd1f44..c0bcf4c4fb12 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -963,12 +963,14 @@ int __fd_install_received(int fd, struct file *file, int __user *ufd,
 		new_fd = get_unused_fd_flags(o_flags);
 		if (new_fd < 0)
 			return new_fd;
-	}
+	} else
+		new_fd = fd;
 
 	if (ufd) {
 		error = put_user(new_fd, ufd);
 		if (error) {
-			put_unused_fd(new_fd);
+			if (fd < 0)
+				put_unused_fd(new_fd);
 			return error;
 		}
 	}
@@ -976,7 +978,6 @@ int __fd_install_received(int fd, struct file *file, int __user *ufd,
 	if (fd < 0)
 		fd_install(new_fd, get_file(file));
 	else {
-		new_fd = fd;
 		error = replace_fd(new_fd, file, o_flags);
 		if (error)
 			return error;

-- 
Kees Cook