| Message ID | 147222403811.18925.983476973845584327.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
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?
On Fri, 26 Aug 2016 13:49:00 -0500 Eric Blake <eblake@redhat.com> wrote: > 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. > Ok. > > + > > 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. > Ok. > 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? > v9fs_rename() and v9fs_link() only take a single name argument (see the "dds" mask passed to pdu_unmarshal()). > > @@ -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? > I don't know but I guess it is simply not implemented. FWIW the only reference I could find for 9P requests that are not documented in http://man.cat-v.org/plan_9/5/ is include/net/9p/9p.h in the linux kernel tree. The official linux client has P9_TRENAME, P9_TRENAMEAT and P9_TMKDIR, but no P9_TMKDIRAT...
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dba11773699b..f4184cae805f 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1497,6 +1497,11 @@ static void v9fs_lcreate(void *opaque) goto out_nofid; } + if (!strcmp(".", name.data) || !strcmp("..", name.data)) { + err = -EEXIST; + goto out_nofid; + } + fidp = get_fid(pdu, dfid); if (fidp == NULL) { err = -ENOENT; @@ -2096,6 +2101,11 @@ static void v9fs_create(void *opaque) goto out_nofid; } + if (!strcmp(".", name.data) || !strcmp("..", name.data)) { + err = -EEXIST; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -EINVAL; @@ -2266,6 +2276,11 @@ static void v9fs_symlink(void *opaque) goto out_nofid; } + if (!strcmp(".", name.data) || !strcmp("..", name.data)) { + err = -EEXIST; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -EINVAL; @@ -2345,6 +2360,11 @@ static void v9fs_link(void *opaque) goto out_nofid; } + if (!strcmp(".", name.data) || !strcmp("..", name.data)) { + err = -EEXIST; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -ENOENT; @@ -2433,6 +2453,16 @@ static void v9fs_unlinkat(void *opaque) goto out_nofid; } + if (!strcmp(".", name.data)) { + err = -EINVAL; + goto out_nofid; + } + + if (!strcmp("..", name.data)) { + err = -ENOTEMPTY; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -EINVAL; @@ -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; + } + 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; + goto out_err; + } + v9fs_path_write_lock(s); err = v9fs_complete_renameat(pdu, olddirfid, &old_name, newdirfid, &new_name); @@ -2877,6 +2918,11 @@ static void v9fs_mknod(void *opaque) goto out_nofid; } + if (!strcmp(".", name.data) || !strcmp("..", name.data)) { + err = -EEXIST; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -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; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT;
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. 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(+)