Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Greg Kurz <groug@kaod.org>]

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

On Thu, 23 Feb 2017 13:44:41 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, Feb 20, 2017 at 03:40:15PM +0100, Greg Kurz wrote:
> > +static ssize_t do_xattrat_op(int op_type, int dirfd, const char *path,
> > +                             const char *name, void *value, size_t size,
> > +                             int flags)
> > +{
> > +    struct xattrat_data *data;
> > +    pid_t pid;
> > +    ssize_t ret = -1;
> > +    int wstatus;
> > +
> > +    data = mmap(NULL, sizeof(*data) + size, PROT_READ | PROT_WRITE,
> > +                MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> > +    if (data == MAP_FAILED) {
> > +        return -1;
> > +    }
> > +    data->ret = -1;
> > +
> > +    pid = fork();
> > +    if (pid < 0) {
> > +        goto err_out;
> > +    } else if (pid == 0) {
> > +        if (fchdir(dirfd) == 0) {
> > +            switch (op_type) {
> > +            case XATTRAT_OP_GET:
> > +                data->ret = lgetxattr(path, name, data->value, size);
> > +                break;
> > +            case XATTRAT_OP_LIST:
> > +                data->ret = llistxattr(path, data->value, size);
> > +                break;
> > +            case XATTRAT_OP_SET:
> > +                data->ret = lsetxattr(path, name, value, size, flags);
> > +                break;
> > +            case XATTRAT_OP_REMOVE:
> > +                data->ret = lremovexattr(path, name);
> > +                break;
> > +            default:
> > +                g_assert_not_reached();
> > +            }
> > +        }
> > +        data->serrno = errno;
> > +        _exit(0);
> > +    }
> > +    assert(waitpid(pid, &wstatus, 0) == pid && WIFEXITED(wstatus));
> > +
> > +    ret = data->ret;
> > +    if (ret < 0) {
> > +        errno = data->serrno;
> > +        goto err_out;
> > +    }
> > +    if (value) {
> > +        memcpy(value, data->value, data->ret);
> > +    }
> > +err_out:
> > +    munmap_preserver_errno(data, sizeof(*data) + size);
> > +    return ret;
> > +}  
> 
> Forking is ugly since QEMU is a multi-threaded program.  We brainstormed

Yeah, forking is ugly and it completely ruins metadata performance (x30
slower in passthrough mode and x300 slower in mapped-xattr mode).

> alternatives on IRC like using /proc/self/fd/$fd to work around the
> missing getxattrat() API.
> 

This should do the trick indeed. If we have to call getxattr()
on some untrusted $path that may be modified by the guest. We
can do:

dirfd = openat_nofollow($mount_fd, dirname($path))
filename = basename($path)

and then we can safely call:

lgetxattr("/proc/self/fd/$dirfd/$filename")

since "/proc/self/fd/$dirfd" is trusted.

> Stefan


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