[Qemu-devel] [PATCH v2 0/5] 9P security fixes

[Qemu-devel] [PATCH v2 0/5] 9P security fixes

[Greg Kurz]

As reported by Felix Wilhelm, at various places in 9pfs, full paths are
created by concatenating a guest originated string to the export path. A
malicious guest could forge a relative path and access files outside the
export path.

A tentative fix was sent recently by Prasad J Pandit, but it was only
focused on the local backend and did not get a positive review. This series
tries to address the issue more globally, based on the official 9P spec.

This v2 is actually a complete rework of my previous post, based on the
feedback I had from Peter Maydell:

http://patchwork.ozlabs.org/patch/662324/

Note that xattrwalk and xattrcreate are no longer part of the pictures
since they are being passed attribute names, not file names actually.

The official linux client only sends 1 path component at a time, without
any / nor NULs nor "." nor ".." and we don't have a functional qtest yet
for 9P, so I had to do limited manual testing using GDB to hack strings
coming from a linux client. It is no longer possible to access files
outside the export path if this series is applied.

I could also run the POSIX file system test suite from TUXERA:

http://www.tuxera.com/community/open-source-posix/

and did not observe any regression with this patch (at least with
the local backend and the none/passthrough/mapped security models).

Patch 5/5 isn't related to the initial path traversal issue but I found it
while working on empty strings, so I send it along anyway.

---

Greg Kurz (5):
      9p: forbid illegal path names
      9p: disallow the NUL character in all strings
      9p: forbid . and .. in file names
      9p: handle walk of ".." in the root directory
      9p: forbid empty extension string


 fsdev/9p-iov-marshal.c |    7 ++
 hw/9pfs/9p.c           |  168 +++++++++++++++++++++++++++++++++++++++++++++---
 hw/9pfs/9p.h           |    1 
 3 files changed, 164 insertions(+), 12 deletions(-)

--
Greg

[Qemu-devel] [PATCH v2 1/5] 9p: forbid illegal path names

[Greg Kurz]

Empty path components don't make sense and may cause undefined behavior,
depending on the backend.

Also, the walk request described in the 9P spec [1] clearly shows that
the client is supposed to send individual path components: the official
linux client never sends portions of path containing the / character for
example.

Moreover, the 9P spec [2] also states that a system can decide to restrict
the set of supported characters used in path components, with an explicit
mention "to remove slashes from name components".

This patch introduces a new name_is_illegal() helper that checks the
names sent by the client are not empty and don't contain unwanted chars.
Since 9pfs is only supported on linux hosts, only the / character is
checked at the moment. When support for other hosts (AKA. win32) is added,
other chars may need to be blacklisted as well.

If a client sends an illegal path component, the request will fail and
EINVAL is returned to the client.

[1] http://man.cat-v.org/plan_9/5/walk
[2] http://man.cat-v.org/plan_9/5/intro

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b6b02b46a9da..dba11773699b 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids)
     return offset;
 }
 
+static bool name_is_illegal(const char *name)
+{
+    return name == NULL || strchr(name, '/') != NULL;
+}
+
 static void v9fs_walk(void *opaque)
 {
     int name_idx;
@@ -1289,6 +1294,10 @@ static void v9fs_walk(void *opaque)
             if (err < 0) {
                 goto out_nofid;
             }
+            if (name_is_illegal(wnames[i].data)) {
+                err = -EINVAL;
+                goto out_nofid;
+            }
             offset += err;
         }
     } else if (nwnames > P9_MAXWELEM) {
@@ -1483,6 +1492,11 @@ static void v9fs_lcreate(void *opaque)
     }
     trace_v9fs_lcreate(pdu->tag, pdu->id, dfid, flags, mode, gid);
 
