From b7361d46e75f12d8d943ca8d33ef82cafce39920 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 6 Mar 2017 17:34:01 +0100 Subject: 9pfs: fix bogus fd check in local_remove() This was spotted by Coverity as a fd leak. This is certainly true, but also local_remove() would always return without doing anything, unless the fd is zero, which is very unlikely. (Coverity issue CID1371732) Signed-off-by: Greg Kurz Reviewed-by: Eric Blake --- hw/9pfs/9p-local.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index f22a3c3..5db7104 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1008,7 +1008,7 @@ static int local_remove(FsContext *ctx, const char *path) int err = -1; dirfd = local_opendir_nofollow(ctx, dirpath); - if (dirfd) { + if (dirfd == -1) { goto out; } -- cgit v1.1 From faab207f115cf9738f110cb088ab35a4b7aef73a Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 6 Mar 2017 17:34:01 +0100 Subject: 9pfs: fix fd leak in local_opendir() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity issue CID1371731 Signed-off-by: Greg Kurz Reviewed-by: Daniel P. Berrange Reviewed-by: Philippe Mathieu-Daudé --- hw/9pfs/9p-local.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 5db7104..09f6a46 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -435,6 +435,7 @@ static int local_opendir(FsContext *ctx, stream = fdopendir(dirfd); if (!stream) { + close(dirfd); return -1; } fs->dir.stream = stream; -- cgit v1.1 From 23da0145cc4be66fdb1033f951dbbf140f457896 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 6 Mar 2017 17:34:01 +0100 Subject: 9pfs: fail local_statfs() earlier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we cannot open the given path, we can return right away instead of passing -1 to fstatfs() and close(). This will make Coverity happy. (Coverity issue CID1371729) Signed-off-by: Greg Kurz Reviewed-by: Daniel P. berrange Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- hw/9pfs/9p-local.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 09f6a46..6d16c4a 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1053,6 +1053,9 @@ static int local_statfs(FsContext *s, V9fsPath *fs_path, struct statfs *stbuf) int fd, ret; fd = local_open_nofollow(s, fs_path->data, O_RDONLY, 0); + if (fd == -1) { + return -1; + } ret = fstatfs(fd, stbuf); close_preserve_errno(fd); return ret; -- cgit v1.1 From b314f6a077a1dbc0463a5dc41162f64950048e72 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 6 Mar 2017 17:34:01 +0100 Subject: 9pfs: don't use AT_EMPTY_PATH in local_set_cred_passthrough() The name argument can never be an empty string, and dirfd always point to the containing directory of the file name. AT_EMPTY_PATH is hence useless here. Also it breaks build with glibc version 2.13 and older. It is actually an oversight of a previous tentative patch to implement this function. We can safely drop it. Reported-by: Mark Cave-Ayland Signed-off-by: Greg Kurz Tested-by: Mark Cave-Ayland Reviewed-by: Eric Blake --- hw/9pfs/9p-local.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 6d16c4a..0ca4c94 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -349,7 +349,7 @@ static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd, const char *name, FsCred *credp) { if (fchownat(dirfd, name, credp->fc_uid, credp->fc_gid, - AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH) < 0) { + AT_SYMLINK_NOFOLLOW) < 0) { /* * If we fail to change ownership and if we are * using security model none. Ignore the error -- cgit v1.1 From 918112c02aff2bac4cb72dc2feba0cb05305813e Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 6 Mar 2017 17:34:01 +0100 Subject: 9pfs: fix O_PATH build break with older glibc versions When O_PATH is used with O_DIRECTORY, it only acts as an optimization: the openat() syscall simply finds the name in the VFS, and doesn't trigger the underlying filesystem. On systems that don't define O_PATH, because they have glibc version 2.13 or older for example, we can safely omit it. We don't want to deactivate O_PATH globally though, in case it is used without O_DIRECTORY. The is done with a dedicated macro. Systems without O_PATH may thus fail to resolve names that involve unreadable directories, compared to newer systems succeeding, but such corner case failure is our only option on those older systems to avoid the security hole of chasing symlinks inappropriately. Signed-off-by: Greg Kurz Reviewed-by: Eric Blake (added last paragraph to changelog as suggested by Eric Blake) Signed-off-by: Greg Kurz --- hw/9pfs/9p-util.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index 091f3ce..cb7b207 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -22,7 +22,12 @@ static inline void close_preserve_errno(int fd) static inline int openat_dir(int dirfd, const char *name) { - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH); +#ifdef O_PATH +#define OPENAT_DIR_O_PATH O_PATH +#else +#define OPENAT_DIR_O_PATH 0 +#endif + return openat(dirfd, name, O_DIRECTORY | O_RDONLY | OPENAT_DIR_O_PATH); } static inline int openat_file(int dirfd, const char *name, int flags, -- cgit v1.1 From b003fc0d8aa5e7060dbf7e5862b8013c73857c7f Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 6 Mar 2017 17:34:01 +0100 Subject: 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common() We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make QEMU vulnerable. While here, we also fix local_unlinkat_common() to use openat_dir() for the same reasons (it was a leftover in the original patchset actually). This fixes CVE-2016-9602. Signed-off-by: Greg Kurz Reviewed-by: Daniel P. Berrange Reviewed-by: Eric Blake --- hw/9pfs/9p-local.c | 2 +- hw/9pfs/9p-util.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 0ca4c94..45e9a1f 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -960,7 +960,7 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, if (flags == AT_REMOVEDIR) { int fd; - fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH); + fd = openat_dir(dirfd, name); if (fd == -1) { goto err_out; } diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index cb7b207..517027c 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -27,7 +27,8 @@ static inline int openat_dir(int dirfd, const char *name) #else #define OPENAT_DIR_O_PATH 0 #endif - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | OPENAT_DIR_O_PATH); + return openat(dirfd, name, + O_DIRECTORY | O_RDONLY | O_NOFOLLOW | OPENAT_DIR_O_PATH); } static inline int openat_file(int dirfd, const char *name, int flags, -- cgit v1.1