[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 3/5] 9p: Added virtfs option 'remap_inodes'
From: |
Christian Schoenebeck |
Subject: |
Re: [Qemu-devel] [PATCH v5 3/5] 9p: Added virtfs option 'remap_inodes' |
Date: |
Sat, 06 Jul 2019 12:22:27 +0200 |
On Mittwoch, 3. Juli 2019 13:13:26 CEST Christian Schoenebeck wrote:
> To support multiple devices on the 9p share, and avoid
> qid path collisions we take the device id as input
[snip]
> - Fixed v9fs_do_readdir() having exposed info outside
> export root with '..' entry.
[snip]
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 8cc65c2c67..39c6c2a894 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
[snip]
> @@ -1940,6 +2041,19 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu,
> V9fsFidState *fidp, int32_t count = 0;
> off_t saved_dir_pos;
> struct dirent *dent;
> + struct stat stbuf;
> + bool fidIsExportRoot;
> +
> + /*
> + * determine if fidp is the export root, which is required for safe
> + * handling of ".." below
> + */
> + err = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> + if (err < 0) {
> + return err;
> + }
> + fidIsExportRoot = pdu->s->dev_id == stbuf.st_dev &&
> + pdu->s->root_ino == stbuf.st_ino;
>
> /* save the directory position */
> saved_dir_pos = v9fs_co_telldir(pdu, fidp);
> @@ -1964,16 +2078,51 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> *pdu, V9fsFidState *fidp, v9fs_string_free(&name);
> return count;
> }
> - /*
> - * Fill up just the path field of qid because the client uses
> - * only that. To fill the entire qid structure we will have
> - * to stat each dirent found, which is expensive
> - */
> - size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> - memcpy(&qid.path, &dent->d_ino, size);
> - /* Fill the other fields with dummy values */
> - qid.type = 0;
> - qid.version = 0;
> +
> + if (fidIsExportRoot && !strcmp("..", dent->d_name)) {
> + /*
> + * if "." is export root, then return qid of export root for
> + * ".." to avoid exposing anything outside the export
> + */
> + err = fid_to_qid(pdu, fidp, &qid);
> + if (err < 0) {
> + v9fs_readdir_unlock(&fidp->fs.dir);
> + v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> + v9fs_string_free(&name);
> + return err;
> + }
Hmm, I start to wonder whether I should postpone that particular bug fix and
not make it part of that QID fix patch series (not even as separate patch
there). Because that fix needs some more adjustments. E.g. I should adjust
dent->d_type here as well; but more notably it should also distinguish between
the case where the export root is mounted as / on guest or not and that's
where this fix could become ugly and grow in size.
To make the case clear: calling on guest
readdir(pathOfSome9pExportRootOnGuest);
currently always returns for its ".." result entry the inode number and d_type
of the export root's parent directory on host, so it exposes information of
host outside the 9p export.
I don't see that as security issue, since the information revealed is limited
to the inode number and d_type, but it is definitely incorrect behaviour.
Best regards,
Christian Schoenebeck
- [Qemu-devel] [PATCH v5 0/5] 9p: Fix file ID collisions, Christian Schoenebeck, 2019/07/03
- [Qemu-devel] [PATCH v5 4/5] 9p: stat_to_qid: implement slow path, Christian Schoenebeck, 2019/07/03
- [Qemu-devel] [PATCH v5 1/5] 9p: unsigned type for type, version, path, Christian Schoenebeck, 2019/07/03
- [Qemu-devel] [PATCH v5 3/5] 9p: Added virtfs option 'remap_inodes', Christian Schoenebeck, 2019/07/03
- Re: [Qemu-devel] [PATCH v5 3/5] 9p: Added virtfs option 'remap_inodes',
Christian Schoenebeck <=
- [Qemu-devel] [PATCH v5 2/5] 9p: Treat multiple devices on one export as an error, Christian Schoenebeck, 2019/07/03
- [Qemu-devel] [PATCH v5 5/5] 9p: Use variable length suffixes for inode remapping, Christian Schoenebeck, 2019/07/03
- Re: [Qemu-devel] [PATCH v5 0/5] 9p: Fix file ID collisions, no-reply, 2019/07/03
- Re: [Qemu-devel] [PATCH v5 0/5] 9p: Fix file ID collisions, no-reply, 2019/07/03