+    if (name_is_illegal(name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, dfid);
     if (fidp == NULL) {
         err = -ENOENT;
@@ -2077,6 +2091,11 @@ static void v9fs_create(void *opaque)
     }
     trace_v9fs_create(pdu->tag, pdu->id, fid, name.data, perm, mode);
 
+    if (name_is_illegal(name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -EINVAL;
@@ -2242,6 +2261,11 @@ static void v9fs_symlink(void *opaque)
     }
     trace_v9fs_symlink(pdu->tag, pdu->id, dfid, name.data, symname.data, gid);
 
+    if (name_is_illegal(name.data) || name_is_illegal(symname.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
@@ -2316,6 +2340,11 @@ static void v9fs_link(void *opaque)
     }
     trace_v9fs_link(pdu->tag, pdu->id, dfid, oldfid, name.data);
 
+    if (name_is_illegal(name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -ENOENT;
@@ -2398,6 +2427,12 @@ static void v9fs_unlinkat(void *opaque)
     if (err < 0) {
         goto out_nofid;
     }
+
+    if (name_is_illegal(name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
@@ -2504,6 +2539,12 @@ static void v9fs_rename(void *opaque)
     if (err < 0) {
         goto out_nofid;
     }
+
+    if (name_is_illegal(name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -ENOENT;
@@ -2616,6 +2657,11 @@ static void v9fs_renameat(void *opaque)
         goto out_err;
     }
 
+    if (name_is_illegal(old_name.data) || name_is_illegal(new_name.data)) {
+        err = -EINVAL;
+        goto out_err;
+    }
+
     v9fs_path_write_lock(s);
     err = v9fs_complete_renameat(pdu, olddirfid,
                                  &old_name, newdirfid, &new_name);
@@ -2826,6 +2872,11 @@ static void v9fs_mknod(void *opaque)
     }
     trace_v9fs_mknod(pdu->tag, pdu->id, fid, mode, major, minor);
 
+    if (name_is_illegal(name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -ENOENT;
@@ -2977,6 +3028,11 @@ static void v9fs_mkdir(void *opaque)
     }
     trace_v9fs_mkdir(pdu->tag, pdu->id, fid, name.data, mode, gid);
 
+    if (name_is_illegal(name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -ENOENT;

[Qemu-devel] [PATCH v2 2/5] 9p: disallow the NUL character in all strings

[Greg Kurz]

According to the 9P spec at http://man.cat-v.org/plan_9/5/intro :

Data items of larger or variable lengths are represented by a
two-byte field specifying a count, n, followed by n bytes of
data.  Text strings are represented this way, with the text
itself stored as a UTF-8 encoded sequence of Unicode charac-
ters (see utf(6)). Text strings in 9P messages are not NUL-
terminated: n counts the bytes of UTF-8 data, which include
no final zero byte.  The NUL character is illegal in all
text strings in 9P, and is therefore excluded from file
names, user names, and so on.

With this patch, if a 9P client sends a text string containing a NUL
character, the request will fail and the client is returned EINVAL.

The checking is done in v9fs_iov_vunmarshal() because it is a convenient
place to check all client originated strings.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/9p-iov-marshal.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index 663cad542900..9bcdc370231d 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -127,7 +127,12 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
                                      str->size);
                 if (copied > 0) {
                     str->data[str->size] = 0;
-                } else {
+                    /* 9P forbids NUL characters in all text strings */
+                    if (strlen(str->data) != str->size) {
+                        copied = -EINVAL;
+                    }
+                }
+                if (copied <= 0) {
                     v9fs_string_free(str);
                 }
             }

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

[Greg Kurz]

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(+)

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;

[Qemu-devel] [PATCH v2 4/5] 9p: handle walk of ".." in the root directory

[Greg Kurz]

The 9P spec at http://man.cat-v.org/plan_9/5/intro says:

All directories must support walks to the directory .. (dot-dot) meaning
parent directory, although by convention directories contain no explicit
entry for .. or . (dot).  The parent of the root directory of a server's
tree is itself.

This means that a client cannot walk further than the root directory
exported by the server. In other words, if the client wants to walk
"/.." or "/foo/../..", the server shoud answer like the request was
to walk "/".

This patch just does that:
- we cache the QID of the root directory at attach time
- during the walk we compare the QID of each path component with the root
  QID to detect if we're in a "/.." situation
- if so, we skip the current component and go to the next one

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c |   40 +++++++++++++++++++++++++++++++---------
 hw/9pfs/9p.h |    1 +
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index f4184cae805f..7b1dfe4e47cb 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1010,6 +1010,7 @@ static void v9fs_attach(void *opaque)
         goto out;
     }
     err += offset;
+    memcpy(&s->root_qid, &qid, sizeof(qid));
     trace_v9fs_attach_return(pdu->tag, pdu->id,
                              qid.type, qid.version, qid.path);
     /*
@@ -1261,6 +1262,14 @@ static bool name_is_illegal(const char *name)
     return name == NULL || strchr(name, '/') != NULL;
 }
 
+static bool not_same_qid(const V9fsQID *qid1, const V9fsQID *qid2)
+{
+    return
+        qid1->type != qid2->type ||
+        qid1->version != qid2->version ||
+        qid1->path != qid2->path;
+}
+
 static void v9fs_walk(void *opaque)
 {
     int name_idx;
@@ -1276,6 +1285,7 @@ static void v9fs_walk(void *opaque)
     V9fsFidState *newfidp = NULL;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
+    V9fsQID qid;
 
     err = pdu_unmarshal(pdu, offset, "ddw", &fid, &newfid, &nwnames);
     if (err < 0) {
@@ -1309,6 +1319,12 @@ static void v9fs_walk(void *opaque)
         err = -ENOENT;
         goto out_nofid;
     }
+
+    err = fid_to_qid(pdu, fidp, &qid);
+    if (err < 0) {
+        goto out;
+    }
+
     v9fs_path_init(&dpath);
     v9fs_path_init(&path);
     /*
@@ -1318,16 +1334,22 @@ static void v9fs_walk(void *opaque)
     v9fs_path_copy(&dpath, &fidp->path);
     v9fs_path_copy(&path, &fidp->path);
     for (name_idx = 0; name_idx < nwnames; name_idx++) {
-        err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data, &path);
-        if (err < 0) {
-            goto out;
-        }
-        err = v9fs_co_lstat(pdu, &path, &stbuf);
-        if (err < 0) {
-            goto out;
+        if (not_same_qid(&pdu->s->root_qid, &qid) ||
+            strcmp("..", wnames[name_idx].data)) {
+            err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data,
+                                       &path);
+            if (err < 0) {
+                goto out;
+            }
+
+            err = v9fs_co_lstat(pdu, &path, &stbuf);
+            if (err < 0) {
+                goto out;
+            }
+            stat_to_qid(&stbuf, &qid);
+            v9fs_path_copy(&dpath, &path);
         }
-        stat_to_qid(&stbuf, &qids[name_idx]);
-        v9fs_path_copy(&dpath, &path);
+        memcpy(&qids[name_idx], &qid, sizeof(qid));
     }
     if (fid == newfid) {
         BUG_ON(fidp->fid_type != P9_FID_NONE);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index b4f757ab5449..a38603398ef5 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -236,6 +236,7 @@ typedef struct V9fsState
     int32_t root_fid;
     Error *migration_blocker;
     V9fsConf fsconf;
+    V9fsQID root_qid;
 } V9fsState;
 
 /* 9p2000.L open flags */

[Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Greg Kurz]

A buggy guest using the 9p2000.u protocol can issue a create request and
pass an empty string as the extension argument. This causes QEMU to crash
in the case of a hard link or a special file, and leads to undefined
behavior, depending on the backend, in the case of a symbolic link.

This patch causes the request to fail with EINVAL in these scenarios.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7b1dfe4e47cb..dc65c3125006 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque)
         }
         fidp->fid_type = P9_FID_DIR;
     } else if (perm & P9_STAT_MODE_SYMLINK) {
+        if (extension.data == NULL) {
+            err = -EINVAL;
+            goto out;
+        }
+
         err = v9fs_co_symlink(pdu, fidp, &name,
                               extension.data, -1 , &stbuf);
         if (err < 0) {
@@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque)
         }
         v9fs_path_copy(&fidp->path, &path);
     } else if (perm & P9_STAT_MODE_LINK) {
-        int32_t ofid = atoi(extension.data);
-        V9fsFidState *ofidp = get_fid(pdu, ofid);
+        V9fsFidState *ofidp;
+
+        if (extension.data == NULL) {
+            err = -EINVAL;
+            goto out;
+        }
+
+        ofidp = get_fid(pdu, atoi(extension.data));
+
         if (ofidp == NULL) {
             err = -EINVAL;
             goto out;
@@ -2188,6 +2200,11 @@ static void v9fs_create(void *opaque)
         uint32_t major, minor;
         mode_t nmode = 0;
 
+        if (extension.data == NULL) {
+            err = -EINVAL;
+            goto out;
+        }
+
         if (sscanf(extension.data, "%c %u %u", &ctype, &major, &minor) != 3) {
             err = -errno;
             goto out;

Re: [Qemu-devel] [PATCH v2 1/5] 9p: forbid illegal path names

[Eric Blake]

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

On 08/26/2016 10:07 AM, Greg Kurz wrote:
> Empty path components don't make sense and may cause undefined behavior,
> depending on the backend.
> 
> Also, the walk request described in the 9P spec [1] clearly shows that
> the client is supposed to send individual path components: the official
> linux client never sends portions of path containing the / character for
> example.
> 
> Moreover, the 9P spec [2] also states that a system can decide to restrict
> the set of supported characters used in path components, with an explicit
> mention "to remove slashes from name components".
> 
> This patch introduces a new name_is_illegal() helper that checks the
> names sent by the client are not empty and don't contain unwanted chars.
> Since 9pfs is only supported on linux hosts, only the / character is
> checked at the moment. When support for other hosts (AKA. win32) is added,
> other chars may need to be blacklisted as well.
> 
> If a client sends an illegal path component, the request will fail and
> EINVAL is returned to the client.
> 
> [1] http://man.cat-v.org/plan_9/5/walk
> [2] http://man.cat-v.org/plan_9/5/intro
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b6b02b46a9da..dba11773699b 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids)
>      return offset;
>  }
>  
> +static bool name_is_illegal(const char *name)
> +{
> +    return name == NULL || strchr(name, '/') != NULL;

Is anyone actually passing NULL?  And the commit message says you are
blacklisting the empty string "", but that is not what you did here.
Depending on whether callers can even pass NULL, you may be able to just
s/name == NULL/!*name/

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

Re: [Qemu-devel] [PATCH v2 2/5] 9p: disallow the NUL character in all strings

[Eric Blake]

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

On 08/26/2016 10:07 AM, Greg Kurz wrote:
> According to the 9P spec at http://man.cat-v.org/plan_9/5/intro :
> 
> Data items of larger or variable lengths are represented by a
> two-byte field specifying a count, n, followed by n bytes of
> data.  Text strings are represented this way, with the text
> itself stored as a UTF-8 encoded sequence of Unicode charac-
> ters (see utf(6)). Text strings in 9P messages are not NUL-
> terminated: n counts the bytes of UTF-8 data, which include
> no final zero byte.  The NUL character is illegal in all
> text strings in 9P, and is therefore excluded from file
> names, user names, and so on.
> 
> With this patch, if a 9P client sends a text string containing a NUL
> character, the request will fail and the client is returned EINVAL.
> 
> The checking is done in v9fs_iov_vunmarshal() because it is a convenient
> place to check all client originated strings.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  fsdev/9p-iov-marshal.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index 663cad542900..9bcdc370231d 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -127,7 +127,12 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
>                                       str->size);
>                  if (copied > 0) {
>                      str->data[str->size] = 0;
> -                } else {
> +                    /* 9P forbids NUL characters in all text strings */
> +                    if (strlen(str->data) != str->size) {

If this were glibc, we could micro-optimize and do:

if (rawmemchr(str->data, 0) != str->data + str->size)

so that strlen() doesn't have to visit the tail end of the string if a
NUL is present early.  But your code is just fine as-is, and doesn't
have to worry about rawmemchr() being present.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

[Eric Blake]

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

Re: [Qemu-devel] [PATCH v2 4/5] 9p: handle walk of ".." in the root directory

[Eric Blake]

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

On 08/26/2016 10:07 AM, Greg Kurz wrote:
> The 9P spec at http://man.cat-v.org/plan_9/5/intro says:
> 
> All directories must support walks to the directory .. (dot-dot) meaning
> parent directory, although by convention directories contain no explicit
> entry for .. or . (dot).  The parent of the root directory of a server's
> tree is itself.
> 
> This means that a client cannot walk further than the root directory
> exported by the server. In other words, if the client wants to walk
> "/.." or "/foo/../..", the server shoud answer like the request was

s/shoud/should/

> to walk "/".
> 
> This patch just does that:
> - we cache the QID of the root directory at attach time
> - during the walk we compare the QID of each path component with the root
>   QID to detect if we're in a "/.." situation
> - if so, we skip the current component and go to the next one
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c |   40 +++++++++++++++++++++++++++++++---------
>  hw/9pfs/9p.h |    1 +
>  2 files changed, 32 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Eric Blake]

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

On 08/26/2016 10:07 AM, Greg Kurz wrote:
> A buggy guest using the 9p2000.u protocol can issue a create request and
> pass an empty string as the extension argument. This causes QEMU to crash
> in the case of a hard link or a special file, and leads to undefined
> behavior, depending on the backend, in the case of a symbolic link.
> 
> This patch causes the request to fail with EINVAL in these scenarios.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c |   21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 7b1dfe4e47cb..dc65c3125006 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque)
>          }
>          fidp->fid_type = P9_FID_DIR;
>      } else if (perm & P9_STAT_MODE_SYMLINK) {
> +        if (extension.data == NULL) {
> +            err = -EINVAL;
> +            goto out;
> +        }

POSIX specifically requires implementations to support creating a
symlink whose target is the empty string.  Linux doesn't [yet] permit
it, but BSD does.  On systems where creating such a symlink is legal,
POSIX requires that such a symlink either be treated as "." if
dereferenced, or be treated as ENOENT on attempt to dereference.  But
since such links can be created, readlink() should be able to read them
without error.

I would argue that we should NOT forbid empty symlinks on creation (but
pass back any error from the underlying host OS); but instead check that
dereferencing such a symlink behaves sanely if it was created.
Meanwhile, a client should not be relying on the behavior (since Linux
disobeys POSIX, portable clients should already be avoiding empty symlinks).

http://austingroupbugs.net/view.php?id=649

> @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque)
>          }
>          v9fs_path_copy(&fidp->path, &path);
>      } else if (perm & P9_STAT_MODE_LINK) {
> -        int32_t ofid = atoi(extension.data);
> -        V9fsFidState *ofidp = get_fid(pdu, ofid);
> +        V9fsFidState *ofidp;
> +
> +        if (extension.data == NULL) {
> +            err = -EINVAL;
> +            goto out;
> +        }

Rejecting an empty destination on hard link or device creation, however,
is indeed appropriate.

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

Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Michael S. Tsirkin]

