[Qemu-devel] [PATCH] 9pfs: local: fix unlink of alien files in mapped-file mode

[Qemu-devel] [PATCH] 9pfs: local: fix unlink of alien files in mapped-file mode

[Greg Kurz]

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;
         }

Re: [Qemu-devel] [PATCH] 9pfs: local: fix unlink of alien files in mapped-file mode

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: local: fix unlink of alien files in mapped-file mode

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 3934 bytes --]

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>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]