| Message ID | 1274916106-25616-4-git-send-email-jvrao@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 26, 2010 at 04:21:42PM -0700, Venkateswararao Jujjuri (JV) wrote: > Add required infrastructure and modify create/open2 and mkdir per the new > security model. > > Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com> > --- > hw/file-op-9p.h | 23 +++++++- > hw/virtio-9p-local.c | 149 ++++++++++++++++++++++++++++++++++++-------------- > hw/virtio-9p.c | 42 ++++++++++---- > 3 files changed, 158 insertions(+), 56 deletions(-) > > diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h > index 2934ff1..73d59b2 100644 > --- a/hw/file-op-9p.h > +++ b/hw/file-op-9p.h > @@ -19,13 +19,32 @@ > #include <sys/stat.h> > #include <sys/uio.h> > #include <sys/vfs.h> > +#define SM_LOCAL_MODE_BITS 0600 > +#define SM_LOCAL_DIR_MODE_BITS 0700 > + > +typedef enum > +{ > + SM_PASSTHROUGH = 1, /* uid/gid set on fileserver files */ > + SM_MAPPED, /* uid/gid part of xattr */ > +} SecModel; > + > +typedef struct FsCred > +{ > + uid_t fc_uid; > + gid_t fc_gid; > + mode_t fc_mode; > + dev_t fc_rdev; > +} FsCred; > > typedef struct FsContext > { > char *fs_root; > + SecModel fs_sm; > uid_t uid; > } FsContext; > > +extern void cred_init(FsCred *); > + > typedef struct FileOperations > { > int (*lstat)(FsContext *, const char *, struct stat *); > @@ -43,7 +62,7 @@ typedef struct FileOperations > int (*closedir)(FsContext *, DIR *); > DIR *(*opendir)(FsContext *, const char *); > int (*open)(FsContext *, const char *, int); > - int (*open2)(FsContext *, const char *, int, mode_t); > + int (*open2)(FsContext *, const char *, int, FsCred *); > void (*rewinddir)(FsContext *, DIR *); > off_t (*telldir)(FsContext *, DIR *); > struct dirent *(*readdir)(FsContext *, DIR *); > @@ -51,7 +70,7 @@ typedef struct FileOperations > ssize_t (*readv)(FsContext *, int, const struct iovec *, int); > ssize_t (*writev)(FsContext *, int, const struct iovec *, int); > off_t (*lseek)(FsContext *, int, off_t, int); > - int (*mkdir)(FsContext *, const char *, mode_t); > + int (*mkdir)(FsContext *, const char *, FsCred *); > int (*fstat)(FsContext *, int, struct stat *); > int (*rename)(FsContext *, const char *, const char *); > int (*truncate)(FsContext *, const char *, off_t); > diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c > index 78960ac..f6c2fe2 100644 > --- a/hw/virtio-9p-local.c > +++ b/hw/virtio-9p-local.c > @@ -17,6 +17,7 @@ > #include <grp.h> > #include <sys/socket.h> > #include <sys/un.h> > +#include <attr/xattr.h> > > static const char *rpath(FsContext *ctx, const char *path) > { > @@ -31,47 +32,39 @@ static int local_lstat(FsContext *ctx, const char *path, struct stat *stbuf) > return lstat(rpath(ctx, path), stbuf); > } > > -static int local_setuid(FsContext *ctx, uid_t uid) > +static int local_set_xattr(const char *path, FsCred *credp) > { > - struct passwd *pw; > - gid_t groups[33]; > - int ngroups; > - static uid_t cur_uid = -1; > - > - if (cur_uid == uid) { > - return 0; > - } > - > - if (setreuid(0, 0)) { > - return -1; > - } > - > - pw = getpwuid(uid); > - if (pw == NULL) { > - return -1; > + int err; > + if (credp->fc_uid != -1) { > + err = setxattr(path, "user.virtfs.uid", &credp->fc_uid, sizeof(uid_t), > + 0); > + if (err) { > + return err; > + } > } > - > - ngroups = 33; > - if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1) { > - return -1; > + if (credp->fc_gid != -1) { > + err = setxattr(path, "user.virtfs.gid", &credp->fc_gid, sizeof(gid_t), > + 0); > + if (err) { > + return err; > + } > } > - > - if (setgroups(ngroups, groups)) { > - return -1; > - } > - > - if (setregid(-1, pw->pw_gid)) { > - return -1; > + if (credp->fc_mode != -1) { > + err = setxattr(path, "user.virtfs.mode", &credp->fc_mode, > + sizeof(mode_t), 0); > + if (err) { > + return err; > + } > } > - > - if (setreuid(-1, uid)) { > - return -1; > + if (credp->fc_rdev != -1) { > + err = setxattr(path, "user.virtfs.rdev", &credp->fc_rdev, > + sizeof(dev_t), 0); > + if (err) { > + return err; > + } > } > - > - cur_uid = uid; > - > - return 0; > -} > + return 0; > + } > > static ssize_t local_readlink(FsContext *ctx, const char *path, > char *buf, size_t bufsz) > @@ -168,9 +161,44 @@ static int local_mksock(FsContext *ctx2, const char *path) > return 0; > } > > -static int local_mkdir(FsContext *ctx, const char *path, mode_t mode) > +static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) > { > - return mkdir(rpath(ctx, path), mode); > + int err = -1; > + int serrno = 0; > + /* Determine the security model */ > + if (fs_ctx->fs_sm == SM_MAPPED) { > + err = mkdir(rpath(fs_ctx, path), SM_LOCAL_DIR_MODE_BITS); > + if (err == -1) { > + return err; > + } > + credp->fc_mode = credp->fc_mode|S_IFDIR; > + err = local_set_xattr(rpath(fs_ctx, path), credp); > + if (err == -1) { > + serrno = errno; > + goto err_end; > + } > + } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { > + err = mkdir(rpath(fs_ctx, path), credp->fc_mode); > + if (err == -1) { > + return err; > + } > + err = chmod(rpath(fs_ctx, path), credp->fc_mode & 07777); > + if (err == -1) { > + serrno = errno; > + goto err_end; > + } > + err = chown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid); > + if (err == -1) { > + serrno = errno; > + goto err_end; > + } > + } > + return err; > + > +err_end: > + remove(rpath(fs_ctx, path)); > + errno = serrno; > + return err; > } > > static int local_fstat(FsContext *ctx, int fd, struct stat *stbuf) > @@ -178,9 +206,49 @@ static int local_fstat(FsContext *ctx, int fd, struct stat *stbuf) > return fstat(fd, stbuf); > } > > -static int local_open2(FsContext *ctx, const char *path, int flags, mode_t mode) > +static int local_open2(FsContext *fs_ctx, const char *path, int flags, > + FsCred *credp) > { > - return open(rpath(ctx, path), flags, mode); > + int fd = -1; > + int err = -1; > + int serrno = 0; > + /* Determine the security model */ > + if (fs_ctx->fs_sm == SM_MAPPED) { > + int err; > + fd = open(rpath(fs_ctx, path), flags, SM_LOCAL_MODE_BITS); > + if (fd == -1) { > + return fd; > + } > + credp->fc_mode = credp->fc_mode|S_IFREG; > + /* Set cleint credentials in xattr */ > + err = local_set_xattr(rpath(fs_ctx, path), credp); > + if (err == -1) { > + serrno = errno; > + goto err_end; > + } > + } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { > + fd = open(rpath(fs_ctx, path), flags, credp->fc_mode); > + if (fd == -1) { > + return fd; > + } > + err = chmod(rpath(fs_ctx, path), credp->fc_mode & 07777); > + if (err == -1) { > + serrno = errno; > + goto err_end; > + } > + err = chown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid); > + if (err == -1) { > + serrno = errno; > + goto err_end; > + } > + } > + return fd; > + > +err_end: > + close(fd); > + remove(rpath(fs_ctx, path)); > + errno = serrno; > + return err; > } > > static int local_symlink(FsContext *ctx, const char *oldpath, > @@ -269,7 +337,6 @@ static int local_statfs(FsContext *s, const char *path, struct statfs *stbuf) > > FileOperations local_ops = { > .lstat = local_lstat, > - .setuid = local_setuid, > .readlink = local_readlink, > .close = local_close, > .closedir = local_closedir, > diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c > index 6feaa53..ef870a3 100644 > --- a/hw/virtio-9p.c > +++ b/hw/virtio-9p.c > @@ -67,14 +67,17 @@ static int omode_to_uflags(int8_t mode) > return ret; > } > > -static int v9fs_do_lstat(V9fsState *s, V9fsString *path, struct stat *stbuf) > +void cred_init(FsCred *credp) > { > - return s->ops->lstat(&s->ctx, path->data, stbuf); > + credp->fc_uid = -1; > + credp->fc_gid = -1; > + credp->fc_mode = -1; > + credp->fc_rdev = -1; > } > > -static int v9fs_do_setuid(V9fsState *s, uid_t uid) > +static int v9fs_do_lstat(V9fsState *s, V9fsString *path, struct stat *stbuf) > { > - return s->ops->setuid(&s->ctx, uid); > + return s->ops->lstat(&s->ctx, path->data, stbuf); > } > > static ssize_t v9fs_do_readlink(V9fsState *s, V9fsString *path, V9fsString *buf) > @@ -164,9 +167,15 @@ static int v9fs_do_mksock(V9fsState *s, V9fsString *path) > return s->ops->mksock(&s->ctx, path->data); > } > > -static int v9fs_do_mkdir(V9fsState *s, V9fsString *path, mode_t mode) > +static int v9fs_do_mkdir(V9fsState *s, V9fsCreateState *vs) > { > - return s->ops->mkdir(&s->ctx, path->data, mode); > + FsCred cred; > + > + cred_init(&cred); > + cred.fc_uid = vs->fidp->uid; > + cred.fc_mode = vs->perm & 0777; > + > + return s->ops->mkdir(&s->ctx, vs->fullname.data, &cred); > } > > static int v9fs_do_fstat(V9fsState *s, int fd, struct stat *stbuf) > @@ -174,9 +183,17 @@ static int v9fs_do_fstat(V9fsState *s, int fd, struct stat *stbuf) > return s->ops->fstat(&s->ctx, fd, stbuf); > } > > -static int v9fs_do_open2(V9fsState *s, V9fsString *path, int flags, mode_t mode) > +static int v9fs_do_open2(V9fsState *s, V9fsCreateState *vs) > { > - return s->ops->open2(&s->ctx, path->data, flags, mode); > + FsCred cred; > + int flags; > + > + cred_init(&cred); > + cred.fc_uid = vs->fidp->uid; > + cred.fc_mode = vs->perm & 0777; > + flags = omode_to_uflags(vs->mode) | O_CREAT; > + > + return s->ops->open2(&s->ctx, vs->fullname.data, flags, &cred); > } > > static int v9fs_do_symlink(V9fsState *s, V9fsString *oldpath, > @@ -348,7 +365,6 @@ static V9fsFidState *lookup_fid(V9fsState *s, int32_t fid) > > for (f = s->fid_list; f; f = f->next) { > if (f->fid == fid) { > - v9fs_do_setuid(s, f->uid); > return f; > } > } > @@ -1762,7 +1778,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err) > } > > if (vs->perm & P9_STAT_MODE_DIR) { > - err = v9fs_do_mkdir(s, &vs->fullname, vs->perm & 0777); > + 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); > @@ -1809,9 +1825,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err) > err = v9fs_do_mksock(s, &vs->fullname); > v9fs_create_post_mksock(s, vs, err); > } else { > - vs->fidp->fd = v9fs_do_open2(s, &vs->fullname, > - omode_to_uflags(vs->mode) | O_CREAT, > - vs->perm & 0777); > + vs->fidp->fd = v9fs_do_open2(s, vs); > v9fs_create_post_open2(s, vs, err); > } > > @@ -2322,10 +2336,12 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf) > > if (!strcmp(fse->security_model, "passthrough")) { > /* Files on the Fileserver set to client user credentials */ > + s->ctx.fs_sm = SM_PASSTHROUGH; > } else if (!strcmp(fse->security_model, "mapped")) { > /* Files on the fileserver are set to QEMU credentials. > * Client user credentials are saved in extended attributes. > */ > + s->ctx.fs_sm = SM_MAPPED; > } else { > /* user haven't specified a correct security option */ > fprintf(stderr, "one of the following must be specified as the" > -- > 1.6.5.2 > > Can you split this to two patch. 1) Introduce just the security model and related xattr 2) Introduce the open2 section Can we get something like the below security_chmod(..) { if (fs_ctx->fs_sm == SM_MAPPED) { } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { } else return -1; } static int local_open2(FsContext *fs_ctx, const char *path, int flags, FsCred *credp) { fd = open(rpath(fs_ctx, path), flags, credp->fc_mode); if (fd == -1) { return fd; } err = security_chmod(rpath(fs_ctx, path), credp); if (err == -1) { serrno = errno; goto err_end; } err = security_chown(rpath(fs_ctx, path), credp); if (err == -1) { serrno = errno; goto err_end; } return fd; err_end: close(fd); remove(rpath(fs_ctx, path)); errno = serrno; return err; } So you basically introduce security_chmod ( I guess we need a better name) in patch 1 That also tells us better how is chmod different between two security models. -aneesh
Aneesh Kumar K.V wrote: > On Wed, May 26, 2010 at 04:21:42PM -0700, Venkateswararao Jujjuri (JV) wrote: >> Add required infrastructure and modify create/open2 and mkdir per the new >> security model. >> >> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com> >> --- >> hw/file-op-9p.h | 23 +++++++- >> hw/virtio-9p-local.c | 149 ++++++++++++++++++++++++++++++++++++-------------- >> hw/virtio-9p.c | 42 ++++++++++---- >> 3 files changed, 158 insertions(+), 56 deletions(-) >> >> diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h >> index 2934ff1..73d59b2 100644 >> --- a/hw/file-op-9p.h >> +++ b/hw/file-op-9p.h >> @@ -19,13 +19,32 @@ >> #include <sys/stat.h> >> #include <sys/uio.h> >> #include <sys/vfs.h> >> +#define SM_LOCAL_MODE_BITS 0600 >> +#define SM_LOCAL_DIR_MODE_BITS 0700 >> + >> +typedef enum >> +{ >> + SM_PASSTHROUGH = 1, /* uid/gid set on fileserver files */ >> + SM_MAPPED, /* uid/gid part of xattr */ >> +} SecModel; >> + >> +typedef struct FsCred >> +{ >> + uid_t fc_uid; >> + gid_t fc_gid; >> + mode_t fc_mode; >> + dev_t fc_rdev; >> +} FsCred; >> >> typedef struct FsContext >> { >> char *fs_root; >> + SecModel fs_sm; >> uid_t uid; >> } FsContext; >> >> +extern void cred_init(FsCred *); >> + >> typedef struct FileOperations >> { >> int (*lstat)(FsContext *, const char *, struct stat *); >> @@ -43,7 +62,7 @@ typedef struct FileOperations >> int (*closedir)(FsContext *, DIR *); >> DIR *(*opendir)(FsContext *, const char *); >> int (*open)(FsContext *, const char *, int); >> - int (*open2)(FsContext *, const char *, int, mode_t); >> + int (*open2)(FsContext *, const char *, int, FsCred *); >> void (*rewinddir)(FsContext *, DIR *); >> off_t (*telldir)(FsContext *, DIR *); >> struct dirent *(*readdir)(FsContext *, DIR *); >> @@ -51,7 +70,7 @@ typedef struct FileOperations >> ssize_t (*readv)(FsContext *, int, const struct iovec *, int); >> ssize_t (*writev)(FsContext *, int, const struct iovec *, int); >> off_t (*lseek)(FsContext *, int, off_t, int); >> - int (*mkdir)(FsContext *, const char *, mode_t); >> + int (*mkdir)(FsContext *, const char *, FsCred *); >> int (*fstat)(FsContext *, int, struct stat *); >> int (*rename)(FsContext *, const char *, const char *); >> int (*truncate)(FsContext *, const char *, off_t); >> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c >> index 78960ac..f6c2fe2 100644 >> --- a/hw/virtio-9p-local.c >> +++ b/hw/virtio-9p-local.c >> @@ -17,6 +17,7 @@ >> #include <grp.h> >> #include <sys/socket.h> >> #include <sys/un.h> >> +#include <attr/xattr.h> >> >> static const char *rpath(FsContext *ctx, const char *path) >> { >> @@ -31,47 +32,39 @@ static int local_lstat(FsContext *ctx, const char *path, struct stat *stbuf) >> return lstat(rpath(ctx, path), stbuf); >> } >> >> -static int local_setuid(FsContext *ctx, uid_t uid) >> +static int local_set_xattr(const char *path, FsCred *credp) >> { >> - struct passwd *pw; >> - gid_t groups[33]; >> - int ngroups; >> - static uid_t cur_uid = -1; >> - >> - if (cur_uid == uid) { >> - return 0; >> - } >> - >> - if (setreuid(0, 0)) { >> - return -1; >> - } >> - >> - pw = getpwuid(uid); >> - if (pw == NULL) { >> - return -1; >> + int err; >> + if (credp->fc_uid != -1) { >> + err = setxattr(path, "user.virtfs.uid", &credp->fc_uid, sizeof(uid_t), >> + 0); >> + if (err) { >> + return err; >> + } >> } >> - >> - ngroups = 33; >> - if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1) { >> - return -1; >> + if (credp->fc_gid != -1) { >> + err = setxattr(path, "user.virtfs.gid", &credp->fc_gid, sizeof(gid_t), >> + 0); >> + if (err) { >> + return err; >> + } >> } >> - >> - if (setgroups(ngroups, groups)) { >> - return -1; >> - } >> - >> - if (setregid(-1, pw->pw_gid)) { >> - return -1; >> + if (credp->fc_mode != -1) { >> + err = setxattr(path, "user.virtfs.mode", &credp->fc_mode, >> + sizeof(mode_t), 0); >> + if (err) { >> + return err; >> + } >> } >> - >> - if (setreuid(-1, uid)) { >> - return -1; >> + if (credp->fc_rdev != -1) { >> + err = setxattr(path, "user.virtfs.rdev", &credp->fc_rdev, >> + sizeof(dev_t), 0); >> + if (err) { >> + return err; >> + } >> } >> - >> - cur_uid = uid; >> - >> - return 0; >> -} >> + return 0; >> + } >> >> static ssize_t local_readlink(FsContext *ctx, const char *path, >> char *buf, size_t bufsz) >> @@ -168,9 +161,44 @@ static int local_mksock(FsContext *ctx2, const char *path) >> return 0; >> } >> >> -static int local_mkdir(FsContext *ctx, const char *path, mode_t mode) >> +static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) >> { >> - return mkdir(rpath(ctx, path), mode); >> + int err = -1; >> + int serrno = 0; >> + /* Determine the security model */ >> + if (fs_ctx->fs_sm == SM_MAPPED) { >> + err = mkdir(rpath(fs_ctx, path), SM_LOCAL_DIR_MODE_BITS); >> + if (err == -1) { >> + return err; >> + } >> + credp->fc_mode = credp->fc_mode|S_IFDIR; >> + err = local_set_xattr(rpath(fs_ctx, path), credp); >> + if (err == -1) { >> + serrno = errno; >> + goto err_end; >> + } >> + } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { >> + err = mkdir(rpath(fs_ctx, path), credp->fc_mode); >> + if (err == -1) { >> + return err; >> + } >> + err = chmod(rpath(fs_ctx, path), credp->fc_mode & 07777); >> + if (err == -1) { >> + serrno = errno; >> + goto err_end; >> + } >> + err = chown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid); >> + if (err == -1) { >> + serrno = errno; >> + goto err_end; >> + } >> + } >> + return err; >> + >> +err_end: >> + remove(rpath(fs_ctx, path)); >> + errno = serrno; >> + return err; >> } >> >> static int local_fstat(FsContext *ctx, int fd, struct stat *stbuf) >> @@ -178,9 +206,49 @@ static int local_fstat(FsContext *ctx, int fd, struct stat *stbuf) >> return fstat(fd, stbuf); >> } >> >> -static int local_open2(FsContext *ctx, const char *path, int flags, mode_t mode) >> +static int local_open2(FsContext *fs_ctx, const char *path, int flags, >> + FsCred *credp) >> { >> - return open(rpath(ctx, path), flags, mode); >> + int fd = -1; >> + int err = -1; >> + int serrno = 0; >> + /* Determine the security model */ >> + if (fs_ctx->fs_sm == SM_MAPPED) { >> + int err; >> + fd = open(rpath(fs_ctx, path), flags, SM_LOCAL_MODE_BITS); >> + if (fd == -1) { >> + return fd; >> + } >> + credp->fc_mode = credp->fc_mode|S_IFREG; >> + /* Set cleint credentials in xattr */ >> + err = local_set_xattr(rpath(fs_ctx, path), credp); >> + if (err == -1) { >> + serrno = errno; >> + goto err_end; >> + } >> + } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { >> + fd = open(rpath(fs_ctx, path), flags, credp->fc_mode); >> + if (fd == -1) { >> + return fd; >> + } >> + err = chmod(rpath(fs_ctx, path), credp->fc_mode & 07777); >> + if (err == -1) { >> + serrno = errno; >> + goto err_end; >> + } >> + err = chown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid); >> + if (err == -1) { >> + serrno = errno; >> + goto err_end; >> + } >> + } >> + return fd; >> + >> +err_end: >> + close(fd); >> + remove(rpath(fs_ctx, path)); >> + errno = serrno; >> + return err; >> } >> >> static int local_symlink(FsContext *ctx, const char *oldpath, >> @@ -269,7 +337,6 @@ static int local_statfs(FsContext *s, const char *path, struct statfs *stbuf) >> >> FileOperations local_ops = { >> .lstat = local_lstat, >> - .setuid = local_setuid, >> .readlink = local_readlink, >> .close = local_close, >> .closedir = local_closedir, >> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c >> index 6feaa53..ef870a3 100644 >> --- a/hw/virtio-9p.c >> +++ b/hw/virtio-9p.c >> @@ -67,14 +67,17 @@ static int omode_to_uflags(int8_t mode) >> return ret; >> } >> >> -static int v9fs_do_lstat(V9fsState *s, V9fsString *path, struct stat *stbuf) >> +void cred_init(FsCred *credp) >> { >> - return s->ops->lstat(&s->ctx, path->data, stbuf); >> + credp->fc_uid = -1; >> + credp->fc_gid = -1; >> + credp->fc_mode = -1; >> + credp->fc_rdev = -1; >> } >> >> -static int v9fs_do_setuid(V9fsState *s, uid_t uid) >> +static int v9fs_do_lstat(V9fsState *s, V9fsString *path, struct stat *stbuf) >> { >> - return s->ops->setuid(&s->ctx, uid); >> + return s->ops->lstat(&s->ctx, path->data, stbuf); >> } >> >> static ssize_t v9fs_do_readlink(V9fsState *s, V9fsString *path, V9fsString *buf) >> @@ -164,9 +167,15 @@ static int v9fs_do_mksock(V9fsState *s, V9fsString *path) >> return s->ops->mksock(&s->ctx, path->data); >> } >> >> -static int v9fs_do_mkdir(V9fsState *s, V9fsString *path, mode_t mode) >> +static int v9fs_do_mkdir(V9fsState *s, V9fsCreateState *vs) >> { >> - return s->ops->mkdir(&s->ctx, path->data, mode); >> + FsCred cred; >> + >> + cred_init(&cred); >> + cred.fc_uid = vs->fidp->uid; >> + cred.fc_mode = vs->perm & 0777; >> + >> + return s->ops->mkdir(&s->ctx, vs->fullname.data, &cred); >> } >> >> static int v9fs_do_fstat(V9fsState *s, int fd, struct stat *stbuf) >> @@ -174,9 +183,17 @@ static int v9fs_do_fstat(V9fsState *s, int fd, struct stat *stbuf) >> return s->ops->fstat(&s->ctx, fd, stbuf); >> } >> >> -static int v9fs_do_open2(V9fsState *s, V9fsString *path, int flags, mode_t mode) >> +static int v9fs_do_open2(V9fsState *s, V9fsCreateState *vs) >> { >> - return s->ops->open2(&s->ctx, path->data, flags, mode); >> + FsCred cred; >> + int flags; >> + >> + cred_init(&cred); >> + cred.fc_uid = vs->fidp->uid; >> + cred.fc_mode = vs->perm & 0777; >> + flags = omode_to_uflags(vs->mode) | O_CREAT; >> + >> + return s->ops->open2(&s->ctx, vs->fullname.data, flags, &cred); >> } >> >> static int v9fs_do_symlink(V9fsState *s, V9fsString *oldpath, >> @@ -348,7 +365,6 @@ static V9fsFidState *lookup_fid(V9fsState *s, int32_t fid) >> >> for (f = s->fid_list; f; f = f->next) { >> if (f->fid == fid) { >> - v9fs_do_setuid(s, f->uid); >> return f; >> } >> } >> @@ -1762,7 +1778,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err) >> } >> >> if (vs->perm & P9_STAT_MODE_DIR) { >> - err = v9fs_do_mkdir(s, &vs->fullname, vs->perm & 0777); >> + 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); >> @@ -1809,9 +1825,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err) >> err = v9fs_do_mksock(s, &vs->fullname); >> v9fs_create_post_mksock(s, vs, err); >> } else { >> - vs->fidp->fd = v9fs_do_open2(s, &vs->fullname, >> - omode_to_uflags(vs->mode) | O_CREAT, >> - vs->perm & 0777); >> + vs->fidp->fd = v9fs_do_open2(s, vs); >> v9fs_create_post_open2(s, vs, err); >> } >> >> @@ -2322,10 +2336,12 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf) >> >> if (!strcmp(fse->security_model, "passthrough")) { >> /* Files on the Fileserver set to client user credentials */ >> + s->ctx.fs_sm = SM_PASSTHROUGH; >> } else if (!strcmp(fse->security_model, "mapped")) { >> /* Files on the fileserver are set to QEMU credentials. >> * Client user credentials are saved in extended attributes. >> */ >> + s->ctx.fs_sm = SM_MAPPED; >> } else { >> /* user haven't specified a correct security option */ >> fprintf(stderr, "one of the following must be specified as the" >> -- >> 1.6.5.2 >> >> > > > Can you split this to two patch. > > 1) Introduce just the security model and related xattr > 2) Introduce the open2 section Yeah.. will be doing it. > > > Can we get something like the below > > security_chmod(..) > { > if (fs_ctx->fs_sm == SM_MAPPED) { > > } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { > > } else > return -1; > } > > static int local_open2(FsContext *fs_ctx, const char *path, int flags, > FsCred *credp) > { > fd = open(rpath(fs_ctx, path), flags, credp->fc_mode); I don't think we can do this.. We create a regular file on the fileserver for the special file on the client. Even for open2 the mode bits are going to be different depending on the security model. So, practically we need to have security_open(), security_mknod, security_symlink() ane in each of that we need to compare the security model and act accordingly. > if (fd == -1) { > return fd; > } > err = security_chmod(rpath(fs_ctx, path), credp); > if (err == -1) { > serrno = errno; > goto err_end; > } > err = security_chown(rpath(fs_ctx, path), credp); > if (err == -1) { > serrno = errno; > goto err_end; > } > return fd; What is this going to buy us? In the current code, one function does everything. With this, we will be calling 3 functions and all 3 functions are going to compare the security model...so the security model will be spread all over the place anyway. Also, please note that the whole security model checks are limited to this file only. Rest of the VirtFS server is mostly agnostic to the security model...with this hope you are ok with the current model. :) Thanks, JV > > err_end: > close(fd); > remove(rpath(fs_ctx, path)); > errno = serrno; > return err; > > > } > > > > So you basically introduce security_chmod ( I guess we need a better name) in patch 1 > That also tells us better how is chmod different between two security models. > > -aneesh
diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h index 2934ff1..73d59b2 100644 --- a/hw/file-op-9p.h +++ b/hw/file-op-9p.h @@ -19,13 +19,32 @@ #include <sys/stat.h> #include <sys/uio.h> #include <sys/vfs.h> +#define SM_LOCAL_MODE_BITS 0600 +#define SM_LOCAL_DIR_MODE_BITS 0700 + +typedef enum +{ + SM_PASSTHROUGH = 1, /* uid/gid set on fileserver files */ + SM_MAPPED, /* uid/gid part of xattr */ +} SecModel; + +typedef struct FsCred +{ + uid_t fc_uid; + gid_t fc_gid; + mode_t fc_mode; + dev_t fc_rdev; +} FsCred; typedef struct FsContext { char *fs_root; + SecModel fs_sm; uid_t uid; } FsContext; +extern void cred_init(FsCred *); + typedef struct FileOperations { int (*lstat)(FsContext *, const char *, struct stat *); @@ -43,7 +62,7 @@ typedef struct FileOperations int (*closedir)(FsContext *, DIR *); DIR *(*opendir)(FsContext *, const char *); int (*open)(FsContext *, const char *, int); - int (*open2)(FsContext *, const char *, int, mode_t); + int (*open2)(FsContext *, const char *, int, FsCred *); void (*rewinddir)(FsContext *, DIR *); off_t (*telldir)(FsContext *, DIR *); struct dirent *(*readdir)(FsContext *, DIR *); @@ -51,7 +70,7 @@ typedef struct FileOperations ssize_t (*readv)(FsContext *, int, const struct iovec *, int); ssize_t (*writev)(FsContext *, int, const struct iovec *, int); off_t (*lseek)(FsContext *, int, off_t, int); - int (*mkdir)(FsContext *, const char *, mode_t); + int (*mkdir)(FsContext *, const char *, FsCred *); int (*fstat)(FsContext *, int, struct stat *); int (*rename)(FsContext *, const char *, const char *); int (*truncate)(FsContext *, const char *, off_t); diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c index 78960ac..f6c2fe2 100644 --- a/hw/virtio-9p-local.c +++ b/hw/virtio-9p-local.c @@ -17,6 +17,7 @@ #include <grp.h> #include <sys/socket.h> #include <sys/un.h> +#include <attr/xattr.h> static const char *rpath(FsContext *ctx, const char *path) { @@ -31,47 +32,39 @@ static int local_lstat(FsContext *ctx, const char *path, struct stat *stbuf) return lstat(rpath(ctx, path), stbuf); } -static int local_setuid(FsContext *ctx, uid_t uid) +static int local_set_xattr(const char *path, FsCred *credp) { - struct passwd *pw; - gid_t groups[33]; - int ngroups; - static uid_t cur_uid = -1; - - if (cur_uid == uid) { - return 0; - } - - if (setreuid(0, 0)) { - return -1; - } - - pw = getpwuid(uid); - if (pw == NULL) { - return -1; + int err; + if (credp->fc_uid != -1) { + err = setxattr(path, "user.virtfs.uid", &credp->fc_uid, sizeof(uid_t), + 0); + if (err) { + return err; + } } - - ngroups = 33; - if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1) { - return -1; + if (credp->fc_gid != -1) { + err = setxattr(path, "user.virtfs.gid", &credp->fc_gid, sizeof(gid_t), + 0); + if (err) { + return err; + } } - - if (setgroups(ngroups, groups)) { - return -1; - } - - if (setregid(-1, pw->pw_gid)) { - return -1; + if (credp->fc_mode != -1) { + err = setxattr(path, "user.virtfs.mode", &credp->fc_mode, + sizeof(mode_t), 0); + if (err) { + return err; + } } - - if (setreuid(-1, uid)) { - return -1; + if (credp->fc_rdev != -1) { + err = setxattr(path, "user.virtfs.rdev", &credp->fc_rdev, + sizeof(dev_t), 0); + if (err) { + return err; + } } - - cur_uid = uid; - - return 0; -} + return 0; + } static ssize_t local_readlink(FsContext *ctx, const char *path, char *buf, size_t bufsz) @@ -168,9 +161,44 @@ static int local_mksock(FsContext *ctx2, const char *path) return 0; } -static int local_mkdir(FsContext *ctx, const char *path, mode_t mode) +static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) { - return mkdir(rpath(ctx, path), mode); + int err = -1; + int serrno = 0; + /* Determine the security model */ + if (fs_ctx->fs_sm == SM_MAPPED) { + err = mkdir(rpath(fs_ctx, path), SM_LOCAL_DIR_MODE_BITS); + if (err == -1) { + return err; + } + credp->fc_mode = credp->fc_mode|S_IFDIR; + err = local_set_xattr(rpath(fs_ctx, path), credp); + if (err == -1) { + serrno = errno; + goto err_end; + } + } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { + err = mkdir(rpath(fs_ctx, path), credp->fc_mode); + if (err == -1) { + return err; + } + err = chmod(rpath(fs_ctx, path), credp->fc_mode & 07777); + if (err == -1) { + serrno = errno; + goto err_end; + } + err = chown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid); + if (err == -1) { + serrno = errno; + goto err_end; + } + } + return err; + +err_end: + remove(rpath(fs_ctx, path)); + errno = serrno; + return err; } static int local_fstat(FsContext *ctx, int fd, struct stat *stbuf) @@ -178,9 +206,49 @@ static int local_fstat(FsContext *ctx, int fd, struct stat *stbuf) return fstat(fd, stbuf); } -static int local_open2(FsContext *ctx, const char *path, int flags, mode_t mode) +static int local_open2(FsContext *fs_ctx, const char *path, int flags, + FsCred *credp) { - return open(rpath(ctx, path), flags, mode); + int fd = -1; + int err = -1; + int serrno = 0; + /* Determine the security model */ + if (fs_ctx->fs_sm == SM_MAPPED) { + int err; + fd = open(rpath(fs_ctx, path), flags, SM_LOCAL_MODE_BITS); + if (fd == -1) { + return fd; + } + credp->fc_mode = credp->fc_mode|S_IFREG; + /* Set cleint credentials in xattr */ + err = local_set_xattr(rpath(fs_ctx, path), credp); + if (err == -1) { + serrno = errno; + goto err_end; + } + } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) { + fd = open(rpath(fs_ctx, path), flags, credp->fc_mode); + if (fd == -1) { + return fd; + } + err = chmod(rpath(fs_ctx, path), credp->fc_mode & 07777); + if (err == -1) { + serrno = errno; + goto err_end; + } + err = chown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid); + if (err == -1) { + serrno = errno; + goto err_end; + } + } + return fd; + +err_end: + close(fd); + remove(rpath(fs_ctx, path)); + errno = serrno; + return err; } static int local_symlink(FsContext *ctx, const char *oldpath, @@ -269,7 +337,6 @@ static int local_statfs(FsContext *s, const char *path, struct statfs *stbuf) FileOperations local_ops = { .lstat = local_lstat, - .setuid = local_setuid, .readlink = local_readlink, .close = local_close, .closedir = local_closedir, diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c index 6feaa53..ef870a3 100644 --- a/hw/virtio-9p.c +++ b/hw/virtio-9p.c @@ -67,14 +67,17 @@ static int omode_to_uflags(int8_t mode) return ret; } -static int v9fs_do_lstat(V9fsState *s, V9fsString *path, struct stat *stbuf) +void cred_init(FsCred *credp) { - return s->ops->lstat(&s->ctx, path->data, stbuf); + credp->fc_uid = -1; + credp->fc_gid = -1; + credp->fc_mode = -1; + credp->fc_rdev = -1; } -static int v9fs_do_setuid(V9fsState *s, uid_t uid) +static int v9fs_do_lstat(V9fsState *s, V9fsString *path, struct stat *stbuf) { - return s->ops->setuid(&s->ctx, uid); + return s->ops->lstat(&s->ctx, path->data, stbuf); } static ssize_t v9fs_do_readlink(V9fsState *s, V9fsString *path, V9fsString *buf) @@ -164,9 +167,15 @@ static int v9fs_do_mksock(V9fsState *s, V9fsString *path) return s->ops->mksock(&s->ctx, path->data); } -static int v9fs_do_mkdir(V9fsState *s, V9fsString *path, mode_t mode) +static int v9fs_do_mkdir(V9fsState *s, V9fsCreateState *vs) { - return s->ops->mkdir(&s->ctx, path->data, mode); + FsCred cred; + + cred_init(&cred); + cred.fc_uid = vs->fidp->uid; + cred.fc_mode = vs->perm & 0777; + + return s->ops->mkdir(&s->ctx, vs->fullname.data, &cred); } static int v9fs_do_fstat(V9fsState *s, int fd, struct stat *stbuf) @@ -174,9 +183,17 @@ static int v9fs_do_fstat(V9fsState *s, int fd, struct stat *stbuf) return s->ops->fstat(&s->ctx, fd, stbuf); } -static int v9fs_do_open2(V9fsState *s, V9fsString *path, int flags, mode_t mode) +static int v9fs_do_open2(V9fsState *s, V9fsCreateState *vs) { - return s->ops->open2(&s->ctx, path->data, flags, mode); + FsCred cred; + int flags; + + cred_init(&cred); + cred.fc_uid = vs->fidp->uid; + cred.fc_mode = vs->perm & 0777; + flags = omode_to_uflags(vs->mode) | O_CREAT; + + return s->ops->open2(&s->ctx, vs->fullname.data, flags, &cred); } static int v9fs_do_symlink(V9fsState *s, V9fsString *oldpath, @@ -348,7 +365,6 @@ static V9fsFidState *lookup_fid(V9fsState *s, int32_t fid) for (f = s->fid_list; f; f = f->next) { if (f->fid == fid) { - v9fs_do_setuid(s, f->uid); return f; } } @@ -1762,7 +1778,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err) } if (vs->perm & P9_STAT_MODE_DIR) { - err = v9fs_do_mkdir(s, &vs->fullname, vs->perm & 0777); + 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); @@ -1809,9 +1825,7 @@ static void v9fs_create_post_lstat(V9fsState *s, V9fsCreateState *vs, int err) err = v9fs_do_mksock(s, &vs->fullname); v9fs_create_post_mksock(s, vs, err); } else { - vs->fidp->fd = v9fs_do_open2(s, &vs->fullname, - omode_to_uflags(vs->mode) | O_CREAT, - vs->perm & 0777); + vs->fidp->fd = v9fs_do_open2(s, vs); v9fs_create_post_open2(s, vs, err); } @@ -2322,10 +2336,12 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf) if (!strcmp(fse->security_model, "passthrough")) { /* Files on the Fileserver set to client user credentials */ + s->ctx.fs_sm = SM_PASSTHROUGH; } else if (!strcmp(fse->security_model, "mapped")) { /* Files on the fileserver are set to QEMU credentials. * Client user credentials are saved in extended attributes. */ + s->ctx.fs_sm = SM_MAPPED; } else { /* user haven't specified a correct security option */ fprintf(stderr, "one of the following must be specified as the"
Add required infrastructure and modify create/open2 and mkdir per the new security model. Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com> --- hw/file-op-9p.h | 23 +++++++- hw/virtio-9p-local.c | 149 ++++++++++++++++++++++++++++++++++++-------------- hw/virtio-9p.c | 42 ++++++++++---- 3 files changed, 158 insertions(+), 56 deletions(-)