On Fri, Aug 26, 2016 at 02:00:37PM -0500, Eric Blake wrote:
> On 08/26/2016 10:07 AM, Greg Kurz wrote:
> > A buggy guest using the 9p2000.u protocol can issue a create request and
> > pass an empty string as the extension argument. This causes QEMU to crash
> > in the case of a hard link or a special file, and leads to undefined
> > behavior, depending on the backend, in the case of a symbolic link.
> > 
> > This patch causes the request to fail with EINVAL in these scenarios.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p.c |   21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 7b1dfe4e47cb..dc65c3125006 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque)
> >          }
> >          fidp->fid_type = P9_FID_DIR;
> >      } else if (perm & P9_STAT_MODE_SYMLINK) {
> > +        if (extension.data == NULL) {
> > +            err = -EINVAL;
> > +            goto out;
> > +        }
> 
> POSIX specifically requires implementations to support creating a
> symlink whose target is the empty string.  Linux doesn't [yet] permit
> it, but BSD does.  On systems where creating such a symlink is legal,
> POSIX requires that such a symlink either be treated as "." if
> dereferenced, or be treated as ENOENT on attempt to dereference.  But
> since such links can be created, readlink() should be able to read them
> without error.
> 
> I would argue that we should NOT forbid empty symlinks on creation (but
> pass back any error from the underlying host OS); but instead check that
> dereferencing such a symlink behaves sanely if it was created.
> Meanwhile, a client should not be relying on the behavior (since Linux
> disobeys POSIX, portable clients should already be avoiding empty symlinks).
> 
> http://austingroupbugs.net/view.php?id=649

Given 9p is only supported on Linux hosts, I think this patch's approach
is OK for now, and it's certainly much simpler than worrying about
the fallout of allowing empty names.

A TODO that documents your suggestions, and including
the considerations in the comment would be
a good idea.



> > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque)
> >          }
> >          v9fs_path_copy(&fidp->path, &path);
> >      } else if (perm & P9_STAT_MODE_LINK) {
> > -        int32_t ofid = atoi(extension.data);
> > -        V9fsFidState *ofidp = get_fid(pdu, ofid);
> > +        V9fsFidState *ofidp;
> > +
> > +        if (extension.data == NULL) {
> > +            err = -EINVAL;
> > +            goto out;
> > +        }
> 
> Rejecting an empty destination on hard link or device creation, however,
> is indeed appropriate.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v2 1/5] 9p: forbid illegal path names

