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

[Greg Kurz]

On Fri, 28 Apr 2017 09:45:23 -0500
Eric Blake <address@hidden> 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 <address@hidden>
> > ---
> >  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 <address@hidden>
>

pgpZWMC8ZQlsb.pgp
Description: OpenPGP digital signature