Re: [Qemu-devel] [for-2.10 PATCH v3] 9pfs: local: fix fchmodat_nofollow() limitations

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

On Wed, 9 Aug 2017 11:19:42 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 08/09/2017 11:00 AM, Greg Kurz wrote:
> > This function has to ensure it doesn't follow a symlink that could be used
> > to escape the virtfs directory. This could be easily achieved if fchmodat()
> > on linux honored the AT_SYMLINK_NOFOLLOW flag as described in POSIX, but
> > it doesn't. There was a tentative to implement a new fchmodat2() syscall
> > with the correct semantics:
> > 
> > https://patchwork.kernel.org/patch/9596301/
> > 
> > but it didn't gain much momentum. Also it was suggested to look at a O_PATH  
> 
> s/a O_PATH/an O_PATH/
> 

Fixed.

> > based solution in the first place.
> > 
> > The current implementation covers most use-cases, but it notably fails if:
> > - the target path has access rights equal to 0000 (openat() returns EPERM),  
> >   => once you've done chmod(0000) on a file, you can never chmod() again  
> > - the target path is UNIX domain socket (openat() returns ENXIO)  
> >   => bind() of UNIX domain sockets fails if the file is on 9pfs  
> > 
> > The solution is to use O_PATH: openat() now succeeds in both cases, and we
> > can ensure the path isn't a symlink with fstat(). The associated entry in
> > "/proc/self/fd" can hence be safely passed to the regular chmod() syscall.  
> 
> My late-breaking question from v2 remains: fstat() on O_PATH only works

Yeah I saw your mail just after sending the v3 :)

> in kernel 3.6 and newer; are we worried about kernels in the window of
> 2.6.39 (when O_PATH was introduced) and 3.5?  Or at this point, are we
> reasonably sure that platforms are either too old for O_PATH at all
> (Hello RHEL 6, with 2.6.32), or else new enough that we aren't going to
> have spurious failures due to fstat() not doing what we want?
> 
> I don't actually know the failure mode of fstat() on kernel 3.5, so if
> someone cares about that working (presumably because they are on a
> platform with such a kernel), please speak up. (Or even run my test
> program included on the v1 thread, to show us what happens)
> 

That seems reasonable to me.

> > +    fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0);
> > +#ifndef O_PATH  
> 
> Please make this '#if O_PATH' or even '#if O_PATH_9P_UTIL'; as it might
> be feasible for someone to
> 
> #ifndef O_PATH
> #define O_PATH 0
> #endif
> 
> where the macro is defined but the feature is not present, messing up
> our code if we only check for a definition.
> 

Ok, I'll do that.

> > +#else
> > +    /* Now we handle racing symlinks. */
> > +    ret = fstat(fd, &stbuf);
> > +    if (ret) {
> > +        goto out;  
> 
> This may leave errno at an unusual value for fchmodat(), if we are on
> kernel 3.5.  But until someone speaks up that it matters, I'm okay
> saving any cleanup work in that area for a followup patch.
> 

Agreed.

> > +    }
> > +    if (S_ISLNK(stbuf.st_mode)) {
> > +        errno = ELOOP;
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    {
> > +        char *proc_path = g_strdup_printf("/proc/self/fd/%d", fd);
> > +        ret = chmod(proc_path, mode);
> > +        g_free(proc_path);
> > +    }
> > +#endif
> > +out:  
> 
> Swap these two lines - your only use of 'goto out' are under the O_PATH
> branch, and therefore you get a compilation failure about unused label
> on older glibc.
> 

Oops.

> With the #if condition fixed and the scope of the #endif fixed,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks !

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]