All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs
@ 2017-01-10 14:32 Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories Greg Kurz
                   ` (6 more replies)
  0 siblings, 7 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

The 9P open and create operations cause QEMU to call the open(2) syscall on
the backend. Unfortunately, it doesn't check if the given pathname already
exists and points to something that could cause open(2) to hang (like a named
pipe for example).

This series fixes the issues for open (only allow directories and regular
files) and create (enforce O_EXCL). It also adds the corresponding tests in
virtio-9p-test.

---

Greg Kurz (7):
      9pfs: restrict open to regular files and directories
      tests: virtio-9p: improve error reporting
      tests: virtio-9p: add LOPEN operation test
      tests: virtio-9p: TLOPEN should fail to open a FIFO
      9pfs: don't create files if pathname already exists
      tests: virtio-9p: add LCREATE operation test
      tests: virtio-9p: TLCREATE should fail if pathname exists


 hw/9pfs/9p.c           |   10 ++
 tests/virtio-9p-test.c |  197 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 199 insertions(+), 8 deletions(-)

--
Greg

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories
  2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
@ 2017-01-10 14:32 ` Greg Kurz
  2017-01-10 14:38   ` Eric Blake
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 2/7] tests: virtio-9p: improve error reporting Greg Kurz
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

It really does not make sense for the 9P server to open anything else but
a regular file or a directory.

Malicious code in a guest could for example create a named pipe, associate
it to a valid fid and pass it to the server in a RLOPEN message. This would
cause QEMU to hang in open(), waiting for someone to open the other end of
the pipe.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index fa58877570f6..edd7b97270e3 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1462,7 +1462,7 @@ static void coroutine_fn v9fs_open(void *opaque)
             goto out;
         }
         err += offset;
-    } else {
+    } else if (S_ISREG(stbuf.st_mode)) {
         if (s->proto_version == V9FS_PROTO_2000L) {
             flags = get_dotl_openflags(s, mode);
         } else {
@@ -1494,6 +1494,9 @@ static void coroutine_fn v9fs_open(void *opaque)
             goto out;
         }
         err += offset;
+    } else {
+        err = -EINVAL;
+        goto out;
     }
     trace_v9fs_open_return(pdu->tag, pdu->id,
                            qid.type, qid.version, qid.path, iounit);

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 2/7] tests: virtio-9p: improve error reporting
  2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories Greg Kurz
@ 2017-01-10 14:32 ` Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 3/7] tests: virtio-9p: add LOPEN operation test Greg Kurz
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 060407b20e39..9556291567a4 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -236,6 +236,16 @@ static void v9fs_req_send(P9Req *req)
     req->t_off = 0;
 }
 
+static const char *rmessage_name(uint8_t id)
+{
+    return
+        id == P9_RLERROR ? "RLERROR" :
+        id == P9_RVERSION ? "RVERSION" :
+        id == P9_RATTACH ? "RATTACH" :
+        id == P9_RWALK ? "RWALK" :
+        "<unknown>";
+}
+
 static void v9fs_req_recv(P9Req *req, uint8_t id)
 {
     QVirtIO9P *v9p = req->v9p;
@@ -258,11 +268,15 @@ static void v9fs_req_recv(P9Req *req, uint8_t id)
     g_assert_cmpint(hdr.size, <=, P9_MAX_SIZE);
     g_assert_cmpint(hdr.tag, ==, req->tag);
 
-    if (hdr.id != id && hdr.id == P9_RLERROR) {
-        uint32_t err;
-        v9fs_uint32_read(req, &err);
-        g_printerr("Received Rlerror (%d) instead of Response %d\n", err, id);
-        g_assert_not_reached();
+    if (hdr.id != id) {
+        g_printerr("Received response %d (%s) instead of %d (%s)\n",
+                   hdr.id, rmessage_name(hdr.id), id, rmessage_name(id));
+
+        if (hdr.id == P9_RLERROR) {
+            uint32_t err;
+            v9fs_uint32_read(req, &err);
+            g_printerr("Rlerror has errno %d (%s)\n", err, strerror(err));
+        }
     }
     g_assert_cmpint(hdr.id, ==, id);
 }

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 3/7] tests: virtio-9p: add LOPEN operation test
  2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 2/7] tests: virtio-9p: improve error reporting Greg Kurz
