Re: [Qemu-devel] [PATCH v5 3/5] 9p: Added virtfs option 'remap_inodes'

[Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>]

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