* re: fs: Expand __fd_install_received() to accept fd
@ 2020-06-22 15:11 Colin Ian King
2020-06-22 15:52 ` Kees Cook
0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2020-06-22 15:11 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Viro, Christian Brauner, Dmitry Kadashev, Jens Axboe,
Arnd Bergmann, Sargun Dhillon, linux-fsdevel, linux-kernel
Hi,
static analysis with Coverity has detected a potential issue with the
following commit:
commit 8336af9ab8c5d64a309cbf72648054af61548899
Author: Kees Cook <keescook@chromium.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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: fs: Expand __fd_install_received() to accept fd
2020-06-22 15:11 fs: Expand __fd_install_received() to accept fd Colin Ian King
@ 2020-06-22 15:52 ` Kees Cook
0 siblings, 0 replies; 2+ messages in thread
From: Kees Cook @ 2020-06-22 15:52 UTC (permalink / raw)
To: Colin Ian King
Cc: Alexander Viro, Christian Brauner, Dmitry Kadashev, Jens Axboe,
Arnd Bergmann, Sargun Dhillon, linux-fsdevel, linux-kernel
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@chromium.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
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-06-22 15:52 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-22 15:11 fs: Expand __fd_install_received() to accept fd Colin Ian King
2020-06-22 15:52 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).