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

[Eric Blake <eblake@redhat.com>]

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