@samitouri / QOSamiQemu / commits / 3802c0e755

hw/9pfs: let callers of v9fs_path_sprintf() and v9fs_fix_path() handle errors

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 ...") Reported-by: Wang Jihe <wangjihe.mail@gmail.com> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3358 Link: https://lore.kernel.org/qemu-devel/1d11dcbfc95b811dcdb48c6d7f3894d0ebd073a2.1779126034.git.qemu_oss@crudebyte.com Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Christian Schoenebeck committed May 18, 2026 at 19:35 UTC 3802c0e755a53b126e717415b54226a468bf7ddf
2 files changed +29 -12
hw/9pfs/9p-local.c
+16 -7
@@ -1261,26 +1261,35 @@ static int local_name_to_path(FsContext *ctx, V9fsPath *dir_path,
1261 } else if (!strcmp(name, "..")) {
1262 if (!strcmp(dir_path->data, ".")) {
1263 /* ".." relative to the root is "." */
1264 - v9fs_path_sprintf(target, ".");
1264 + if (v9fs_path_sprintf(target, ".") < 0) {
1265 + return -1;
1266 + }
1267 } else {
1266 - char *tmp = g_path_get_dirname(dir_path->data);
1268 + g_autofree char *tmp = g_path_get_dirname(dir_path->data);
1269 /* Symbolic links are resolved by the client. We can assume
1270 * that ".." relative to "foo/bar" is equivalent to "foo"
1271 */
1270 - v9fs_path_sprintf(target, "%s", tmp);
1271 - g_free(tmp);
1272 + if (v9fs_path_sprintf(target, "%s", tmp) < 0) {
1273 + return -1;
1274 + }
1275 }
1276 } else {
1277 assert(!strchr(name, '/'));
1275 - v9fs_path_sprintf(target, "%s/%s", dir_path->data, name);
1278 + if (v9fs_path_sprintf(target, "%s/%s", dir_path->data, name) < 0) {
1279 + return -1;
1280 + }
1281 }
1282 } else if (!strcmp(name, "/") || !strcmp(name, ".") ||
1283 !strcmp(name, "..")) {
1284 /* This is the root fid */
1280 - v9fs_path_sprintf(target, ".");
1285 + if (v9fs_path_sprintf(target, ".") < 0) {
1286 + return -1;
1287 + }
1288 } else {
1289 assert(!strchr(name, '/'));
1283 - v9fs_path_sprintf(target, "./%s", name);
1290 + if (v9fs_path_sprintf(target, "./%s", name) < 0) {
1291 + return -1;
1292 + }
1293 }
1294 return 0;
1295 }
hw/9pfs/9p.c
+13 -5
@@ -3325,12 +3325,14 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp,
3325 goto out;
3326 }
3327 } else {
3328 - char *dir_name = g_path_get_dirname(fidp->path.data);
3328 + g_autofree char *dir_name = g_path_get_dirname(fidp->path.data);
3329 V9fsPath dir_path;
3330
3331 v9fs_path_init(&dir_path);
3332 - v9fs_path_sprintf(&dir_path, "%s", dir_name);
3333 - g_free(dir_name);
3332 + err = v9fs_path_sprintf(&dir_path, "%s", dir_name);
3333 + if (err < 0) {
3334 + goto out;
3335 + }
3336
3337 err = v9fs_co_name_to_path(pdu, &dir_path, name->data, &new_path);
3338 v9fs_path_free(&dir_path);
@@ -3351,7 +3353,10 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp,
3353 while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &tfidp)) {
3354 if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) {
3355 /* replace the name */
3354 - v9fs_fix_path(&tfidp->path, &new_path, strlen(fidp->path.data));
3356 + if (v9fs_fix_path(&tfidp->path, &new_path,
3357 + strlen(fidp->path.data)) < 0) {
3358 + clunk_fid(s, tfidp->fid);
3359 + }
3360 }
3361 }
3362 out:
@@ -3448,7 +3453,10 @@ static int coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir,
3453 while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &tfidp)) {
3454 if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) {
3455 /* replace the name */
3451 - v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data));
3456 + if (v9fs_fix_path(&tfidp->path, &newpath,
3457 + strlen(oldpath.data)) < 0) {
3458 + clunk_fid(s, tfidp->fid);
3459 + }
3460 }
3461 }
3462 out: