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

[Greg Kurz]

On Sat, 06 Jul 2019 12:22:27 +0200
Christian Schoenebeck via Qemu-devel <address@hidden> wrote:

> 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.
> 

Definitely. This should be fixed independently of this series. Maybe follow
the same approach as commit 56f101ecce0e "9pfs: handle walk of ".." in the
root directory", ie. basically make /.. an alias of /.

> Best regards,
> Christian Schoenebeck
>