Re: [Qemu-devel] [PATCH -V3 5/7] virtio-9p: Implemented security model for symlink and link.

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

On 05/21/2010 04:26 PM, Venkateswararao Jujjuri (JV) wrote:
> Signed-off-by: Venkateswararao Jujjuri<jvrao@linux.vnet.ibm.com>
> ---
>   hw/file-op-9p.h      |    4 +-
>   hw/virtio-9p-local.c |   87 +++++++++++++++++++++++++++++++++++++++----------
>   hw/virtio-9p.c       |   24 ++++++++++----
>   3 files changed, 88 insertions(+), 27 deletions(-)
>
> diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
> index 752fbad..0a9c10a 100644
> --- a/hw/file-op-9p.h
> +++ b/hw/file-op-9p.h
> @@ -54,8 +54,8 @@ typedef struct FileOperations
>       int (*mknod)(FsContext *, const char *, FsCred *);
>       int (*utime)(FsContext *, const char *, const struct utimbuf *);
>       int (*remove)(FsContext *, const char *);
> -    int (*symlink)(FsContext *, const char *, const char *);
> -    int (*link)(FsContext *, const char *, const char *);
> +    int (*symlink)(FsContext *, const char *, const char *, FsCred *);
> +    int (*link)(FsContext *, const char *, const char *, FsCred *);
>       int (*setuid)(FsContext *, uid_t);
>       int (*close)(FsContext *, int);
>       int (*closedir)(FsContext *, DIR *);
> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
> index bfcd695..ca0f065 100644
> --- a/hw/virtio-9p-local.c
> +++ b/hw/virtio-9p-local.c
> @@ -64,12 +64,25 @@ static int local_set_xattr(const char *path, FsCred *credp)
>           }
>       }
>        return 0;
> - }
> +}
>
> -static ssize_t local_readlink(FsContext *ctx, const char *path,
> -                                char *buf, size_t bufsz)
> +static ssize_t local_readlink(FsContext *fs_ctx, const char *path,
> +        char *buf, size_t bufsz)
>   {
> -    return readlink(rpath(ctx, path), buf, bufsz);
> +    ssize_t tsize = -1;
> +    if (fs_ctx->fs_sm == sm_mapped) {
> +        int fd;
> +        fd = open(rpath(fs_ctx, path), O_RDONLY);
> +        if (fd == -1) {
> +            return -1;
> +        }
> +        tsize = read(fd, (void *)buf, bufsz);
> +        close(fd);
> +        return tsize;
> +    } else if (fs_ctx->fs_sm == sm_passthrough) {
> +        tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
> +    }
> +    return tsize;
>   }
>
>   static int local_close(FsContext *ctx, int fd)
> @@ -247,32 +260,70 @@ err_end:
>       return err;
>   }
>
> -static int local_symlink(FsContext *ctx, const char *oldpath,
> -                            const char *newpath)
> +
> +static int local_symlink(FsContext *fs_ctx, const char *oldpath,
> +        const char *newpath, FsCred *credp)
>   {
> -    return symlink(oldpath, rpath(ctx, newpath));
> +    int err = -1;
> +    /* Determine the security model */
> +    if (fs_ctx->fs_sm == sm_mapped) {
> +        int fd;
> +        ssize_t oldpath_size, write_size;
> +        fd = open(rpath(fs_ctx, newpath), O_CREAT|O_EXCL|O_RDWR,
> +                SM_LOCAL_MODE_BITS);
> +        if (fd == -1) {
> +            return fd;
> +        }
> +        /* Write the oldpath (target) to the file. */
> +        oldpath_size = strlen(oldpath) + 1;
> +        write_size = write(fd, (void *)oldpath, oldpath_size);
>    

Signals are pretty frequent in qemu so not handling EINTR is asking for 
trouble.

Regards,

Anthony Liguori

> +        if (write_size != oldpath_size) {
> +            close(fd);
> +            err = -1;
> +            goto err_end;
> +        }
> +        close(fd);
> +        /* Set cleint credentials in symlink's xattr */
> +        credp->fc_mode = credp->fc_mode|S_IFLNK;
> +        err = local_set_xattr(rpath(fs_ctx, newpath), credp);
> +        if (err == -1) {
> +            goto err_end;
> +        }
> +    } else if (fs_ctx->fs_sm == sm_passthrough) {
> +        err = symlink(oldpath, rpath(fs_ctx, newpath));
> +        if (err) {
> +            return err;
> +        }
> +        err = chmod(rpath(fs_ctx, newpath), credp->fc_mode&  07777);
> +        if (err == -1) {
> +            goto err_end;
> +        }
> +        err = chown(rpath(fs_ctx, newpath), credp->fc_uid, credp->fc_gid);
> +        if (err == -1) {
> +            goto err_end;
> +        }
> +    }
> +    return err;
> +
> +err_end:
> +    remove(rpath(fs_ctx, newpath));
> +    return err;
>   }
>
> -static int local_link(FsContext *ctx, const char *oldpath, const char *newpath)
> +static int local_link(FsContext *fs_ctx, const char *oldpath,
> +        const char *newpath, FsCred *credp)
>   {
> -    char *tmp = qemu_strdup(rpath(ctx, oldpath));
> -    int err, serrno = 0;
> +    char *tmp = qemu_strdup(rpath(fs_ctx, oldpath));
> +    int err;
>
>       if (tmp == NULL) {
>           return -ENOMEM;
>       }
>
> -    err = link(tmp, rpath(ctx, newpath));
> -    if (err == -1) {
> -        serrno = errno;
> -    }
> +    err = link(tmp, rpath(fs_ctx, newpath));
>
>       qemu_free(tmp);
>
> -    if (err == -1) {
> -        errno = serrno;
> -    }
> -
>       return err;
>   }
>
> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> index 30f649d..fbc846b 100644
> --- a/hw/virtio-9p.c
> +++ b/hw/virtio-9p.c
> @@ -197,15 +197,25 @@ static int v9fs_do_open2(V9fsState *s, V9fsCreateState *vs)
>       return s->ops->open2(&s->ctx, vs->fullname.data, flags,&cred);
>   }
>
> -static int v9fs_do_symlink(V9fsState *s, V9fsString *oldpath,
> -                            V9fsString *newpath)
> +static int v9fs_do_symlink(V9fsState *s, V9fsCreateState *vs)
>   {
> -    return s->ops->symlink(&s->ctx, oldpath->data, newpath->data);
> +    FsCred cred;
> +    cred_init(&cred);
> +    cred.fc_uid = vs->fidp->uid;
> +    cred.fc_mode = vs->perm | 0777;
> +
> +    return s->ops->symlink(&s->ctx, vs->extension.data, vs->fullname.data,
> +&cred);
>   }
>
> -static int v9fs_do_link(V9fsState *s, V9fsString *oldpath, V9fsString *newpath)
> +static int v9fs_do_link(V9fsState *s, V9fsFidState *nfidp, V9fsCreateState *vs)
>   {
> -    return s->ops->link(&s->ctx, oldpath->data, newpath->data);
> +    FsCred cred;
> +    cred_init(&cred);
> +    cred.fc_uid = nfidp->uid;
> +    cred.fc_mode = vs->perm&  0777;
> +
> +    return s->ops->link(&s->ctx, nfidp->path.data, vs->fullname.data,&cred);
>   }
>
>   static int v9fs_do_truncate(V9fsState *s, V9fsString *path, off_t size)
> @@ -1782,7 +1792,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err)
>           err = v9fs_do_mkdir(s, vs);
>           v9fs_create_post_mkdir(s, vs, err);
>       } else if (vs->perm&  P9_STAT_MODE_SYMLINK) {
> -        err = v9fs_do_symlink(s,&vs->extension,&vs->fullname);
> +        err = v9fs_do_symlink(s, vs);
>           v9fs_create_post_perms(s, vs, err);
>       } else if (vs->perm&  P9_STAT_MODE_LINK) {
>           int32_t nfid = atoi(vs->extension.data);
> @@ -1791,7 +1801,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err)
>               err = -errno;
>               v9fs_post_create(s, vs, err);
>           }
> -        err = v9fs_do_link(s,&nfidp->path,&vs->fullname);
> +        err = v9fs_do_link(s, nfidp, vs);
>           v9fs_create_post_perms(s, vs, err);
>       } else if (vs->perm&  P9_STAT_MODE_DEVICE) {
>           char ctype;
>