[Greg Kurz]

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

On Fri, 26 Aug 2016 13:33:28 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 08/26/2016 10:07 AM, Greg Kurz wrote:
> > Empty path components don't make sense and may cause undefined behavior,
> > depending on the backend.
> > 
> > Also, the walk request described in the 9P spec [1] clearly shows that
> > the client is supposed to send individual path components: the official
> > linux client never sends portions of path containing the / character for
> > example.
> > 
> > Moreover, the 9P spec [2] also states that a system can decide to restrict
> > the set of supported characters used in path components, with an explicit
> > mention "to remove slashes from name components".
> > 
> > This patch introduces a new name_is_illegal() helper that checks the
> > names sent by the client are not empty and don't contain unwanted chars.
> > Since 9pfs is only supported on linux hosts, only the / character is
> > checked at the moment. When support for other hosts (AKA. win32) is added,
> > other chars may need to be blacklisted as well.
> > 
> > If a client sends an illegal path component, the request will fail and
> > EINVAL is returned to the client.
> > 
> > [1] http://man.cat-v.org/plan_9/5/walk
> > [2] http://man.cat-v.org/plan_9/5/intro
> > 
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index b6b02b46a9da..dba11773699b 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids)
> >      return offset;
> >  }
> >  
> > +static bool name_is_illegal(const char *name)
> > +{
> > +    return name == NULL || strchr(name, '/') != NULL;  
> 
> Is anyone actually passing NULL?  And the commit message says you are
> blacklisting the empty string "", but that is not what you did here.
> Depending on whether callers can even pass NULL, you may be able to just
> s/name == NULL/!*name/
> 

Oops you're right, v9fs_iov_vunmarshal() always allocates and sets str->data, even
for empty strings... so I guess this cannot be call with NULL and your suggestion
is the way to go.

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

Re: [Qemu-devel] [PATCH v2 2/5] 9p: disallow the NUL character in all strings

[Greg Kurz]

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

On Fri, 26 Aug 2016 13:41:23 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 08/26/2016 10:07 AM, Greg Kurz wrote:
> > According to the 9P spec at http://man.cat-v.org/plan_9/5/intro :
> > 
> > Data items of larger or variable lengths are represented by a
> > two-byte field specifying a count, n, followed by n bytes of
> > data.  Text strings are represented this way, with the text
> > itself stored as a UTF-8 encoded sequence of Unicode charac-
> > ters (see utf(6)). Text strings in 9P messages are not NUL-
> > terminated: n counts the bytes of UTF-8 data, which include
> > no final zero byte.  The NUL character is illegal in all
> > text strings in 9P, and is therefore excluded from file
> > names, user names, and so on.
> > 
> > With this patch, if a 9P client sends a text string containing a NUL
> > character, the request will fail and the client is returned EINVAL.
> > 
> > The checking is done in v9fs_iov_vunmarshal() because it is a convenient
> > place to check all client originated strings.
> > 
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  fsdev/9p-iov-marshal.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> > index 663cad542900..9bcdc370231d 100644
> > --- a/fsdev/9p-iov-marshal.c
> > +++ b/fsdev/9p-iov-marshal.c
> > @@ -127,7 +127,12 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
> >                                       str->size);
> >                  if (copied > 0) {
> >                      str->data[str->size] = 0;
> > -                } else {
> > +                    /* 9P forbids NUL characters in all text strings */
> > +                    if (strlen(str->data) != str->size) {  
> 
> If this were glibc, we could micro-optimize and do:
> 
> if (rawmemchr(str->data, 0) != str->data + str->size)
> 
> so that strlen() doesn't have to visit the tail end of the string if a
> NUL is present early.  But your code is just fine as-is, and doesn't

Hmmm... if a NUL is present early, why would strlen() visit the tail end of
the string ?

Looking at the glibc sources (string/strlen.c and string/rawmemchr.c), both calls
share the same implementation: handle initial characters 1 by 1 and then test a
longword at a time... and FWIW, strlen() knows at compile time it looks for 0 
instead of a runtime character for rawmemchr(). I have the feeling that strlen()
is the more optimized actually.

> have to worry about rawmemchr() being present.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


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

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

[Greg Kurz]

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

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

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

Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Greg Kurz]

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

On Fri, 26 Aug 2016 14:00:37 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 08/26/2016 10:07 AM, Greg Kurz wrote:
> > A buggy guest using the 9p2000.u protocol can issue a create request and
> > pass an empty string as the extension argument. This causes QEMU to crash
> > in the case of a hard link or a special file, and leads to undefined
> > behavior, depending on the backend, in the case of a symbolic link.
> > 
> > This patch causes the request to fail with EINVAL in these scenarios.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---

Wait... empty strings coming from pdu_unmarshal() never have data == NULL
so this whole patch is pointless :) and BTW, only the symlink case is about
file names.

