When trying to remove a file from a directory, both created in non-mapped
mode, the file remains and EBADF is returned to the guest.
This is a regression introduced by commit "df4938a6651b 9pfs: local:
unlinkat: don't follow symlinks" when fixing CVE-2016-9602. It changed the
way we unlink the metadata file from
ret = remove("$dir/.virtfs_metadata/$name");
if (ret < 0 && errno != ENOENT) {
/* Error out */
}
/* Ignore absence of metadata */
to
fd = openat("$dir/.virtfs_metadata")
unlinkat(fd, "$name")
if (ret < 0 && errno != ENOENT) {
/* Error out */
}
/* Ignore absence of metadata */
If $dir was created in non-mapped mode, openat() fails with ENOENT and
we pass -1 to unlinkat(), which fails in turn with EBADF.
We just need to check the return of openat() and ignore ENOENT, in order
to restore the behaviour we had with remove().
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p-local.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index f3ebca4f7a56..4e9823b08e74 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -983,12 +983,20 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
* .virtfs_metadata directory.
*/
map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR);
- ret = unlinkat(map_dirfd, name, 0);
- close_preserve_errno(map_dirfd);
- if (ret < 0 && errno != ENOENT) {
+ if (map_dirfd != -1) {
+ ret = unlinkat(map_dirfd, name, 0);
+ close_preserve_errno(map_dirfd);
+ if (ret < 0 && errno != ENOENT) {
+ /*
+ * We didn't had the .virtfs_metadata file. May be file created
+ * in non-mapped mode ?. Ignore ENOENT.
+ */
+ goto err_out;
+ }
+ } else if (errno != ENOENT) {
/*
- * We didn't had the .virtfs_metadata file. May be file created
- * in non-mapped mode ?. Ignore ENOENT.
+ * We didn't had the parent .virtfs_metadata directory. May be
+ * file created in non-mapped mode ?. Ignore ENOENT.
*/
goto err_out;
}
On 04/28/2017 03:54 AM, Greg Kurz wrote: > When trying to remove a file from a directory, both created in non-mapped > mode, the file remains and EBADF is returned to the guest. > > This is a regression introduced by commit "df4938a6651b 9pfs: local: > unlinkat: don't follow symlinks" when fixing CVE-2016-9602. It changed the > way we unlink the metadata file from > > ret = remove("$dir/.virtfs_metadata/$name"); > if (ret < 0 && errno != ENOENT) { > /* Error out */ > } > /* Ignore absence of metadata */ > > to > > fd = openat("$dir/.virtfs_metadata") > unlinkat(fd, "$name") Yep, failure to check for errors. Is this something Coverity can flag? > > We just need to check the return of openat() and ignore ENOENT, in order > to restore the behaviour we had with remove(). > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-local.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index f3ebca4f7a56..4e9823b08e74 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -983,12 +983,20 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, > * .virtfs_metadata directory. > */ > map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); > - ret = unlinkat(map_dirfd, name, 0); > - close_preserve_errno(map_dirfd); > - if (ret < 0 && errno != ENOENT) { > + if (map_dirfd != -1) { > + ret = unlinkat(map_dirfd, name, 0); > + close_preserve_errno(map_dirfd); > + if (ret < 0 && errno != ENOENT) { > + /* > + * We didn't had the .virtfs_metadata file. May be file created > + * in non-mapped mode ?. Ignore ENOENT. This is code motion (in fact, the second time you've moved this comment), but you might as well fix the poor grammar while you are here: We didn't have the .virtfs_metadata file. Maybe the file was created in non-mapped mode? Ignore ENOENT. > + */ > + goto err_out; > + } > + } else if (errno != ENOENT) { > /* > - * We didn't had the .virtfs_metadata file. May be file created > - * in non-mapped mode ?. Ignore ENOENT. > + * We didn't had the parent .virtfs_metadata directory. May be > + * file created in non-mapped mode ?. Ignore ENOENT. And certainly fix the grammar of your new comment (no need to copy-and-paste the poor wording): We didn't have the parent .virtfs_metadata directory. Maybe the file was created in non-mapped mode? Ignore ENOENT. But the code fix is correct, and a comment wording change is minor enough that you can add my: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Fri, 28 Apr 2017 09:45:23 -0500 Eric Blake <eblake@redhat.com> wrote: > On 04/28/2017 03:54 AM, Greg Kurz wrote: > > When trying to remove a file from a directory, both created in non-mapped > > mode, the file remains and EBADF is returned to the guest. > > > > This is a regression introduced by commit "df4938a6651b 9pfs: local: > > unlinkat: don't follow symlinks" when fixing CVE-2016-9602. It changed the > > way we unlink the metadata file from > > > > ret = remove("$dir/.virtfs_metadata/$name"); > > if (ret < 0 && errno != ENOENT) { > > /* Error out */ > > } > > /* Ignore absence of metadata */ > > > > to > > > > fd = openat("$dir/.virtfs_metadata") > > unlinkat(fd, "$name") > > Yep, failure to check for errors. Is this something Coverity can flag? > I guess it should if it doesn't yet. > > > > We just need to check the return of openat() and ignore ENOENT, in order > > to restore the behaviour we had with remove(). > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p-local.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index f3ebca4f7a56..4e9823b08e74 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -983,12 +983,20 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, > > * .virtfs_metadata directory. > > */ > > map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); > > - ret = unlinkat(map_dirfd, name, 0); > > - close_preserve_errno(map_dirfd); > > - if (ret < 0 && errno != ENOENT) { > > + if (map_dirfd != -1) { > > + ret = unlinkat(map_dirfd, name, 0); > > + close_preserve_errno(map_dirfd); > > + if (ret < 0 && errno != ENOENT) { > > + /* > > + * We didn't had the .virtfs_metadata file. May be file created > > + * in non-mapped mode ?. Ignore ENOENT. > > This is code motion (in fact, the second time you've moved this > comment), but you might as well fix the poor grammar while you are here: > > We didn't have the .virtfs_metadata file. Maybe the file was created in > non-mapped mode? Ignore ENOENT. > > > + */ > > + goto err_out; > > + } > > + } else if (errno != ENOENT) { > > /* > > - * We didn't had the .virtfs_metadata file. May be file created > > - * in non-mapped mode ?. Ignore ENOENT. > > + * We didn't had the parent .virtfs_metadata directory. May be > > + * file created in non-mapped mode ?. Ignore ENOENT. > > And certainly fix the grammar of your new comment (no need to > copy-and-paste the poor wording): > > We didn't have the parent .virtfs_metadata directory. Maybe the file was > created in non-mapped mode? Ignore ENOENT. > FYI, I've dropped the existing comments and added this instead: @@ -957,6 +957,14 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, if (ctx->export_flags & V9FS_SM_MAPPED_FILE) { int map_dirfd; + /* We need to remove the metadata as well: + * - the metadata directory if we're removing a directory + * - the metadata file in the parent's metadata directory + * + * If any of these are missing (ie, ENOENT) then we're probably + * trying to remove something that wasn't created in mapped-file + * mode. We just ignore the error. + */ if (flags == AT_REMOVEDIR) { int fd; I'll push it to my 9p-next tree. > But the code fix is correct, and a comment wording change is minor > enough that you can add my: > > Reviewed-by: Eric Blake <eblake@redhat.com> >
© 2016 - 2024 Red Hat, Inc.