Concurrent legacy Tread requests (9p2000.u) on the same directory FID
race the FID's shared dirent stream.
In the initial Tread implementation (a92315553d) the dirent stream
was entirely unprotected. Commit 7cde47d4a8 introduced a lock, which
mitigated the problem, but without sufficient full coverage for this
request type, as it only held the lock during individual dirent
stream steps. A concurrent request could mutate the dirent stream in
between. Furthermore the initial rewinddir() (to offset 0) was still
not protected at all.
This could cause corrupted directory entries being returned to
client, with entries missing, duplicate entries and wrong order, up
to heap corruption of 9p server's QEMU process due to garbage seek
targets.
Fix this by holding the lock for the entire directory reading
transaction (i.e. for the whole v9fs_do_readdir_with_stat() call).
Fixes: a92315553d ("virtio-9p: Add P9_TREAD support")
Signed-off-by: Christian Schoenebeck <[email protected]>
---
hw/9pfs/9p.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4764d9db13..730d1279bb 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2421,6 +2421,10 @@ v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState
*fidp,
return offset;
}
+/*
+ * The only caller of this function (v9fs_read()) holds the per-FID readdir
+ * lock during the whole call of this function.
+ */
static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
V9fsFidState *fidp,
uint32_t max_count)
@@ -2431,6 +2435,7 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU
*pdu,
int32_t count = 0;
struct stat stbuf;
off_t saved_dir_pos;
+ off_t current_dir_pos;
struct dirent *dent;
/* save the directory position */
@@ -2442,12 +2447,11 @@ static int coroutine_fn
v9fs_do_readdir_with_stat(V9fsPDU *pdu,
while (1) {
v9fs_path_init(&path);
- v9fs_readdir_lock(&fidp->fs.dir);
-
err = v9fs_co_readdir(pdu, fidp, &dent);
if (err || !dent) {
break;
}
+ current_dir_pos = qemu_dirent_off(dent);
err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
if (err < 0) {
break;
@@ -2461,8 +2465,6 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU
*pdu,
break;
}
if ((count + v9stat.size + 2) > max_count) {
- v9fs_readdir_unlock(&fidp->fs.dir);
-
/* Ran out of buffer. Set dir back to old position and return */
v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
v9fs_stat_free(&v9stat);
@@ -2473,8 +2475,6 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU
*pdu,
/* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
len = pdu_marshal(pdu, 11 + count, "S", &v9stat);
- v9fs_readdir_unlock(&fidp->fs.dir);
-
if (len < 0) {
v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
v9fs_stat_free(&v9stat);
@@ -2484,11 +2484,9 @@ static int coroutine_fn
v9fs_do_readdir_with_stat(V9fsPDU *pdu,
count += len;
v9fs_stat_free(&v9stat);
v9fs_path_free(&path);
- saved_dir_pos = qemu_dirent_off(dent);
+ saved_dir_pos = current_dir_pos;
}
- v9fs_readdir_unlock(&fidp->fs.dir);
-
v9fs_path_free(&path);
if (err < 0) {
return err;
@@ -2528,10 +2526,17 @@ static void coroutine_fn v9fs_read(void *opaque)
err = -EOPNOTSUPP;
goto out;
}
+ /*
+ * Hold the lock for the entire readdir transaction (rewind and
+ * getdents) to prevent dirent stream corruption on concurrent requests
+ * on the same FID.
+ */
+ v9fs_readdir_lock(&fidp->fs.dir);
if (off == 0) {
v9fs_co_rewinddir(pdu, fidp);
}
count = v9fs_do_readdir_with_stat(pdu, fidp, max_count);
+ v9fs_readdir_unlock(&fidp->fs.dir);
if (count < 0) {
err = count;
goto out;
--
2.47.3