> >  hw/9pfs/9p.c |   21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 7b1dfe4e47cb..dc65c3125006 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque)
> >          }
> >          fidp->fid_type = P9_FID_DIR;
> >      } else if (perm & P9_STAT_MODE_SYMLINK) {
> > +        if (extension.data == NULL) {
> > +            err = -EINVAL;
> > +            goto out;
> > +        }  
>

I realize that this part belongs to patch 1 actually: it is the implementation of
symbolic links that comes with 9P2000.u (different from the TSYMLINK request in
9P2000.L). In which case, the hunk would have been:

+        if (name_is_illegal(extension.data)) {
+            err = -EINVAL;
+            goto out;
+        }

> POSIX specifically requires implementations to support creating a
> symlink whose target is the empty string.  Linux doesn't [yet] permit
> it, but BSD does.  On systems where creating such a symlink is legal,
> POSIX requires that such a symlink either be treated as "." if
> dereferenced, or be treated as ENOENT on attempt to dereference.  But
> since such links can be created, readlink() should be able to read them
> without error.
> 
> I would argue that we should NOT forbid empty symlinks on creation (but
> pass back any error from the underlying host OS); but instead check that
> dereferencing such a symlink behaves sanely if it was created.
> Meanwhile, a client should not be relying on the behavior (since Linux
> disobeys POSIX, portable clients should already be avoiding empty symlinks).
> 
> http://austingroupbugs.net/view.php?id=649
> 

Indeed, maybe we should let the backend decide if it allows symlink with
an empty target, since the target name is simply "stored" somewhere and
not used to create a new path. In which case, we should do the same with
v9fs_symlink(). And we would have two exceptions to the name_is_illegal()
helper, because we still want to avoid '/' in file names...

On the other hand, we only support linux hosts where the call to symlink()
will fail with ENOENT and guests using the official linux kernel 9p client
never send an empty target...

For the sake of simplicity, I'd rather have the target names to follow the
same rules as other file names, and return ENOENT directly (the link you
provide states it is a valid option).

Peter,

Since you suggested to do explicit error checking on empty file names, do
you have an opinion on the case of symlinks with an empty target ?

> > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque)
> >          }
> >          v9fs_path_copy(&fidp->path, &path);
> >      } else if (perm & P9_STAT_MODE_LINK) {
> > -        int32_t ofid = atoi(extension.data);
> > -        V9fsFidState *ofidp = get_fid(pdu, ofid);
> > +        V9fsFidState *ofidp;
> > +
> > +        if (extension.data == NULL) {
> > +            err = -EINVAL;
> > +            goto out;
> > +        }  
> 
> Rejecting an empty destination on hard link or device creation, however,
> is indeed appropriate.
> 

In the case of hard links, extension.data is a FID, not a file name.

In the case of device creation, extension.data is "type major minor", not
a file name again.

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

Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Greg Kurz]

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