@ 2017-01-10 14:32 ` Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 4/7] tests: virtio-9p: TLOPEN should fail to open a FIFO Greg Kurz
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

The "fs_lopen" test creates a file on the backend, walks to it with
the walk operation and issues a LOPEN operation, which is expected to
succeed. Note that LOPEN is the OPEN operation implemented by the
9P2000.L version of the 9P protocol, which is used by the linux kernel
client.

References:

https://github.com/chaos/diod/blob/master/protocol.md#lopen----open-a-file
http://man.cat-v.org/plan_9/5/open

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 9556291567a4..14b4787930ae 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -243,6 +243,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RVERSION ? "RVERSION" :
         id == P9_RATTACH ? "RATTACH" :
         id == P9_RWALK ? "RWALK" :
+        id == P9_RLOPEN ? "RLOPEN" :
         "<unknown>";
 }
 
@@ -395,6 +396,33 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
     v9fs_req_free(req);
 }
 
+/* size[4] Tlopen tag[2] fid[4] flags[4] */
+static P9Req *v9fs_tlopen(QVirtIO9P *v9p, uint32_t fid, uint32_t flags)
+{
+    P9Req *req;
+
+    req = v9fs_req_init(v9p,  4 + 4, P9_TLOPEN, ++(v9p->p9_req_tag));
+    v9fs_uint32_write(req, fid);
+    v9fs_uint32_write(req, flags);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rlopen tag[2] qid[13] iounit[4] */
+static void v9fs_rlopen(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
+{
+    v9fs_req_recv(req, P9_RLOPEN);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    } else {
+        v9fs_memskip(req, 13);
+    }
+    if (iounit) {
+        v9fs_uint32_read(req, iounit);
+    }
+    v9fs_req_free(req);
+}
+
 static void fs_version(QVirtIO9P *v9p)
 {
     const char *version = "9P2000.L";
@@ -483,6 +511,32 @@ static void fs_walk_dotdot(QVirtIO9P *v9p)
     g_free(wnames[0]);
 }
 
+static void fs_lopen(QVirtIO9P *v9p)
+{
+    char *const wnames[] = { g_strdup(__func__) };
+    char *path = g_strdup_printf("%s/%s", v9p->test_share, wnames[0]);
+    uint32_t iounit;
+    int fd;
+    P9Req *req;
+
+    fd = creat(path, 0600);
+    g_assert(fd >= 0);
+    close(fd);
+
+    fs_attach(v9p);
+    req = v9fs_twalk(v9p, 0, 1, 1, wnames);
+    v9fs_rwalk(req, NULL, NULL);
+
+    req = v9fs_tlopen(v9p, 1, O_RDONLY);
+    v9fs_rlopen(req, NULL, &iounit);
+
+    g_assert_cmpint(iounit, <=, P9_MAX_SIZE - P9_IOHDRSZ);
+
+    unlink(path);
+    g_free(path);
+    g_free(wnames[0]);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -512,6 +566,7 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/no_slash", fs_walk_no_slash);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/dotdot_from_root",
                        fs_walk_dotdot);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/lopen/basic", fs_lopen);
 
     return g_test_run();
 }

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 4/7] tests: virtio-9p: TLOPEN should fail to open a FIFO
  2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
                   ` (2 preceding siblings ...)
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 3/7] tests: virtio-9p: add LOPEN operation test Greg Kurz
@ 2017-01-10 14:32 ` Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 5/7] 9pfs: don't create files if pathname already exists Greg Kurz
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

A 9P server should only try to open regular files and directories.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 14b4787930ae..334833f88a6e 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -537,6 +537,29 @@ static void fs_lopen(QVirtIO9P *v9p)
     g_free(wnames[0]);
 }
 
+static void fs_lopen_fifo_not_allowed(QVirtIO9P *v9p)
+{
+    char *const wnames[] = { g_strdup(__func__) };
+    char *test_fifo;
+    P9Req *req;
+    uint32_t err;
+
+    test_fifo = g_strdup_printf("%s/%s", v9p->test_share, wnames[0]);
+    g_assert(mkfifo(test_fifo, 0600) == 0);
+
+    fs_attach(v9p);
+    req = v9fs_twalk(v9p, 0, 1, 1, wnames);
+    v9fs_rwalk(req, NULL, NULL);
+    req = v9fs_tlopen(v9p, 1, O_RDONLY);
+    v9fs_rlerror(req, &err);
+
+    g_assert_cmpint(err, ==, EINVAL);
+
+    unlink(test_fifo);
+    g_free(test_fifo);
+    g_free(wnames[0]);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -567,6 +590,8 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/walk/dotdot_from_root",
                        fs_walk_dotdot);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/lopen/basic", fs_lopen);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/lopen/fifo_not_allowed",
+                       fs_lopen_fifo_not_allowed);
 
     return g_test_run();
 }

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 5/7] 9pfs: don't create files if pathname already exists
  2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
                   ` (3 preceding siblings ...)
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 4/7] tests: virtio-9p: TLOPEN should fail to open a FIFO Greg Kurz
@ 2017-01-10 14:32 ` Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 6/7] tests: virtio-9p: add LCREATE operation test Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 7/7] tests: virtio-9p: TLCREATE should fail if pathname exists Greg Kurz
  6 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

As specified in the http://man.cat-v.org/plan_9/5/open :

"An attempt to create a file in a directory where the given
name already exists will be rejected"

Malicious code in a guest could for example create a named pipe and
then pass its name to the server in a RLCREATE message. This would
cause QEMU to hang in open(), waiting for someone to open the other
end of the pipe.

Let's fix this by simply using O_EXCL.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index edd7b97270e3..d9686601deda 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1545,7 +1545,7 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
 
     flags = get_dotl_openflags(pdu->s, flags);
     err = v9fs_co_open2(pdu, fidp, &name, gid,
-                        flags | O_CREAT, mode, &stbuf);
+                        flags | O_CREAT | O_EXCL, mode, &stbuf);
     if (err < 0) {
         goto out;
     }
@@ -2252,7 +2252,8 @@ static void coroutine_fn v9fs_create(void *opaque)
         v9fs_path_copy(&fidp->path, &path);
     } else {
         err = v9fs_co_open2(pdu, fidp, &name, -1,
-                            omode_to_uflags(mode)|O_CREAT, perm, &stbuf);
+                            omode_to_uflags(mode) | O_CREAT | O_EXCL, perm,
+                            &stbuf);
         if (err < 0) {
             goto out;
         }

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 6/7] tests: virtio-9p: add LCREATE operation test
  2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
                   ` (4 preceding siblings ...)
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 5/7] 9pfs: don't create files if pathname already exists Greg Kurz
@ 2017-01-10 14:32 ` Greg Kurz
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 7/7] tests: virtio-9p: TLCREATE should fail if pathname exists Greg Kurz
  6 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

The "fs_lcreate" test clones the root directory fid, walks to it and
issues a LCREATE operation, which is expected to succeed. It then
ensures a file was indeed created in the underlying filesystem.
Note that LCREATE is the CREATE operation implemented by the 9P2000.L
version of the 9P protocol, which is used by the linux kernel client.

References:

https://github.com/chaos/diod/blob/master/protocol.md#lcreate----create-regular-file
http://man.cat-v.org/plan_9/5/open

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 334833f88a6e..7f2ee41ca0f0 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -244,6 +244,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RATTACH ? "RATTACH" :
         id == P9_RWALK ? "RWALK" :
         id == P9_RLOPEN ? "RLOPEN" :
+        id == P9_RLCREATE ? "RLCREATE" :
         "<unknown>";
 }
 
@@ -423,6 +424,38 @@ static void v9fs_rlopen(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
     v9fs_req_free(req);
 }
 
+/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
+static P9Req *v9fs_tlcreate(QVirtIO9P *v9p, uint32_t fid, const char *name,
+                            uint32_t flags, uint32_t mode, uint32_t gid)
+{
+    P9Req *req;
+
+    req = v9fs_req_init(v9p,  4 + 4 + v9fs_string_size(name) + 4 + 4 + 4,
+                        P9_TLCREATE, ++(v9p->p9_req_tag));
+    v9fs_uint32_write(req, fid);
+    v9fs_string_write(req, name);
+    v9fs_uint32_write(req, flags);
+    v9fs_uint32_write(req, mode);
+    v9fs_uint32_write(req, gid);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
+static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
+{
+    v9fs_req_recv(req, P9_RLCREATE);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    } else {
+        v9fs_memskip(req, 13);
+    }
+    if (iounit) {
+        v9fs_uint32_read(req, iounit);
+    }
+    v9fs_req_free(req);
+}
+
 static void fs_version(QVirtIO9P *v9p)
 {
     const char *version = "9P2000.L";
@@ -560,6 +593,34 @@ static void fs_lopen_fifo_not_allowed(QVirtIO9P *v9p)
     g_free(wnames[0]);
 }
 
+static void fs_lcreate(QVirtIO9P *v9p)
+{
+    char *const wnames[] = { g_strdup(__func__) };
+    char *path = g_strdup_printf("%s/%s", v9p->test_share, wnames[0]);
+    uint32_t iounit;
+    P9Req *req;
+
+    fs_attach(v9p);
+    /* Clone the root directory fid and open it */
+    req = v9fs_twalk(v9p, 0, 1, 0, NULL);
+    v9fs_rwalk(req, NULL, NULL);
+    req = v9fs_tlopen(v9p, 1, O_RDWR);
+    v9fs_rlopen(req, NULL, NULL);
+
+    req = v9fs_tlcreate(v9p, 1, wnames[0], O_RDONLY, 0600, getgid());
+    v9fs_rlcreate(req, NULL, &iounit);
+
+    g_assert_cmpint(iounit, <=, P9_MAX_SIZE - P9_IOHDRSZ);
+
+    /* Was the file created ? */
+    req = v9fs_twalk(v9p, 0, 2, 1, wnames);
+    v9fs_rwalk(req, NULL, NULL);
+
+    unlink(path);
+    g_free(path);
+    g_free(wnames[0]);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -592,6 +653,7 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/lopen/basic", fs_lopen);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/lopen/fifo_not_allowed",
                        fs_lopen_fifo_not_allowed);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/lcreate/basic", fs_lcreate);
 
     return g_test_run();
 }

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 7/7] tests: virtio-9p: TLCREATE should fail if pathname exists
  2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
                   ` (5 preceding siblings ...)
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 6/7] tests: virtio-9p: add LCREATE operation test Greg Kurz
@ 2017-01-10 14:32 ` Greg Kurz
  6 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-10 14:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Aneesh Kumar K.V, Greg Kurz

As specified in the http://man.cat-v.org/plan_9/5/open :

"An attempt to create a file in a directory where the given
name already exists will be rejected"

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 7f2ee41ca0f0..2ba321b990d3 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -621,6 +621,35 @@ static void fs_lcreate(QVirtIO9P *v9p)
     g_free(wnames[0]);
 }
 
+static void fs_lcreate_exclusive(QVirtIO9P *v9p)
+{
+    char *const wnames[] = { g_strdup(__func__) };
+    char *path = g_strdup_printf("%s/%s", v9p->test_share, wnames[0]);
+    int fd;
+    uint32_t err;
+    P9Req *req;
+
+    fd = creat(path, 0600);
+    g_assert(fd >= 0);
+    close(fd);
+
+    fs_attach(v9p);
+    /* Clone the root directory fid and open it */
+    req = v9fs_twalk(v9p, 0, 1, 0, NULL);
+    v9fs_rwalk(req, NULL, NULL);
+    req = v9fs_tlopen(v9p, 1, O_RDWR);
+    v9fs_rlopen(req, NULL, NULL);
+
+    req = v9fs_tlcreate(v9p, 1, wnames[0], O_RDONLY, 0600, getgid());
+    v9fs_rlerror(req, &err);
+
+    g_assert_cmpint(err, ==, EEXIST);
+
+    unlink(path);
+    g_free(path);
+    g_free(wnames[0]);
+}
+
 typedef void (*v9fs_test_fn)(QVirtIO9P *v9p);
 
 static void v9fs_run_pci_test(gconstpointer data)
@@ -654,6 +683,8 @@ int main(int argc, char **argv)
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/lopen/fifo_not_allowed",
                        fs_lopen_fifo_not_allowed);
     v9fs_qtest_pci_add("/virtio/9p/pci/fs/lcreate/basic", fs_lcreate);
+    v9fs_qtest_pci_add("/virtio/9p/pci/fs/lcreate/exclusive",
+                       fs_lcreate_exclusive);
 
     return g_test_run();
 }

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories
  2017-01-10 14:32 ` [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories Greg Kurz
@ 2017-01-10 14:38   ` Eric Blake
  2017-01-10 14:41     ` Eric Blake
  2017-01-11  9:54     ` Greg Kurz
  0 siblings, 2 replies; 11+ messages in thread
From: Eric Blake @ 2017-01-10 14:38 UTC (permalink / raw)
  To: Greg Kurz, qemu-devel; +Cc: Aneesh Kumar K.V

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

On 01/10/2017 08:32 AM, Greg Kurz wrote:
> It really does not make sense for the 9P server to open anything else but
> a regular file or a directory.
> 
> Malicious code in a guest could for example create a named pipe, associate
> it to a valid fid and pass it to the server in a RLOPEN message. This would
> cause QEMU to hang in open(), waiting for someone to open the other end of
> the pipe.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index fa58877570f6..edd7b97270e3 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1462,7 +1462,7 @@ static void coroutine_fn v9fs_open(void *opaque)
>              goto out;
>          }
>          err += offset;
> -    } else {
> +    } else if (S_ISREG(stbuf.st_mode)) {
>          if (s->proto_version == V9FS_PROTO_2000L) {

TOCTTOU race.  You are checking the stat() results and only then calling
open(), rather than calling open() first and validating fstat().  That
means the guest can STILL cause you to open() a pipe by changing the
file type in between the stat and the open.

I think you need to rework this patch to open() first, then validate
(closing the fd if necessary); the open can be done with O_NONBLOCK to
avoid hanging on a pipe.  Yes, that's more annoying, but that's life
with TOCTTOU races.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories
  2017-01-10 14:38   ` Eric Blake
@ 2017-01-10 14:41     ` Eric Blake
  2017-01-11  9:54     ` Greg Kurz
  1 sibling, 0 replies; 11+ messages in thread
From: Eric Blake @ 2017-01-10 14:41 UTC (permalink / raw)
  To: Greg Kurz, qemu-devel; +Cc: Aneesh Kumar K.V

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

On 01/10/2017 08:38 AM, Eric Blake wrote:
> I think you need to rework this patch to open() first, then validate
> (closing the fd if necessary); the open can be done with O_NONBLOCK to
> avoid hanging on a pipe.

And O_NOCTTY to avoid becoming the controlling process of a terminal.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories
  2017-01-10 14:38   ` Eric Blake
  2017-01-10 14:41     ` Eric Blake
@ 2017-01-11  9:54     ` Greg Kurz
  1 sibling, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2017-01-11  9:54 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, Aneesh Kumar K.V

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

On Tue, 10 Jan 2017 08:38:27 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 01/10/2017 08:32 AM, Greg Kurz wrote:
> > It really does not make sense for the 9P server to open anything else but
> > a regular file or a directory.
> > 
> > Malicious code in a guest could for example create a named pipe, associate
> > it to a valid fid and pass it to the server in a RLOPEN message. This would
> > cause QEMU to hang in open(), waiting for someone to open the other end of
> > the pipe.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index fa58877570f6..edd7b97270e3 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1462,7 +1462,7 @@ static void coroutine_fn v9fs_open(void *opaque)
> >              goto out;
> >          }
> >          err += offset;
> > -    } else {
> > +    } else if (S_ISREG(stbuf.st_mode)) {
> >          if (s->proto_version == V9FS_PROTO_2000L) {  
> 
> TOCTTOU race.  You are checking the stat() results and only then calling
> open(), rather than calling open() first and validating fstat().  That
> means the guest can STILL cause you to open() a pipe by changing the
> file type in between the stat and the open.
> 

You're right. And of course, this TOCTTOU race also affects the
file versus directory choice: we can end up with a fid with type
P9_FID_FILE whereas the underlying fd points to a directory... not
sure how the rest of the code copes with that. :-\

Side note: support for older versions of the protocol greatly
contribute to the overall obfuscation of the code... I wonder
if we have non-V9FS_PROTO_2000L users, and how I could deprecate
the damn thing...

> I think you need to rework this patch to open() first, then validate
> (closing the fd if necessary); the open can be done with O_NONBLOCK to
> avoid hanging on a pipe.  Yes, that's more annoying, but that's life
> with TOCTTOU races.
> 

Yeah... and even if linux doesn't implement O_NONBLOCK behaviour for files,
for completeness we should revert this with fcntl(). The fix should hence
be done at the backend level.

Good news, at least: the directory branch will eventually call
opendir()->open(O_DIRECTORY) and thus doesn't need fixing.
Also the fixing of the regular file case will also take care of
the P9_FID_FILE/directory discrepancy mentioned above.

Thanks for the advice anyway. :)

--
Greg

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-01-11  9:54 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-10 14:32 [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs Greg Kurz
2017-01-10 14:32 ` [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories Greg Kurz
2017-01-10 14:38   ` Eric Blake
2017-01-10 14:41     ` Eric Blake
2017-01-11  9:54     ` Greg Kurz
2017-01-10 14:32 ` [Qemu-devel] [PATCH 2/7] tests: virtio-9p: improve error reporting Greg Kurz
2017-01-10 14:32 ` [Qemu-devel] [PATCH 3/7] tests: virtio-9p: add LOPEN operation test Greg Kurz
2017-01-10 14:32 ` [Qemu-devel] [PATCH 4/7] tests: virtio-9p: TLOPEN should fail to open a FIFO Greg Kurz
2017-01-10 14:32 ` [Qemu-devel] [PATCH 5/7] 9pfs: don't create files if pathname already exists Greg Kurz
2017-01-10 14:32 ` [Qemu-devel] [PATCH 6/7] tests: virtio-9p: add LCREATE operation test Greg Kurz
2017-01-10 14:32 ` [Qemu-devel] [PATCH 7/7] tests: virtio-9p: TLCREATE should fail if pathname exists Greg Kurz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.