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

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

On Freitag, 30. August 2019 13:48:27 CEST Greg Kurz wrote:
> > > 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 /.

That's actually what the suggested fix already did in v5 here (see diff above). 
However I was worried whether I thought about all edge cases. So I also need 
some more testing and hence clearly decided to postpone this fix for now.

Best regards,
Christian Schoenebeck