On Sun, 28 Aug 2016 19:21:25 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 26 Aug 2016 14:00:37 -0500
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 08/26/2016 10:07 AM, Greg Kurz wrote:  
> > > A buggy guest using the 9p2000.u protocol can issue a create request and
> > > pass an empty string as the extension argument. This causes QEMU to crash
> > > in the case of a hard link or a special file, and leads to undefined
> > > behavior, depending on the backend, in the case of a symbolic link.
> > > 
> > > This patch causes the request to fail with EINVAL in these scenarios.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---  
> 
> Wait... empty strings coming from pdu_unmarshal() never have data == NULL
> so this whole patch is pointless :) and BTW, only the symlink case is about
> file names.
> 
> > >  hw/9pfs/9p.c |   21 +++++++++++++++++++--
> > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 7b1dfe4e47cb..dc65c3125006 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque)
> > >          }
> > >          fidp->fid_type = P9_FID_DIR;
> > >      } else if (perm & P9_STAT_MODE_SYMLINK) {
> > > +        if (extension.data == NULL) {
> > > +            err = -EINVAL;
> > > +            goto out;
> > > +        }    
> >  
> 
> I realize that this part belongs to patch 1 actually: it is the implementation of
> symbolic links that comes with 9P2000.u (different from the TSYMLINK request in
> 9P2000.L). In which case, the hunk would have been:
> 
> +        if (name_is_illegal(extension.data)) {
> +            err = -EINVAL;
> +            goto out;
> +        }
> 
> > POSIX specifically requires implementations to support creating a
> > symlink whose target is the empty string.  Linux doesn't [yet] permit
> > it, but BSD does.  On systems where creating such a symlink is legal,
> > POSIX requires that such a symlink either be treated as "." if
> > dereferenced, or be treated as ENOENT on attempt to dereference.  But
> > since such links can be created, readlink() should be able to read them
> > without error.
> > 
> > I would argue that we should NOT forbid empty symlinks on creation (but
> > pass back any error from the underlying host OS); but instead check that
> > dereferencing such a symlink behaves sanely if it was created.
> > Meanwhile, a client should not be relying on the behavior (since Linux
> > disobeys POSIX, portable clients should already be avoiding empty symlinks).
> > 
> > http://austingroupbugs.net/view.php?id=649
> >   
> 
> Indeed, maybe we should let the backend decide if it allows symlink with
> an empty target, since the target name is simply "stored" somewhere and
> not used to create a new path. In which case, we should do the same with
> v9fs_symlink(). And we would have two exceptions to the name_is_illegal()
> helper, because we still want to avoid '/' in file names...
> 

Thinking again... I guess '/' in names should result in ENOENT since it seems
to be a common way to say something is wrong with a path... or we should have
separate error paths between the '/'-in-names and the empty name cases.

> On the other hand, we only support linux hosts where the call to symlink()
> will fail with ENOENT and guests using the official linux kernel 9p client
> never send an empty target...
> 
> For the sake of simplicity, I'd rather have the target names to follow the
> same rules as other file names, and return ENOENT directly (the link you
> provide states it is a valid option).
> 
> Peter,
> 
> Since you suggested to do explicit error checking on empty file names, do
> you have an opinion on the case of symlinks with an empty target ?
> 
> > > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque)
> > >          }
> > >          v9fs_path_copy(&fidp->path, &path);
> > >      } else if (perm & P9_STAT_MODE_LINK) {
> > > -        int32_t ofid = atoi(extension.data);
> > > -        V9fsFidState *ofidp = get_fid(pdu, ofid);
> > > +        V9fsFidState *ofidp;
> > > +
> > > +        if (extension.data == NULL) {
> > > +            err = -EINVAL;
> > > +            goto out;
> > > +        }    
> > 
> > Rejecting an empty destination on hard link or device creation, however,
> > is indeed appropriate.
> >   
> 
> In the case of hard links, extension.data is a FID, not a file name.
> 
> In the case of device creation, extension.data is "type major minor", not
> a file name again.


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

Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Peter Maydell]

