This patch mitigates issues with very large absolute paths.
- Add error handling to all v9fs_path_sprintf() calls in
local_name_to_path()
- Update callers of v9fs_fix_path() to check return values.
- When path formatting fails, clunk the affected FIDs to prevent use of
invalid paths.
- Use g_autofree for temporary variables to simplify code.
Even though paths are usually limited to PATH_MAX (typically 4k) on guest,
this limitation can be circumvented by using *at() functions on guest and
creating very deep directory structures. This was a problem for QEMU 9p
server, as it currently tracks the absolute path for each FID internally
that always requires assembly of a (potentially ver large) absolute path.
A true long-term fix would be getting rid of storing an absolute path for
each FID internally. However that would likely be a massive change with
uncertain implications.
This patch therefore just mitigates the problem by immediately clunking
(i.e. closing) all FIDs whose path exceed a limit that we could handle.
As this only accounts to very unusual large absolute paths not ever been
reported on (sane) production machines, this is currently considered an
acceptable mitigation that should only (counter)affect malicious attempts.
Fixes: 2f008a8c97e2 ("hw/9pfs: Use the correct signed type ...")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3358
Signed-off-by: Christian Schoenebeck <[email protected]>
---
hw/9pfs/9p-local.c | 23 ++++++++++++++++-------
hw/9pfs/9p.c | 18 +++++++++++++-----
2 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 24cb1da90a..aa48306b0e 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1261,26 +1261,35 @@ static int local_name_to_path(FsContext *ctx, V9fsPath
*dir_path,
} else if (!strcmp(name, "..")) {
if (!strcmp(dir_path->data, ".")) {
/* ".." relative to the root is "." */
- v9fs_path_sprintf(target, ".");
+ if (v9fs_path_sprintf(target, ".") < 0) {
+ return -1;
+ }
} else {
- char *tmp = g_path_get_dirname(dir_path->data);
+ g_autofree char *tmp = g_path_get_dirname(dir_path->data);
/* Symbolic links are resolved by the client. We can assume
* that ".." relative to "foo/bar" is equivalent to "foo"
*/
- v9fs_path_sprintf(target, "%s", tmp);
- g_free(tmp);
+ if (v9fs_path_sprintf(target, "%s", tmp) < 0) {
+ return -1;
+ }
}
} else {
assert(!strchr(name, '/'));
- v9fs_path_sprintf(target, "%s/%s", dir_path->data, name);
+ if (v9fs_path_sprintf(target, "%s/%s", dir_path->data, name) < 0) {
+ return -1;
+ }
}
} else if (!strcmp(name, "/") || !strcmp(name, ".") ||
!strcmp(name, "..")) {
/* This is the root fid */
- v9fs_path_sprintf(target, ".");
+ if (v9fs_path_sprintf(target, ".") < 0) {
+ return -1;
+ }
} else {
assert(!strchr(name, '/'));
- v9fs_path_sprintf(target, "./%s", name);
+ if (v9fs_path_sprintf(target, "./%s", name) < 0) {
+ return -1;
+ }
}
return 0;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d704de644f..b4314d2549 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3325,12 +3325,14 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU
*pdu, V9fsFidState *fidp,
goto out;
}
} else {
- char *dir_name = g_path_get_dirname(fidp->path.data);
+ g_autofree char *dir_name = g_path_get_dirname(fidp->path.data);
V9fsPath dir_path;
v9fs_path_init(&dir_path);
- v9fs_path_sprintf(&dir_path, "%s", dir_name);
- g_free(dir_name);
+ err = v9fs_path_sprintf(&dir_path, "%s", dir_name);
+ if (err < 0) {
+ goto out;
+ }
err = v9fs_co_name_to_path(pdu, &dir_path, name->data, &new_path);
v9fs_path_free(&dir_path);
@@ -3351,7 +3353,10 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU
*pdu, V9fsFidState *fidp,
while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &tfidp)) {
if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) {
/* replace the name */
- v9fs_fix_path(&tfidp->path, &new_path, strlen(fidp->path.data));
+ if (v9fs_fix_path(&tfidp->path, &new_path,
+ strlen(fidp->path.data)) < 0) {
+ clunk_fid(s, tfidp->fid);
+ }
}
}
out:
@@ -3448,7 +3453,10 @@ static int coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu,
V9fsPath *olddir,
while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &tfidp)) {
if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) {
/* replace the name */
- v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data));
+ if (v9fs_fix_path(&tfidp->path, &newpath,
+ strlen(oldpath.data)) < 0) {
+ clunk_fid(s, tfidp->fid);
+ }
}
}
out:
--
2.47.3
