Re: [Qemu-devel] [PATCH v2 3/5] 9p: forbid . and .. in file names

[Eric Blake <eblake@redhat.com>]

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

On 08/26/2016 10:07 AM, Greg Kurz wrote:
> According to the 9P spec http://man.cat-v.org/plan_9/5/open about the
> create request:
> 
> The names . and .. are special; it is illegal to create files with these
> names.
> 
> This patch causes the create and lcreate requests to fail with EINVAL if
> the file name is either "." or "..".
> 
> Even if it isn't explicitly written in the spec, this patch extends the
> checking to all requests that may cause a filename to be created:
> 
>     - mknod
>     - rename
>     - renameat
>     - mkdir
>     - link
>     - symlink
> 
> The unlinkat request also gets patched for consistency (even if
> rmdir("foo/..") is expected to fail according to POSIX.1-2001).
> 
> The various error values come from the linux manual pages.

Linux doesn't always obey the POSIX rules for which errno to use, but I
think your choices here are mostly okay.

> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> @@ -2545,6 +2575,11 @@ static void v9fs_rename(void *opaque)
>          goto out_nofid;
>      }
>  
> +    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
> +        err = -EBUSY;
> +        goto out_nofid;
> +    }

POSIX suggests that EISDIR is better than EBUSY here.

> +
>      fidp = get_fid(pdu, fid);
>      if (fidp == NULL) {
>          err = -ENOENT;
> @@ -2662,6 +2697,12 @@ static void v9fs_renameat(void *opaque)
>          goto out_err;
>      }
>  
> +    if (!strcmp(".", old_name.data) || !strcmp("..", old_name.data) ||
> +        !strcmp(".", new_name.data) || !strcmp("..", new_name.data)) {
> +        err = -EBUSY;

Ditto.

Wait. Why is v9fs_rename() only checking one name, but v9fs_renameat()
checking both old_name and new_name?  Also, should link be checking both
the source and destination name?

> @@ -3033,6 +3079,11 @@ static void v9fs_mkdir(void *opaque)
>          goto out_nofid;
>      }
>  
> +    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
> +        err = -EEXIST;
> +        goto out_nofid;
> +    }
> +

Unrelated to this patch, but why do we have v9fs_renameat but not
v9fs_mkdirat?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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