On 28 August 2016 at 13:21, Greg Kurz <groug@kaod.org> wrote:
> Peter,
>
> Since you suggested to do explicit error checking on empty file names, do
> you have an opinion on the case of symlinks with an empty target ?

I have no strong opinion here. I was just aiming for the general
principles of consistency and performing checks for invalid requests
in the common layer code where possible.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] 9p: disallow the NUL character in all strings

[Greg Kurz]

On Fri, 26 Aug 2016 17:07:08 +0200
Greg Kurz <groug@kaod.org> wrote:

> According to the 9P spec at http://man.cat-v.org/plan_9/5/intro :
> 
> Data items of larger or variable lengths are represented by a
> two-byte field specifying a count, n, followed by n bytes of
> data.  Text strings are represented this way, with the text
> itself stored as a UTF-8 encoded sequence of Unicode charac-
> ters (see utf(6)). Text strings in 9P messages are not NUL-
> terminated: n counts the bytes of UTF-8 data, which include
> no final zero byte.  The NUL character is illegal in all
> text strings in 9P, and is therefore excluded from file
> names, user names, and so on.
> 
> With this patch, if a 9P client sends a text string containing a NUL
> character, the request will fail and the client is returned EINVAL.
> 
> The checking is done in v9fs_iov_vunmarshal() because it is a convenient
> place to check all client originated strings.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  fsdev/9p-iov-marshal.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index 663cad542900..9bcdc370231d 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -127,7 +127,12 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
>                                       str->size);
>                  if (copied > 0) {
>                      str->data[str->size] = 0;
> -                } else {
> +                    /* 9P forbids NUL characters in all text strings */
> +                    if (strlen(str->data) != str->size) {
> +                        copied = -EINVAL;
> +                    }

Ugh ! QEMU sends terminating NULs :-\ which cause virtfs-proxy-helper to fail
here during init.

> +                }
> +                if (copied <= 0) {
>                      v9fs_string_free(str);
>                  }
>              }
> 
> 

Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Eric Blake]

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

