Re: [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string

[Greg Kurz]

On Sun, 28 Aug 2016 19:21:25 +0200
Greg Kurz <address@hidden> wrote:

> On Fri, 26 Aug 2016 14:00:37 -0500
> Eric Blake <address@hidden> wrote:
> 
> > On 08/26/2016 10:07 AM, Greg Kurz wrote:  
> > > A buggy guest using the 9p2000.u protocol can issue a create request and
> > > pass an empty string as the extension argument. This causes QEMU to crash
> > > in the case of a hard link or a special file, and leads to undefined
> > > behavior, depending on the backend, in the case of a symbolic link.
> > > 
> > > This patch causes the request to fail with EINVAL in these scenarios.
> > > 
> > > Signed-off-by: Greg Kurz <address@hidden>
> > > ---  
> 
> Wait... empty strings coming from pdu_unmarshal() never have data == NULL
> so this whole patch is pointless :) and BTW, only the symlink case is about
> file names.
> 
> > >  hw/9pfs/9p.c |   21 +++++++++++++++++++--
> > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 7b1dfe4e47cb..dc65c3125006 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque)
> > >          }
> > >          fidp->fid_type = P9_FID_DIR;
> > >      } else if (perm & P9_STAT_MODE_SYMLINK) {
> > > +        if (extension.data == NULL) {
> > > +            err = -EINVAL;
> > > +            goto out;
> > > +        }    
> >  
> 
> I realize that this part belongs to patch 1 actually: it is the 
> implementation of
> symbolic links that comes with 9P2000.u (different from the TSYMLINK request 
> in
> 9P2000.L). In which case, the hunk would have been:
> 
> +        if (name_is_illegal(extension.data)) {
> +            err = -EINVAL;
> +            goto out;
> +        }
> 
> > POSIX specifically requires implementations to support creating a
> > symlink whose target is the empty string.  Linux doesn't [yet] permit
> > it, but BSD does.  On systems where creating such a symlink is legal,
> > POSIX requires that such a symlink either be treated as "." if
> > dereferenced, or be treated as ENOENT on attempt to dereference.  But
> > since such links can be created, readlink() should be able to read them
> > without error.
> > 
> > I would argue that we should NOT forbid empty symlinks on creation (but
> > pass back any error from the underlying host OS); but instead check that
> > dereferencing such a symlink behaves sanely if it was created.
> > Meanwhile, a client should not be relying on the behavior (since Linux
> > disobeys POSIX, portable clients should already be avoiding empty symlinks).
> > 
> > http://austingroupbugs.net/view.php?id=649
> >   
> 
> Indeed, maybe we should let the backend decide if it allows symlink with
> an empty target, since the target name is simply "stored" somewhere and
> not used to create a new path. In which case, we should do the same with
> v9fs_symlink(). And we would have two exceptions to the name_is_illegal()
> helper, because we still want to avoid '/' in file names...
> 

Thinking again... I guess '/' in names should result in ENOENT since it seems
to be a common way to say something is wrong with a path... or we should have
separate error paths between the '/'-in-names and the empty name cases.

> On the other hand, we only support linux hosts where the call to symlink()
> will fail with ENOENT and guests using the official linux kernel 9p client
> never send an empty target...
> 
> For the sake of simplicity, I'd rather have the target names to follow the
> same rules as other file names, and return ENOENT directly (the link you
> provide states it is a valid option).
> 
> Peter,
> 
> Since you suggested to do explicit error checking on empty file names, do
> you have an opinion on the case of symlinks with an empty target ?
> 
> > > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque)
> > >          }
> > >          v9fs_path_copy(&fidp->path, &path);
> > >      } else if (perm & P9_STAT_MODE_LINK) {
> > > -        int32_t ofid = atoi(extension.data);
> > > -        V9fsFidState *ofidp = get_fid(pdu, ofid);
> > > +        V9fsFidState *ofidp;
> > > +
> > > +        if (extension.data == NULL) {
> > > +            err = -EINVAL;
> > > +            goto out;
> > > +        }    
> > 
> > Rejecting an empty destination on hard link or device creation, however,
> > is indeed appropriate.
> >   
> 
> In the case of hard links, extension.data is a FID, not a file name.
> 
> In the case of device creation, extension.data is "type major minor", not
> a file name again.

pgpN5ZuWGF7sy.pgp
Description: OpenPGP digital signature