From 2e2293c238d4daa791ab7ff60b326a322892df3a Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sat, 8 Feb 2020 09:24:19 +0100 Subject: [PATCH 1/6] tests/virtio-9p: add terminating null in v9fs_string_read() The 9p protocol sends strings in general without null termination over the wire. However for future use of this functions it is beneficial for the delivered string to be null terminated though for being able to use the string with standard C functions which often rely on strings being null terminated. Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: <52c84e2ce3bcafc2a38eed13b8c8e23bc1a8ecb9.1579567019.git.qemu_oss@crudebyte.com> Signed-off-by: Greg Kurz --- tests/qtest/virtio-9p-test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c index e7b58e3a0c..06263edb53 100644 --- a/tests/qtest/virtio-9p-test.c +++ b/tests/qtest/virtio-9p-test.c @@ -130,8 +130,9 @@ static void v9fs_string_read(P9Req *req, uint16_t *len, char **string) *len = local_len; } if (string) { - *string = g_malloc(local_len); + *string = g_malloc(local_len + 1); v9fs_memread(req, *string, local_len); + (*string)[local_len] = 0; } else { v9fs_memskip(req, local_len); } From e16453a31a00c1c0a199cab0617e8aa888f6419a Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sat, 8 Feb 2020 09:24:19 +0100 Subject: [PATCH 2/6] 9pfs: require msize >= 4096 A client establishes a session by sending a Tversion request along with a 'msize' parameter which client uses to suggest server a maximum message size ever to be used for communication (for both requests and replies) between client and server during that session. If client suggests a 'msize' smaller than 4096 then deny session by server immediately with an error response (Rlerror for "9P2000.L" clients or Rerror for "9P2000.u" clients) instead of replying with Rversion. So far any msize submitted by client with Tversion was simply accepted by server without any check. Introduction of some minimum msize makes sense, because e.g. a msize < 7 would not allow any subsequent 9p operation at all, because 7 is the size of the header section common by all 9p message types. A substantial higher value of 4096 was chosen though to prevent potential issues with some message types. E.g. Rreadlink may yield up to a size of PATH_MAX which is usually 4096, and like almost all 9p message types, Rreadlink is not allowed to be truncated by the 9p protocol. This chosen size also prevents a similar issue with Rreaddir responses (provided client always sends adequate 'count' parameter with Treaddir), because even though directory entries retrieval may be split up over several T/Rreaddir messages; a Rreaddir response must not truncate individual directory entries though. So msize should be large enough to return at least one directory entry with the longest possible file name supported by host. Most file systems support a max. file name length of 255. Largest known file name lenght limit would be currently ReiserFS with max. 4032 bytes, which is also covered by this min. msize value because 4032 + 35 < 4096. Furthermore 4096 is already the minimum msize of the Linux kernel's 9pfs client. Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: <8ceecb7fb9fdbeabbe55c04339349a36929fb8e3.1579567019.git.qemu_oss@crudebyte.com> Signed-off-by: Greg Kurz --- hw/9pfs/9p.c | 12 ++++++++++++ hw/9pfs/9p.h | 11 +++++++++++ 2 files changed, 23 insertions(+) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index b0e445d6bd..c63f549f39 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1363,8 +1363,20 @@ static void coroutine_fn v9fs_version(void *opaque) s->proto_version = V9FS_PROTO_2000L; } else { v9fs_string_sprintf(&version, "unknown"); + /* skip min. msize check, reporting invalid version has priority */ + goto marshal; } + if (s->msize < P9_MIN_MSIZE) { + err = -EMSGSIZE; + error_report( + "9pfs: Client requested msize < minimum msize (" + stringify(P9_MIN_MSIZE) ") supported by this server." + ); + goto out; + } + +marshal: err = pdu_marshal(pdu, offset, "ds", s->msize, &version); if (err < 0) { goto out; diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index 8d07a0b301..b8f72a3bd9 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -100,6 +100,17 @@ typedef enum P9ProtoVersion { V9FS_PROTO_2000L = 0x02, } P9ProtoVersion; +/** + * @brief Minimum message size supported by this 9pfs server. + * + * A client establishes a session by sending a Tversion request along with a + * 'msize' parameter which suggests the server a maximum message size ever to be + * used for communication (for both requests and replies) between client and + * server during that session. If client suggests a 'msize' smaller than this + * value then session is denied by server with an error response. + */ +#define P9_MIN_MSIZE 4096 + #define P9_NOTAG UINT16_MAX #define P9_NOFID UINT32_MAX #define P9_MAXWELEM 16 From d36a5c227099f3f74da27a730350b012b6e99cdd Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sat, 8 Feb 2020 09:24:19 +0100 Subject: [PATCH 3/6] 9pfs: validate count sent by client with T_readdir A good 9p client sends T_readdir with "count" parameter that's sufficiently smaller than client's initially negotiated msize (maximum message size). We perform a check for that though to avoid the server to be interrupted with a "Failed to encode VirtFS reply type 41" transport error message by bad clients. This count value constraint uses msize - 11, because 11 is the header size of R_readdir. Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: <3990d3891e8ae2074709b56449e96ab4b4b93b7d.1579567020.git.qemu_oss@crudebyte.com> [groug: added comment ] Signed-off-by: Greg Kurz --- hw/9pfs/9p.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index c63f549f39..9e046f7acb 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -2434,6 +2434,7 @@ static void coroutine_fn v9fs_readdir(void *opaque) int32_t count; uint32_t max_count; V9fsPDU *pdu = opaque; + V9fsState *s = pdu->s; retval = pdu_unmarshal(pdu, offset, "dqd", &fid, &initial_offset, &max_count); @@ -2442,6 +2443,14 @@ static void coroutine_fn v9fs_readdir(void *opaque) } trace_v9fs_readdir(pdu->tag, pdu->id, fid, initial_offset, max_count); + /* Enough space for a R_readdir header: size[4] Rreaddir tag[2] count[4] */ + if (max_count > s->msize - 11) { + max_count = s->msize - 11; + warn_report_once( + "9p: bad client: T_readdir with count > msize - 11" + ); + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { retval = -EINVAL; From af46a3b233787323417bbca7b1aa3290935a5bdf Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sat, 8 Feb 2020 09:24:19 +0100 Subject: [PATCH 4/6] hw/9pfs/9p-synth: added directory for readdir test This will provide the following virtual files by the 9pfs synth driver: - /ReadDirDir/ReadDirFile99 - /ReadDirDir/ReadDirFile98 ... - /ReadDirDir/ReadDirFile1 - /ReadDirDir/ReadDirFile0 This virtual directory and its virtual 100 files will be used by the upcoming 9pfs readdir tests. Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: <5408c28c8de25dd575b745cef63bf785305ccef2.1579567020.git.qemu_oss@crudebyte.com> Signed-off-by: Greg Kurz --- hw/9pfs/9p-synth.c | 19 +++++++++++++++++++ hw/9pfs/9p-synth.h | 5 +++++ 2 files changed, 24 insertions(+) diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c index 54239c9bbf..7eb210ffa8 100644 --- a/hw/9pfs/9p-synth.c +++ b/hw/9pfs/9p-synth.c @@ -578,6 +578,25 @@ static int synth_init(FsContext *ctx, Error **errp) NULL, v9fs_synth_qtest_flush_write, ctx); assert(!ret); + + /* Directory for READDIR test */ + { + V9fsSynthNode *dir = NULL; + ret = qemu_v9fs_synth_mkdir( + NULL, 0700, QTEST_V9FS_SYNTH_READDIR_DIR, &dir + ); + assert(!ret); + for (i = 0; i < QTEST_V9FS_SYNTH_READDIR_NFILES; ++i) { + char *name = g_strdup_printf( + QTEST_V9FS_SYNTH_READDIR_FILE, i + ); + ret = qemu_v9fs_synth_add_file( + dir, 0, name, NULL, NULL, ctx + ); + assert(!ret); + g_free(name); + } + } } return 0; diff --git a/hw/9pfs/9p-synth.h b/hw/9pfs/9p-synth.h index af7a993a1e..036d7e4a5b 100644 --- a/hw/9pfs/9p-synth.h +++ b/hw/9pfs/9p-synth.h @@ -55,6 +55,11 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode, #define QTEST_V9FS_SYNTH_LOPEN_FILE "LOPEN" #define QTEST_V9FS_SYNTH_WRITE_FILE "WRITE" +/* for READDIR test */ +#define QTEST_V9FS_SYNTH_READDIR_DIR "ReadDirDir" +#define QTEST_V9FS_SYNTH_READDIR_FILE "ReadDirFile%d" +#define QTEST_V9FS_SYNTH_READDIR_NFILES 100 + /* Any write to the "FLUSH" file is handled one byte at a time by the * backend. If the byte is zero, the backend returns success (ie, 1), * otherwise it forces the server to try again forever. Thus allowing From 4829469fd9ff091bc99176512b194b468ed60c1f Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sat, 8 Feb 2020 09:24:19 +0100 Subject: [PATCH 5/6] tests/virtio-9p: added readdir test The first readdir test simply checks the amount of directory entries returned by 9pfs server, according to the created amount of virtual files on 9pfs synth driver side. Then the subsequent readdir test also checks whether all directory entries have the expected file names (as created on 9pfs synth driver side), ignoring their precise order in result list though. Signed-off-by: Christian Schoenebeck Message-Id: Reviewed-by: Greg Kurz Signed-off-by: Greg Kurz --- tests/qtest/virtio-9p-test.c | 152 +++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c index 06263edb53..2167322985 100644 --- a/tests/qtest/virtio-9p-test.c +++ b/tests/qtest/virtio-9p-test.c @@ -68,6 +68,11 @@ static void v9fs_memread(P9Req *req, void *addr, size_t len) req->r_off += len; } +static void v9fs_uint8_read(P9Req *req, uint8_t *val) +{ + v9fs_memread(req, val, 1); +} + static void v9fs_uint16_write(P9Req *req, uint16_t val) { uint16_t le_val = cpu_to_le16(val); @@ -101,6 +106,12 @@ static void v9fs_uint32_read(P9Req *req, uint32_t *val) le32_to_cpus(val); } +static void v9fs_uint64_read(P9Req *req, uint64_t *val) +{ + v9fs_memread(req, val, 8); + le64_to_cpus(val); +} + /* len[2] string[len] */ static uint16_t v9fs_string_size(const char *string) { @@ -191,6 +202,7 @@ static const char *rmessage_name(uint8_t id) id == P9_RLOPEN ? "RLOPEN" : id == P9_RWRITE ? "RWRITE" : id == P9_RFLUSH ? "RFLUSH" : + id == P9_RREADDIR ? "READDIR" : ""; } @@ -348,6 +360,82 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid) v9fs_req_free(req); } +/* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */ +static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t offset, + uint32_t count, uint16_t tag) +{ + P9Req *req; + + req = v9fs_req_init(v9p, 4 + 8 + 4, P9_TREADDIR, tag); + v9fs_uint32_write(req, fid); + v9fs_uint64_write(req, offset); + v9fs_uint32_write(req, count); + v9fs_req_send(req); + return req; +} + +struct V9fsDirent { + v9fs_qid qid; + uint64_t offset; + uint8_t type; + char *name; + struct V9fsDirent *next; +}; + +/* size[4] Rreaddir tag[2] count[4] data[count] */ +static void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t *nentries, + struct V9fsDirent **entries) +{ + uint32_t local_count; + struct V9fsDirent *e = NULL; + uint16_t slen; + uint32_t n = 0; + + v9fs_req_recv(req, P9_RREADDIR); + v9fs_uint32_read(req, &local_count); + + if (count) { + *count = local_count; + } + + for (int32_t togo = (int32_t)local_count; + togo >= 13 + 8 + 1 + 2; + togo -= 13 + 8 + 1 + 2 + slen, ++n) + { + if (!e) { + e = g_malloc(sizeof(struct V9fsDirent)); + if (entries) { + *entries = e; + } + } else { + e = e->next = g_malloc(sizeof(struct V9fsDirent)); + } + e->next = NULL; + /* qid[13] offset[8] type[1] name[s] */ + v9fs_memread(req, &e->qid, 13); + v9fs_uint64_read(req, &e->offset); + v9fs_uint8_read(req, &e->type); + v9fs_string_read(req, &slen, &e->name); + } + + if (nentries) { + *nentries = n; + } + + v9fs_req_free(req); +} + +static void v9fs_free_dirents(struct V9fsDirent *e) +{ + struct V9fsDirent *next = NULL; + + for (; e; e = next) { + next = e->next; + g_free(e->name); + g_free(e); + } +} + /* size[4] Tlopen tag[2] fid[4] flags[4] */ static P9Req *v9fs_tlopen(QVirtio9P *v9p, uint32_t fid, uint32_t flags, uint16_t tag) @@ -480,6 +568,69 @@ static void fs_walk(void *obj, void *data, QGuestAllocator *t_alloc) g_free(wqid); } +static bool fs_dirents_contain_name(struct V9fsDirent *e, const char* name) +{ + for (; e; e = e->next) { + if (!strcmp(e->name, name)) { + return true; + } + } + return false; +} + +static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc) +{ + QVirtio9P *v9p = obj; + alloc = t_alloc; + char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) }; + uint16_t nqid; + v9fs_qid qid; + uint32_t count, nentries; + struct V9fsDirent *entries = NULL; + P9Req *req; + + fs_attach(v9p, NULL, t_alloc); + req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0); + v9fs_req_wait_for_reply(req, NULL); + v9fs_rwalk(req, &nqid, NULL); + g_assert_cmpint(nqid, ==, 1); + + req = v9fs_tlopen(v9p, 1, O_DIRECTORY, 0); + v9fs_req_wait_for_reply(req, NULL); + v9fs_rlopen(req, &qid, NULL); + + /* + * submit count = msize - 11, because 11 is the header size of Rreaddir + */ + req = v9fs_treaddir(v9p, 1, 0, P9_MAX_SIZE - 11, 0); + v9fs_req_wait_for_reply(req, NULL); + v9fs_rreaddir(req, &count, &nentries, &entries); + + /* + * Assuming msize (P9_MAX_SIZE) is large enough so we can retrieve all + * dir entries with only one readdir request. + */ + g_assert_cmpint( + nentries, ==, + QTEST_V9FS_SYNTH_READDIR_NFILES + 2 /* "." and ".." */ + ); + + /* + * Check all file names exist in returned entries, ignore their order + * though. + */ + g_assert_cmpint(fs_dirents_contain_name(entries, "."), ==, true); + g_assert_cmpint(fs_dirents_contain_name(entries, ".."), ==, true); + for (int i = 0; i < QTEST_V9FS_SYNTH_READDIR_NFILES; ++i) { + char *name = g_strdup_printf(QTEST_V9FS_SYNTH_READDIR_FILE, i); + g_assert_cmpint(fs_dirents_contain_name(entries, name), ==, true); + g_free(name); + } + + v9fs_free_dirents(entries); + g_free(wnames[0]); +} + static void fs_walk_no_slash(void *obj, void *data, QGuestAllocator *t_alloc) { QVirtio9P *v9p = obj; @@ -658,6 +809,7 @@ static void register_virtio_9p_test(void) NULL); qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored, NULL); + qos_add_test("fs/readdir/basic", "virtio-9p", fs_readdir, NULL); } libqos_init(register_virtio_9p_test); From 2822602cbe2be98229b882101dfdb9d3a738c611 Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sat, 8 Feb 2020 09:24:19 +0100 Subject: [PATCH 6/6] MAINTAINERS: 9pfs: Add myself as reviewer Currently 9pfs is only taken care of by Greg. Since I am actively working on 9pfs and already became quite used to the code base, it makes sense to volunteer as reviewer for 9pfs related patches. Signed-off-by: Christian Schoenebeck Cc: Greg Kurz Message-Id: Signed-off-by: Greg Kurz --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index e72b5e5f69..ce46c0a552 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1578,6 +1578,7 @@ F: include/hw/virtio/ virtio-9p M: Greg Kurz +R: Christian Schoenebeck S: Odd Fixes F: hw/9pfs/ X: hw/9pfs/xen-9p*