On 08/28/2016 12:34 PM, Greg Kurz wrote:

>>
>> For the sake of simplicity, I'd rather have the target names to follow the
>> same rules as other file names, and return ENOENT directly (the link you
>> provide states it is a valid option).
>>
>> Peter,
>>
>> Since you suggested to do explicit error checking on empty file names, do
>> you have an opinion on the case of symlinks with an empty target ?

Failing with ENOENT for both "" and for "a/b" seems reasonable to me (a
directory entry named "a/b" can never exist, just as "" cannot exist).

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

Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Greg Kurz]

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

On Mon, 29 Aug 2016 14:35:07 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 08/28/2016 12:34 PM, Greg Kurz wrote:
> 
> >>
> >> For the sake of simplicity, I'd rather have the target names to follow the
> >> same rules as other file names, and return ENOENT directly (the link you
> >> provide states it is a valid option).
> >>
> >> Peter,
> >>
> >> Since you suggested to do explicit error checking on empty file names, do
> >> you have an opinion on the case of symlinks with an empty target ?  
> 
> Failing with ENOENT for both "" and for "a/b" seems reasonable to me (a
> directory entry named "a/b" can never exist, just as "" cannot exist).
> 

But of course the target may still be a path with slashes :)

And since we don't care for / until the link is dereferenced, I'm not so sure
there is any checking to be done on the target name actually.

--
Greg

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