* [Qemu-devel] [V5 PATCH 1/8] Implement qemu_read_full
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 2/8] virtio-9p: Provide chroot environment server side interfaces M. Mohan Kumar
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
Add qemu_read_full function
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
osdep.c | 32 ++++++++++++++++++++++++++++++++
qemu-common.h | 2 ++
2 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/osdep.c b/osdep.c
index 327583b..8d84a88 100644
--- a/osdep.c
+++ b/osdep.c
@@ -127,6 +127,38 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
}
/*
+ * A variant of read(2) which handles interrupted read.
+ * Simlar to qemu_write_full function
+ *
+ * Return the number of bytes read.
+ *
+ * This function does not work with non-blocking fd's.
+ * errno is set if fewer than `count' bytes are read because of any
+ * error
+ */
+ssize_t qemu_read_full(int fd, void *buf, size_t count)
+{
+ ssize_t ret = 0;
+ ssize_t total = 0;
+
+ while (count) {
+ ret = read(fd, buf, count);
+ if (ret <= 0) {
+ if (errno == EINTR) {
+ continue;
+ }
+ break;
+ }
+
+ count -= ret;
+ buf += ret;
+ total += ret;
+ }
+
+ return total;
+}
+
+/*
* Opens a socket with FD_CLOEXEC set
*/
int qemu_socket(int domain, int type, int protocol)
diff --git a/qemu-common.h b/qemu-common.h
index c7ff280..83786d7 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -207,6 +207,8 @@ void qemu_mutex_unlock_iothread(void);
int qemu_open(const char *name, int flags, ...);
ssize_t qemu_write_full(int fd, const void *buf, size_t count)
QEMU_WARN_UNUSED_RESULT;
+ssize_t qemu_read_full(int fd, void *buf, size_t count)
+ QEMU_WARN_UNUSED_RESULT;
void qemu_set_cloexec(int fd);
#ifndef _WIN32
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [V5 PATCH 2/8] virtio-9p: Provide chroot environment server side interfaces
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 1/8] Implement qemu_read_full M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-17 8:54 ` [Qemu-devel] " Stefan Hajnoczi
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 3/8] virtio-9p: Add client side interfaces for chroot environment M. Mohan Kumar
` (5 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
Implement chroot server side interfaces like sending the file
descriptor to qemu process, reading the object request from socket etc.
Also add chroot main function and other helper routines.
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
Makefile.objs | 1 +
hw/9pfs/virtio-9p-chroot-sv.c | 169 +++++++++++++++++++++++++++++++++++++++++
hw/9pfs/virtio-9p-chroot.h | 57 ++++++++++++++
hw/9pfs/virtio-9p.c | 32 ++++++++
hw/file-op-9p.h | 4 +
5 files changed, 263 insertions(+), 0 deletions(-)
create mode 100644 hw/9pfs/virtio-9p-chroot-sv.c
create mode 100644 hw/9pfs/virtio-9p-chroot.h
diff --git a/Makefile.objs b/Makefile.objs
index f930cdd..bf8ebd9 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -282,6 +282,7 @@ hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
9pfs-nested-$(CONFIG_VIRTFS) = virtio-9p-debug.o
9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-local.o virtio-9p-xattr.o
9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-xattr-user.o virtio-9p-posix-acl.o
+9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-chroot-sv.o
hw-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y))
$(addprefix 9pfs/, $(9pfs-nested-y)): CFLAGS += -I$(SRC_PATH)/hw/
diff --git a/hw/9pfs/virtio-9p-chroot-sv.c b/hw/9pfs/virtio-9p-chroot-sv.c
new file mode 100644
index 0000000..86d10cd
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot-sv.c
@@ -0,0 +1,169 @@
+/*
+ * Virtio 9p chroot environment for contained access to exported path
+ * Server side
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ * M. Mohan Kumar <mohan@in.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the copying file in the top-level directory
+ *
+ */
+
+#include <sys/fsuid.h>
+#include <sys/resource.h>
+#include <signal.h>
+#include "virtio.h"
+#include "qemu_socket.h"
+#include "qemu-thread.h"
+#include "qerror.h"
+#include "virtio-9p.h"
+#include "virtio-9p-chroot.h"
+
+/* Send file descriptor and error status to qemu process */
+static void chroot_sendfd(int sockfd, const FdInfo *fd_info)
+{
+ struct msghdr msg = { };
+ struct iovec iov;
+ struct cmsghdr *cmsg;
+ int retval;
+ union MsgControl msg_control;
+
+ iov.iov_base = (void *)fd_info;
+ iov.iov_len = sizeof(*fd_info);
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ /* No ancillary data on error/fd invalid flag */
+ if (fd_info->fi_flags & FI_FDVALID) {
+ msg.msg_control = &msg_control;
+ msg.msg_controllen = sizeof(msg_control);
+
+ cmsg = &msg_control.cmsg;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(fd_info->fi_fd));
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ memcpy(CMSG_DATA(cmsg), &fd_info->fi_fd, sizeof(fd_info->fi_fd));
+ }
+ retval = sendmsg(sockfd, &msg, 0);
+ if (retval == EPIPE || retval == EBADF) {
+ _exit(1);
+ }
+ if (fd_info->fi_flags & FI_FDVALID) {
+ close(fd_info->fi_fd);
+ }
+}
+
+/* Read V9fsFileObjectRequest written by QEMU process */
+static int chroot_read_request(int sockfd, V9fsFileObjectRequest *request)
+{
+ int retval;
+ retval = qemu_read_full(sockfd, request, sizeof(*request));
+ if (retval != sizeof(*request)) {
+ if (errno == EBADF) {
+ _exit(1);
+ }
+ return EIO;
+ }
+ return 0;
+}
+
+static int chroot_daemonize(int chroot_sock)
+{
+ sigset_t sigset;
+ struct rlimit nr_fd;
+ int fd;
+
+ /* Block all signals for this process */
+ sigprocmask(SIG_SETMASK, &sigset, NULL);
+
+ /* Daemonize */
+ if (setsid() < 0) {
+ return errno;
+ }
+
+ /* Close other file descriptors */
+ getrlimit(RLIMIT_NOFILE, &nr_fd);
+ for (fd = 0; fd < nr_fd.rlim_cur; fd++) {
+ if (fd != chroot_sock) {
+ close(fd);
+ }
+ }
+ chdir("/");
+
+ /* Create files with mode as requested by client */
+ umask(0);
+ return 0;
+}
+
+/*
+ * Fork a process and chroot into the share path. Communication
+ * between qemu process and chroot process happens via socket.
+ * All file descriptors (including stdout and stderr) are closed
+ * except one socket descriptor (which is used for communicating
+ * between qemu process and chroot process).
+ * Note: To avoid errors in forked process in multi threaded environment
+ * use only async-signal safe functions only. For more information see
+ * man fork(3p), signal(7)
+ */
+int v9fs_chroot(FsContext *fs_ctx)
+{
+ int fd_pair[2], chroot_sock, error;
+ V9fsFileObjectRequest request;
+ pid_t pid;
+ uint64_t code;
+ FdInfo fd_info;
+
+ if (socketpair(PF_UNIX, SOCK_STREAM, 0, fd_pair) < 0) {
+ error_report("socketpair %s", strerror(errno));
+ return -1;
+ }
+
+ pid = fork();
+ if (pid < 0) {
+ error_report("fork %s", strerror(errno));
+ return -1;
+ }
+ if (pid != 0) {
+ fs_ctx->chroot_socket = fd_pair[0];
+ close(fd_pair[1]);
+ return 0;
+ }
+
+ close(fd_pair[0]);
+ chroot_sock = fd_pair[1];
+ if (chroot(fs_ctx->fs_root) < 0) {
+ code = CHROOT_ERROR << 32 | errno;
+ error = qemu_write_full(chroot_sock, &code, sizeof(code));
+ _exit(1);
+ }
+
+ error = chroot_daemonize(chroot_sock);
+ if (error) {
+ code = SETSID_ERROR << 32 | error;
+ error = qemu_write_full(chroot_sock, &code, sizeof(code));
+ _exit(1);
+ }
+
+ /*
+ * Write 0 to chroot socket to indicate chroot process creation is
+ * successful
+ */
+ code = 0;
+ if (qemu_write_full(chroot_sock, &code, sizeof(code))
+ != sizeof(code)) {
+ _exit(1);
+ }
+ /* get the request from the socket */
+ while (1) {
+ memset(&fd_info, 0, sizeof(fd_info));
+ if (chroot_read_request(chroot_sock, &request) == EIO) {
+ fd_info.fi_fd = 0;
+ fd_info.fi_error = EIO;
+ fd_info.fi_flags = FI_SOCKERR;
+ chroot_sendfd(chroot_sock, &fd_info);
+ }
+ }
+}
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
new file mode 100644
index 0000000..73cf144
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -0,0 +1,57 @@
+#ifndef _QEMU_VIRTIO_9P_CHROOT_H
+#define _QEMU_VIRTIO_9P_CHROOT_H
+
+/* types for V9fsFileObjectRequest */
+#define T_OPEN 1
+#define T_CREATE 2
+#define T_MKDIR 3
+#define T_MKNOD 4
+#define T_SYMLINK 5
+#define T_LINK 6
+#define CHROOT_ERROR 1ULL
+#define SETSID_ERROR 2ULL
+
+/*
+ * Structure used by chroot functions to transmit file descriptor and
+ * error info
+ */
+typedef struct {
+ int fi_fd;
+#define FI_FDVALID 1
+#define FI_SOCKERR 2
+ int fi_flags;
+ int fi_error;
+} FdInfo;
+
+union MsgControl {
+ struct cmsghdr cmsg;
+ char control[CMSG_SPACE(sizeof(int))];
+};
+
+struct V9fsFileObjectData
+{
+ int flags;
+ int mode;
+ uid_t uid;
+ gid_t gid;
+ dev_t dev;
+ int path_len;
+ int oldpath_len;
+ int type;
+};
+
+struct V9fsFileObjectPath
+{
+ char path[PATH_MAX];
+ char old_path[PATH_MAX];
+};
+
+typedef struct V9fsFileObjectRequest
+{
+ struct V9fsFileObjectData data;
+ struct V9fsFileObjectPath path;
+} V9fsFileObjectRequest;
+
+int v9fs_chroot(FsContext *fs_ctx);
+
+#endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 27e7750..3aff70b 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -14,10 +14,12 @@
#include "virtio.h"
#include "pc.h"
#include "qemu_socket.h"
+#include "qerror.h"
#include "virtio-9p.h"
#include "fsdev/qemu-fsdev.h"
#include "virtio-9p-debug.h"
#include "virtio-9p-xattr.h"
+#include "virtio-9p-chroot.h"
int debug_9p_pdu;
@@ -3743,5 +3745,35 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
s->tag_len;
s->vdev.get_config = virtio_9p_get_config;
+ if (s->ctx.fs_sm == SM_PASSTHROUGH) {
+ uint64_t code;
+ qemu_mutex_init(&s->ctx.chroot_mutex);
+ s->ctx.chroot_ioerror = 0;
+ if (v9fs_chroot(&s->ctx) < 0) {
+ exit(1);
+ }
+
+ /*
+ * Chroot process sends 0 to indicate chroot process creation is
+ * successful
+ */
+ if (read(s->ctx.chroot_socket, &code, sizeof(code)) != sizeof(code)) {
+ error_report("chroot process creation failed");
+ exit(1);
+ }
+ if (code != 0) {
+ switch (code >> 32) {
+ case CHROOT_ERROR:
+ error_report("chroot system call failed: %s",
+ strerror(code & 0xFFFFFFFF));
+ break;
+ case SETSID_ERROR:
+ error_report("setsid failed: %s", strerror(code & 0xFFFFFFFF));
+ break;
+ }
+ exit(1);
+ }
+ }
+
return &s->vdev;
}
diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
index 126e60e..b52b432 100644
--- a/hw/file-op-9p.h
+++ b/hw/file-op-9p.h
@@ -19,6 +19,7 @@
#include <sys/stat.h>
#include <sys/uio.h>
#include <sys/vfs.h>
+#include "qemu-thread.h"
#define SM_LOCAL_MODE_BITS 0600
#define SM_LOCAL_DIR_MODE_BITS 0700
@@ -55,6 +56,9 @@ typedef struct FsContext
SecModel fs_sm;
uid_t uid;
struct xattr_operations **xops;
+ QemuMutex chroot_mutex;
+ int chroot_socket;
+ int chroot_ioerror;
} FsContext;
void cred_init(FsCred *);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [V5 PATCH 3/8] virtio-9p: Add client side interfaces for chroot environment
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 1/8] Implement qemu_read_full M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 2/8] virtio-9p: Provide chroot environment server side interfaces M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 4/8] virtio-9p: Add support to open a file in " M. Mohan Kumar
` (4 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
Define QEMU side interfaces used for chroot environment.
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
Makefile.objs | 2 +-
hw/9pfs/virtio-9p-chroot-clnt.c | 96 +++++++++++++++++++++++++++++++++++++++
hw/9pfs/virtio-9p-chroot.h | 1 +
3 files changed, 98 insertions(+), 1 deletions(-)
create mode 100644 hw/9pfs/virtio-9p-chroot-clnt.c
diff --git a/Makefile.objs b/Makefile.objs
index bf8ebd9..c98cb72 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -282,7 +282,7 @@ hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
9pfs-nested-$(CONFIG_VIRTFS) = virtio-9p-debug.o
9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-local.o virtio-9p-xattr.o
9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-xattr-user.o virtio-9p-posix-acl.o
-9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-chroot-sv.o
+9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-chroot-sv.o virtio-9p-chroot-clnt.o
hw-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y))
$(addprefix 9pfs/, $(9pfs-nested-y)): CFLAGS += -I$(SRC_PATH)/hw/
diff --git a/hw/9pfs/virtio-9p-chroot-clnt.c b/hw/9pfs/virtio-9p-chroot-clnt.c
new file mode 100644
index 0000000..ac65f03
--- /dev/null
+++ b/hw/9pfs/virtio-9p-chroot-clnt.c
@@ -0,0 +1,96 @@
+/*
+ * Virtio 9p chroot environment for contained access to exported path
+ * Client code
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ * M. Mohan Kumar <mohan@in.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the copying file in the top-level directory
+ *
+ */
+
+#include <sys/fsuid.h>
+#include <sys/resource.h>
+#include <signal.h>
+#include "virtio.h"
+#include "qemu_socket.h"
+#include "qemu-thread.h"
+#include "qerror.h"
+#include "virtio-9p.h"
+#include "virtio-9p-chroot.h"
+
+/* Receive file descriptor and error status from chroot process */
+static int v9fs_receivefd(int sockfd, int *error)
+{
+ struct msghdr msg = { };
+ struct iovec iov;
+ union MsgControl msg_control;
+ struct cmsghdr *cmsg;
+ int retval, fd;
+ FdInfo fd_info;
+
+ iov.iov_base = &fd_info;
+ iov.iov_len = sizeof(fd_info);
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = &msg_control;
+ msg.msg_controllen = sizeof(msg_control);
+
+ *error = 0;
+ retval = recvmsg(sockfd, &msg, 0);
+ if (retval < 0) {
+ *error = EIO;
+ return -EIO;
+ }
+ if (fd_info.fi_flags & FI_SOCKERR) {
+ *error = EIO;
+ return -EIO;
+ }
+ /* If error is set, ancillary data is not present */
+ if (fd_info.fi_error) {
+ *error = fd_info.fi_error;
+ return -1;
+ }
+
+ if (!fd_info.fi_flags & FI_FDVALID) {
+ return 0;
+ }
+
+ for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+ if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+ cmsg->cmsg_level != SOL_SOCKET ||
+ cmsg->cmsg_type != SCM_RIGHTS) {
+ continue;
+ }
+ fd = *((int *)CMSG_DATA(cmsg));
+ return fd;
+ }
+
+ *error = EAGAIN;
+ return -1;
+}
+
+/*
+ * V9fsFileObjectRequest is written into the socket by QEMU process.
+ * Then this request is read by chroot process using read_request function
+ */
+static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request)
+{
+ int retval;
+ retval = qemu_write_full(sockfd, request, sizeof(*request));
+ if (retval != sizeof(*request)) {
+ return EIO;
+ }
+ return 0;
+}
+
+void chroot_dummy(void)
+{
+ (void)v9fs_receivefd;
+ (void)v9fs_write_request;
+}
+
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index 73cf144..9122edc 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -53,5 +53,6 @@ typedef struct V9fsFileObjectRequest
} V9fsFileObjectRequest;
int v9fs_chroot(FsContext *fs_ctx);
+void chroot_dummy(void);
#endif /* _QEMU_VIRTIO_9P_CHROOT_H */
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [V5 PATCH 4/8] virtio-9p: Add support to open a file in chroot environment
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
` (2 preceding siblings ...)
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 3/8] virtio-9p: Add client side interfaces for chroot environment M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-17 10:23 ` [Qemu-devel] " Stefan Hajnoczi
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 5/8] virtio-9p: Create support " M. Mohan Kumar
` (3 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
This patch adds both server and client side support to open a file
(directory) in the chroot environment
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
hw/9pfs/virtio-9p-chroot-clnt.c | 22 +++++++++++++---
hw/9pfs/virtio-9p-chroot-sv.c | 26 +++++++++++++++++++
hw/9pfs/virtio-9p-chroot.h | 2 +-
hw/9pfs/virtio-9p-local.c | 53 ++++++++++++++++++++++++++++++++++++---
4 files changed, 94 insertions(+), 9 deletions(-)
diff --git a/hw/9pfs/virtio-9p-chroot-clnt.c b/hw/9pfs/virtio-9p-chroot-clnt.c
index ac65f03..7f72add 100644
--- a/hw/9pfs/virtio-9p-chroot-clnt.c
+++ b/hw/9pfs/virtio-9p-chroot-clnt.c
@@ -88,9 +88,23 @@ static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request)
return 0;
}
-void chroot_dummy(void)
+/* Return opened file descriptor on success or -1 on error */
+int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *request,
+ int *error)
{
- (void)v9fs_receivefd;
- (void)v9fs_write_request;
+ int fd;
+ qemu_mutex_lock(&fs_ctx->chroot_mutex);
+ if (fs_ctx->chroot_ioerror) {
+ *error = EIO;
+ fd = -1;
+ goto unlock;
+ }
+ v9fs_write_request(fs_ctx->chroot_socket, request);
+ fd = v9fs_receivefd(fs_ctx->chroot_socket, error);
+ if (fd == -EIO && *error == EIO) {
+ fs_ctx->chroot_ioerror = 1;
+ }
+unlock:
+ qemu_mutex_unlock(&fs_ctx->chroot_mutex);
+ return fd;
}
-
diff --git a/hw/9pfs/virtio-9p-chroot-sv.c b/hw/9pfs/virtio-9p-chroot-sv.c
index 86d10cd..e9952b3 100644
--- a/hw/9pfs/virtio-9p-chroot-sv.c
+++ b/hw/9pfs/virtio-9p-chroot-sv.c
@@ -70,6 +70,21 @@ static int chroot_read_request(int sockfd, V9fsFileObjectRequest *request)
return 0;
}
+/*
+ * Helper routine to open a file and return fd and error status in
+ * FdInfo structure
+ */
+static void chroot_do_open(V9fsFileObjectRequest *request, FdInfo *fd_info)
+{
+ fd_info->fi_fd = open(request->path.path, request->data.flags);
+ if (fd_info->fi_fd < 0) {
+ fd_info->fi_error = errno;
+ } else {
+ fd_info->fi_error = 0;
+ fd_info->fi_flags = FI_FDVALID;
+ }
+}
+
static int chroot_daemonize(int chroot_sock)
{
sigset_t sigset;
@@ -164,6 +179,17 @@ int v9fs_chroot(FsContext *fs_ctx)
fd_info.fi_error = EIO;
fd_info.fi_flags = FI_SOCKERR;
chroot_sendfd(chroot_sock, &fd_info);
+ continue;
+ }
+ switch (request.data.type) {
+ case T_OPEN:
+ chroot_do_open(&request, &fd_info);
+ break;
+ default:
+ fd_info.fi_fd = 0;
+ fd_info.fi_error = EIO;
+ break;
}
+ chroot_sendfd(chroot_sock, &fd_info);
}
}
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index 9122edc..057dcde 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -53,6 +53,6 @@ typedef struct V9fsFileObjectRequest
} V9fsFileObjectRequest;
int v9fs_chroot(FsContext *fs_ctx);
-void chroot_dummy(void);
+int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *or, int *error);
#endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 0a015de..bef0272 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -13,6 +13,9 @@
#include "virtio.h"
#include "virtio-9p.h"
#include "virtio-9p-xattr.h"
+#include "qemu_socket.h"
+#include "fsdev/qemu-fsdev.h"
+#include "virtio-9p-chroot.h"
#include <arpa/inet.h>
#include <pwd.h>
#include <grp.h>
@@ -20,6 +23,35 @@
#include <sys/un.h>
#include <attr/xattr.h>
+/* Helper routine to fill V9fsFileObjectRequest structure */
+static void fill_request(V9fsFileObjectRequest *request, const char *path,
+ FsCred *credp)
+{
+ memset(request, 0, sizeof(*request));
+ request->data.path_len = strlen(path);
+ strcpy(request->path.path, (path));
+ if (credp) {
+ request->data.mode = credp->fc_mode;
+ request->data.uid = credp->fc_uid;
+ request->data.gid = credp->fc_gid;
+ request->data.dev = credp->fc_rdev;
+ }
+}
+
+static int passthrough_open(FsContext *fs_ctx, const char *path, int flags)
+{
+ V9fsFileObjectRequest request;
+ int fd, error = 0;
+
+ fill_request(&request, path, NULL);
+ request.data.flags = flags;
+ request.data.type = T_OPEN;
+ fd = v9fs_request(fs_ctx, &request, &error);
+ if (fd < 0) {
+ errno = error;
+ }
+ return fd;
+}
static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
{
@@ -138,14 +170,27 @@ static int local_closedir(FsContext *ctx, DIR *dir)
return closedir(dir);
}
-static int local_open(FsContext *ctx, const char *path, int flags)
+static int local_open(FsContext *fs_ctx, const char *path, int flags)
{
- return open(rpath(ctx, path), flags);
+ if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ return passthrough_open(fs_ctx, path, flags);
+ } else {
+ return open(rpath(fs_ctx, path), flags);
+ }
}
-static DIR *local_opendir(FsContext *ctx, const char *path)
+static DIR *local_opendir(FsContext *fs_ctx, const char *path)
{
- return opendir(rpath(ctx, path));
+ if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ int fd;
+ fd = passthrough_open(fs_ctx, path, O_DIRECTORY);
+ if (fd < 0) {
+ return NULL;
+ }
+ return fdopendir(fd);
+ } else {
+ return opendir(rpath(fs_ctx, path));
+ }
}
static void local_rewinddir(FsContext *ctx, DIR *dir)
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [V5 PATCH 4/8] virtio-9p: Add support to open a file in chroot environment
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 4/8] virtio-9p: Add support to open a file in " M. Mohan Kumar
@ 2011-02-17 10:23 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2011-02-17 10:23 UTC (permalink / raw)
To: M. Mohan Kumar; +Cc: blauwirbel, qemu-devel
On Wed, Feb 16, 2011 at 12:23 PM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +/* Helper routine to fill V9fsFileObjectRequest structure */
> +static void fill_request(V9fsFileObjectRequest *request, const char *path,
> + FsCred *credp)
> +{
> + memset(request, 0, sizeof(*request));
> + request->data.path_len = strlen(path);
> + strcpy(request->path.path, (path));
It's not obvious that this strcpy() is safe. I tried following this
back into hw/virtio-9p.c and I don't see an explicit PATH_MAX length
limit for path.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [V5 PATCH 5/8] virtio-9p: Create support in chroot environment
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
` (3 preceding siblings ...)
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 4/8] virtio-9p: Add support to open a file in " M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 6/8] virtio-9p: Support for creating special files M. Mohan Kumar
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
Add both server & client side interfaces to create regular files in
chroot environment
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
hw/9pfs/virtio-9p-chroot-sv.c | 41 +++++++++++++++++++++++++++++++++++++++++
hw/9pfs/virtio-9p-local.c | 21 +++++++++++++++++++--
2 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/virtio-9p-chroot-sv.c b/hw/9pfs/virtio-9p-chroot-sv.c
index e9952b3..bab5f7c 100644
--- a/hw/9pfs/virtio-9p-chroot-sv.c
+++ b/hw/9pfs/virtio-9p-chroot-sv.c
@@ -85,6 +85,44 @@ static void chroot_do_open(V9fsFileObjectRequest *request, FdInfo *fd_info)
}
}
+/*
+ * Helper routine to create a file and return the file descriptor and
+ * error status in FdInfo structure.
+ */
+static void chroot_do_create(V9fsFileObjectRequest *request, FdInfo *fd_info)
+{
+ uid_t cur_uid;
+ gid_t cur_gid;
+
+ cur_uid = geteuid();
+ cur_gid = getegid();
+
+ fd_info->fi_fd = -1;
+
+ if (setfsuid(request->data.uid) < 0) {
+ fd_info->fi_error = errno;
+ return;
+ }
+ if (setfsgid(request->data.gid) < 0) {
+ fd_info->fi_error = errno;
+ goto unset_uid;
+ }
+
+ fd_info->fi_fd = open(request->path.path, request->data.flags,
+ request->data.mode);
+
+ if (fd_info->fi_fd < 0) {
+ fd_info->fi_error = errno;
+ } else {
+ fd_info->fi_error = 0;
+ fd_info->fi_flags = FI_FDVALID;
+ }
+
+ setfsgid(cur_gid);
+unset_uid:
+ setfsuid(cur_uid);
+}
+
static int chroot_daemonize(int chroot_sock)
{
sigset_t sigset;
@@ -185,6 +223,9 @@ int v9fs_chroot(FsContext *fs_ctx)
case T_OPEN:
chroot_do_open(&request, &fd_info);
break;
+ case T_CREATE:
+ chroot_do_create(&request, &fd_info);
+ break;
default:
fd_info.fi_fd = 0;
fd_info.fi_error = EIO;
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index bef0272..51911dd 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -53,6 +53,22 @@ static int passthrough_open(FsContext *fs_ctx, const char *path, int flags)
return fd;
}
+static int passthrough_create(FsContext *fs_ctx, const char *path, int flags,
+ FsCred *credp)
+{
+ V9fsFileObjectRequest request;
+ int fd, error = 0;
+
+ fill_request(&request, path, credp);
+ request.data.flags = flags;
+ request.data.type = T_CREATE;
+ fd = v9fs_request(fs_ctx, &request, &error);
+ if (fd < 0) {
+ errno = error;
+ }
+ return fd;
+}
+
static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
{
int err;
@@ -377,8 +393,7 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags,
serrno = errno;
goto err_end;
}
- } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
- (fs_ctx->fs_sm == SM_NONE)) {
+ } else if (fs_ctx->fs_sm == SM_NONE) {
fd = open(rpath(fs_ctx, path), flags, credp->fc_mode);
if (fd == -1) {
return fd;
@@ -388,6 +403,8 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags,
serrno = errno;
goto err_end;
}
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ fd = passthrough_create(fs_ctx, path, flags, credp);
}
return fd;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [V5 PATCH 6/8] virtio-9p: Support for creating special files
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
` (4 preceding siblings ...)
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 5/8] virtio-9p: Create support " M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-17 10:49 ` [Qemu-devel] " Stefan Hajnoczi
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 7/8] virtio-9p: Move file post creation changes to none security model M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 8/8] virtio-9p: Chroot environment for other functions M. Mohan Kumar
7 siblings, 1 reply; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
Add both server and client side interfaces to create special files
(directory, device nodes, links and symbolic links)
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
hw/9pfs/virtio-9p-chroot-clnt.c | 26 ++++++++++
hw/9pfs/virtio-9p-chroot-sv.c | 59 ++++++++++++++++++++++
hw/9pfs/virtio-9p-chroot.h | 2 +
hw/9pfs/virtio-9p-local.c | 104 +++++++++++++++++++++++++++------------
4 files changed, 160 insertions(+), 31 deletions(-)
diff --git a/hw/9pfs/virtio-9p-chroot-clnt.c b/hw/9pfs/virtio-9p-chroot-clnt.c
index 7f72add..30366d3 100644
--- a/hw/9pfs/virtio-9p-chroot-clnt.c
+++ b/hw/9pfs/virtio-9p-chroot-clnt.c
@@ -108,3 +108,29 @@ unlock:
qemu_mutex_unlock(&fs_ctx->chroot_mutex);
return fd;
}
+
+int v9fs_create_special(FsContext *fs_ctx,
+ V9fsFileObjectRequest *request, int *error)
+{
+ int fd;
+ qemu_mutex_lock(&fs_ctx->chroot_mutex);
+ if (fs_ctx->chroot_ioerror) {
+ *error = EIO;
+ fd = -1;
+ goto unlock;
+ } else {
+ *error = 0;
+ }
+ v9fs_write_request(fs_ctx->chroot_socket, request);
+ fd = v9fs_receivefd(fs_ctx->chroot_socket, error);
+ if (fd == -EIO && *error == EIO) {
+ fs_ctx->chroot_ioerror = 1;
+ }
+unlock:
+ qemu_mutex_unlock(&fs_ctx->chroot_mutex);
+ if (*error) {
+ return -1;
+ } else {
+ return 0;
+ }
+}
diff --git a/hw/9pfs/virtio-9p-chroot-sv.c b/hw/9pfs/virtio-9p-chroot-sv.c
index bab5f7c..e3d8843 100644
--- a/hw/9pfs/virtio-9p-chroot-sv.c
+++ b/hw/9pfs/virtio-9p-chroot-sv.c
@@ -123,6 +123,59 @@ unset_uid:
setfsuid(cur_uid);
}
+/*
+ * Create directory, symbolic link, link, device node and regular files
+ * Similar to create, but it does not return the fd of created object
+ * Returns 0 on success, returns errno on failure
+ */
+static void chroot_do_create_special(V9fsFileObjectRequest *request,
+ FdInfo *fd_info)
+{
+ int cur_uid, cur_gid;
+
+ cur_uid = geteuid();
+ cur_gid = getegid();
+
+ fd_info->fi_fd = -1;
+ fd_info->fi_error = 0;
+
+ if (setfsuid(request->data.uid) < 0) {
+ fd_info->fi_error = errno;
+ return;
+ }
+ if (setfsgid(request->data.gid) < 0) {
+ fd_info->fi_error = errno;
+ goto unset_uid;
+ }
+
+ switch (request->data.type) {
+ case T_MKDIR:
+ fd_info->fi_fd = mkdir(request->path.path, request->data.mode);
+ break;
+ case T_SYMLINK:
+ fd_info->fi_fd = symlink(request->path.old_path, request->path.path);
+ break;
+ case T_LINK:
+ fd_info->fi_fd = link(request->path.old_path, request->path.path);
+ break;
+ default:
+ fd_info->fi_fd = mknod(request->path.path, request->data.mode,
+ request->data.dev);
+ break;
+ }
+
+ if (fd_info->fi_fd < 0) {
+ fd_info->fi_error = errno;
+ } else {
+ fd_info->fi_error = 0;
+ fd_info->fi_fd = 0;
+ }
+
+ setfsgid(cur_gid);
+unset_uid:
+ setfsuid(cur_uid);
+}
+
static int chroot_daemonize(int chroot_sock)
{
sigset_t sigset;
@@ -226,6 +279,12 @@ int v9fs_chroot(FsContext *fs_ctx)
case T_CREATE:
chroot_do_create(&request, &fd_info);
break;
+ case T_MKDIR:
+ case T_SYMLINK:
+ case T_LINK:
+ case T_MKNOD:
+ chroot_do_create_special(&request, &fd_info);
+ break;
default:
fd_info.fi_fd = 0;
fd_info.fi_error = EIO;
diff --git a/hw/9pfs/virtio-9p-chroot.h b/hw/9pfs/virtio-9p-chroot.h
index 057dcde..7ae1389 100644
--- a/hw/9pfs/virtio-9p-chroot.h
+++ b/hw/9pfs/virtio-9p-chroot.h
@@ -54,5 +54,7 @@ typedef struct V9fsFileObjectRequest
int v9fs_chroot(FsContext *fs_ctx);
int v9fs_request(FsContext *fs_ctx, V9fsFileObjectRequest *or, int *error);
+int v9fs_create_special(FsContext *fs_ctx,
+ V9fsFileObjectRequest *request, int *error);
#endif /* _QEMU_VIRTIO_9P_CHROOT_H */
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 51911dd..9975ed1 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -69,6 +69,39 @@ static int passthrough_create(FsContext *fs_ctx, const char *path, int flags,
return fd;
}
+static int passthrough_create_special(FsContext *fs_ctx, const char *oldpath,
+ const char *path, FsCred *credp, int type)
+{
+ V9fsFileObjectRequest request;
+ int retval, error = 0;
+
+ fill_request(&request, path, credp);
+ switch (type) {
+ case T_MKNOD:
+ request.data.type = T_MKNOD;
+ break;
+ case T_MKDIR:
+ request.data.type = T_MKDIR;
+ break;
+ case T_SYMLINK:
+ request.data.type = T_SYMLINK;
+ break;
+ case T_LINK:
+ request.data.type = T_LINK;
+ break;
+ }
+ if (oldpath) {
+ request.data.oldpath_len = strlen(oldpath);
+ strcpy(request.path.old_path, oldpath);
+ }
+ retval = v9fs_create_special(fs_ctx, &request, &error);
+ if (retval < 0) {
+ errno = error;
+ return 0;
+ }
+ return retval;
+}
+
static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
{
int err;
@@ -286,8 +319,7 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
serrno = errno;
goto err_end;
}
- } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
- (fs_ctx->fs_sm == SM_NONE)) {
+ } else if (fs_ctx->fs_sm == SM_NONE) {
err = mknod(rpath(fs_ctx, path), credp->fc_mode, credp->fc_rdev);
if (err == -1) {
return err;
@@ -297,6 +329,12 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
serrno = errno;
goto err_end;
}
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ err = passthrough_create_special(fs_ctx, NULL, path, credp, T_MKNOD);
+ if (err < 0) {
+ serrno = errno;
+ goto err_end;
+ }
}
return err;
@@ -323,8 +361,7 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
serrno = errno;
goto err_end;
}
- } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
- (fs_ctx->fs_sm == SM_NONE)) {
+ } else if (fs_ctx->fs_sm == SM_NONE) {
err = mkdir(rpath(fs_ctx, path), credp->fc_mode);
if (err == -1) {
return err;
@@ -334,6 +371,12 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
serrno = errno;
goto err_end;
}
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ err = passthrough_create_special(fs_ctx, NULL, path, credp, T_MKDIR);
+ if (err < 0) {
+ serrno = errno;
+ goto err_end;
+ }
}
return err;
@@ -451,23 +494,19 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath,
serrno = errno;
goto err_end;
}
- } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
- (fs_ctx->fs_sm == SM_NONE)) {
+ } else if (fs_ctx->fs_sm == SM_NONE) {
err = symlink(oldpath, rpath(fs_ctx, newpath));
if (err) {
return err;
}
err = lchown(rpath(fs_ctx, newpath), credp->fc_uid, credp->fc_gid);
- if (err == -1) {
- /*
- * If we fail to change ownership and if we are
- * using security model none. Ignore the error
- */
- if (fs_ctx->fs_sm != SM_NONE) {
- serrno = errno;
- goto err_end;
- } else
- err = 0;
+ err = 0;
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ err = passthrough_create_special(fs_ctx, oldpath, newpath, credp,
+ T_SYMLINK);
+ if (err < 0) {
+ serrno = errno;
+ goto err_end;
}
}
return err;
@@ -478,24 +517,27 @@ err_end:
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)
{
- char *tmp = qemu_strdup(rpath(ctx, oldpath));
int err, serrno = 0;
- if (tmp == NULL) {
- return -ENOMEM;
- }
-
- err = link(tmp, rpath(ctx, newpath));
- if (err == -1) {
- serrno = errno;
- }
-
- qemu_free(tmp);
-
- if (err == -1) {
- errno = serrno;
+ if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ err = passthrough_create_special(fs_ctx, oldpath, newpath, NULL,
+ T_LINK);
+ if (err < 0) {
+ serrno = errno;
+ }
+ } else {
+ char *tmp = qemu_strdup(rpath(fs_ctx, oldpath));
+ if (tmp == NULL) {
+ return -ENOMEM;
+ }
+ err = link(tmp, rpath(fs_ctx, newpath));
+ if (err == -1) {
+ serrno = errno;
+ }
+ qemu_free(tmp);
}
return err;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [V5 PATCH 6/8] virtio-9p: Support for creating special files
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 6/8] virtio-9p: Support for creating special files M. Mohan Kumar
@ 2011-02-17 10:49 ` Stefan Hajnoczi
2011-02-18 5:58 ` M. Mohan Kumar
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2011-02-17 10:49 UTC (permalink / raw)
To: M. Mohan Kumar; +Cc: blauwirbel, qemu-devel
On Wed, Feb 16, 2011 at 12:23 PM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> + switch (type) {
> + case T_MKNOD:
> + request.data.type = T_MKNOD;
> + break;
> + case T_MKDIR:
> + request.data.type = T_MKDIR;
> + break;
> + case T_SYMLINK:
> + request.data.type = T_SYMLINK;
> + break;
> + case T_LINK:
> + request.data.type = T_LINK;
> + break;
> + }
Or just request.data.type = type?
> + if (oldpath) {
> + request.data.oldpath_len = strlen(oldpath);
> + strcpy(request.path.old_path, oldpath);
It's not obvious that this strcpy() is safe.
> + }
> + retval = v9fs_create_special(fs_ctx, &request, &error);
> + if (retval < 0) {
> + errno = error;
> + return 0;
This looks suspicious. Should the return value be negative on error?
> + char *tmp = qemu_strdup(rpath(fs_ctx, oldpath));
> + if (tmp == NULL) {
QEMU strdup never returns NULL.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] Re: [V5 PATCH 6/8] virtio-9p: Support for creating special files
2011-02-17 10:49 ` [Qemu-devel] " Stefan Hajnoczi
@ 2011-02-18 5:58 ` M. Mohan Kumar
0 siblings, 0 replies; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-18 5:58 UTC (permalink / raw)
To: qemu-devel; +Cc: blauwirbel, Stefan Hajnoczi
On Thursday 17 February 2011 4:19:17 pm Stefan Hajnoczi wrote:
> On Wed, Feb 16, 2011 at 12:23 PM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> > + switch (type) {
> > + case T_MKNOD:
> > + request.data.type = T_MKNOD;
> > + break;
> > + case T_MKDIR:
> > + request.data.type = T_MKDIR;
> > + break;
> > + case T_SYMLINK:
> > + request.data.type = T_SYMLINK;
> > + break;
> > + case T_LINK:
> > + request.data.type = T_LINK;
> > + break;
> > + }
>
> Or just request.data.type = type?
>
Oops, I will modify.
> > + if (oldpath) {
> > + request.data.oldpath_len = strlen(oldpath);
> > + strcpy(request.path.old_path, oldpath);
>
> It's not obvious that this strcpy() is safe.
>
I will change the code.
> > + }
> > + retval = v9fs_create_special(fs_ctx, &request, &error);
> > + if (retval < 0) {
> > + errno = error;
> > + return 0;
>
> This looks suspicious. Should the return value be negative on error?
Yes, it should return negative on error, I will update.
>
> > + char *tmp = qemu_strdup(rpath(fs_ctx, oldpath));
> > + if (tmp == NULL) {
>
> QEMU strdup never returns NULL.
Ok.
>
> Stefan
----
M. Mohan Kumar
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [V5 PATCH 7/8] virtio-9p: Move file post creation changes to none security model
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
` (5 preceding siblings ...)
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 6/8] virtio-9p: Support for creating special files M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 8/8] virtio-9p: Chroot environment for other functions M. Mohan Kumar
7 siblings, 0 replies; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
After creating a file object, its permission and ownership details are updated
as per client's request for both passthrough and none security model. But with
chrooted environment its not required for passthrough security model. Move all
post file creation changes to none security model
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
hw/9pfs/virtio-9p-local.c | 19 ++++++-------------
1 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 9975ed1..9d2d31c 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -169,21 +169,14 @@ static int local_set_xattr(const char *path, FsCred *credp)
return 0;
}
-static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
+static int local_post_create_none(FsContext *fs_ctx, const char *path,
FsCred *credp)
{
+ int retval;
if (chmod(rpath(fs_ctx, path), credp->fc_mode & 07777) < 0) {
return -1;
}
- if (lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid) < 0) {
- /*
- * If we fail to change ownership and if we are
- * using security model none. Ignore the error
- */
- if (fs_ctx->fs_sm != SM_NONE) {
- return -1;
- }
- }
+ retval = lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
return 0;
}
@@ -324,7 +317,7 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp)
if (err == -1) {
return err;
}
- err = local_post_create_passthrough(fs_ctx, path, credp);
+ err = local_post_create_none(fs_ctx, path, credp);
if (err == -1) {
serrno = errno;
goto err_end;
@@ -366,7 +359,7 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp)
if (err == -1) {
return err;
}
- err = local_post_create_passthrough(fs_ctx, path, credp);
+ err = local_post_create_none(fs_ctx, path, credp);
if (err == -1) {
serrno = errno;
goto err_end;
@@ -441,7 +434,7 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags,
if (fd == -1) {
return fd;
}
- err = local_post_create_passthrough(fs_ctx, path, credp);
+ err = local_post_create_none(fs_ctx, path, credp);
if (err == -1) {
serrno = errno;
goto err_end;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [V5 PATCH 8/8] virtio-9p: Chroot environment for other functions
2011-02-16 12:23 [Qemu-devel] [V5 PATCH 0/8] virtio-9p: Use chroot to safely access files in passthrough security model M. Mohan Kumar
` (6 preceding siblings ...)
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 7/8] virtio-9p: Move file post creation changes to none security model M. Mohan Kumar
@ 2011-02-16 12:23 ` M. Mohan Kumar
2011-02-17 11:02 ` [Qemu-devel] " Stefan Hajnoczi
7 siblings, 1 reply; 14+ messages in thread
From: M. Mohan Kumar @ 2011-02-16 12:23 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Daniel P. Berrange, blauwirbel
Add chroot functionality for systemcalls that can operate on a file
using relative directory file descriptor.
Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
hw/9pfs/virtio-9p-local.c | 222 +++++++++++++++++++++++++++++++++++++++------
1 files changed, 192 insertions(+), 30 deletions(-)
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 9d2d31c..357ad4e 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -22,6 +22,7 @@
#include <sys/socket.h>
#include <sys/un.h>
#include <attr/xattr.h>
+#include <libgen.h>
/* Helper routine to fill V9fsFileObjectRequest structure */
static void fill_request(V9fsFileObjectRequest *request, const char *path,
@@ -102,14 +103,35 @@ static int passthrough_create_special(FsContext *fs_ctx, const char *oldpath,
return retval;
}
+/*
+ * Returns file descriptor of dirname(path)
+ * This fd can be used by *at functions
+ */
+static int get_dirfd(FsContext *fs_ctx, const char *path)
+{
+ V9fsFileObjectRequest request;
+ int fd, error = 0;
+ char *dpath = qemu_strdup(path);
+
+ fill_request(&request, dirname(dpath), NULL);
+ request.data.type = T_OPEN;
+ fd = v9fs_request(fs_ctx, &request, &error);
+ if (fd < 0) {
+ errno = error;
+ }
+ qemu_free(dpath);
+ return fd;
+}
+
static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
{
int err;
- err = lstat(rpath(fs_ctx, path), stbuf);
- if (err) {
- return err;
- }
+
if (fs_ctx->fs_sm == SM_MAPPED) {
+ err = lstat(rpath(fs_ctx, path), stbuf);
+ if (err) {
+ return err;
+ }
/* Actual credentials are part of extended attrs */
uid_t tmp_uid;
gid_t tmp_gid;
@@ -131,6 +153,27 @@ static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
sizeof(dev_t)) > 0) {
stbuf->st_rdev = tmp_dev;
}
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ int pfd, serrno = 0;
+ char *tmp_path;
+
+ pfd = get_dirfd(fs_ctx, path);
+ if (pfd < 0) {
+ return -1;
+ }
+ tmp_path = qemu_strdup(path);
+ err = fstatat(pfd, basename(tmp_path), stbuf, AT_SYMLINK_NOFOLLOW);
+ if (err < 0) {
+ serrno = errno;
+ }
+ close(pfd);
+ qemu_free(tmp_path);
+ errno = serrno;
+ } else {
+ err = lstat(rpath(fs_ctx, path), stbuf);
+ if (err) {
+ return err;
+ }
}
return err;
}
@@ -195,9 +238,23 @@ static ssize_t local_readlink(FsContext *fs_ctx, const char *path,
} while (tsize == -1 && errno == EINTR);
close(fd);
return tsize;
- } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
- (fs_ctx->fs_sm == SM_NONE)) {
+ } else if (fs_ctx->fs_sm == SM_NONE) {
tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ int pfd, serrno = 0;
+ char *tmp_path;
+ pfd = get_dirfd(fs_ctx, path);
+ if (pfd < 0) {
+ return -1;
+ }
+ tmp_path = qemu_strdup(path);
+ tsize = readlinkat(pfd, basename(tmp_path), buf, bufsz);
+ if (tsize < 0) {
+ serrno = 0;
+ }
+ close(pfd);
+ qemu_free(tmp_path);
+ errno = serrno;
}
return tsize;
}
@@ -289,8 +346,23 @@ static int local_chmod(FsContext *fs_ctx, const char *path, FsCred *credp)
{
if (fs_ctx->fs_sm == SM_MAPPED) {
return local_set_xattr(rpath(fs_ctx, path), credp);
- } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
- (fs_ctx->fs_sm == SM_NONE)) {
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ int pfd, err, serrno = 0;
+ char *tmp_path;
+ pfd = get_dirfd(fs_ctx, path);
+ if (pfd < 0) {
+ return -1;
+ }
+ tmp_path = qemu_strdup(path);
+ err = fchmodat(pfd, basename(tmp_path), credp->fc_mode, 0);
+ if (err == -1) {
+ serrno = errno;
+ }
+ qemu_free(tmp_path);
+ close(pfd);
+ errno = serrno;
+ return err;
+ } else if (fs_ctx->fs_sm == SM_NONE) {
return chmod(rpath(fs_ctx, path), credp->fc_mode);
}
return -1;
@@ -538,53 +610,143 @@ static int local_link(FsContext *fs_ctx, const char *oldpath,
static int local_truncate(FsContext *ctx, const char *path, off_t size)
{
- return truncate(rpath(ctx, path), size);
+ if (ctx->fs_sm == SM_PASSTHROUGH) {
+ int fd, retval;
+ fd = passthrough_open(ctx, path, O_RDWR);
+ if (fd < 0) {
+ return fd;
+ }
+ retval = ftruncate(fd, size);
+ close(fd);
+ return retval;
+ } else {
+ return truncate(rpath(ctx, path), size);
+ }
}
static int local_rename(FsContext *ctx, const char *oldpath,
const char *newpath)
{
- char *tmp;
- int err;
-
- tmp = qemu_strdup(rpath(ctx, oldpath));
+ int err, serrno = 0;
- err = rename(tmp, rpath(ctx, newpath));
- if (err == -1) {
- int serrno = errno;
- qemu_free(tmp);
+ if (ctx->fs_sm == SM_PASSTHROUGH) {
+ int opfd, npfd;
+ char *old_tmppath, *new_tmppath;
+ opfd = get_dirfd(ctx, oldpath);
+ if (opfd < 0) {
+ return -1;
+ }
+ npfd = get_dirfd(ctx, newpath);
+ if (npfd < 0) {
+ close(opfd);
+ return -1;
+ }
+ old_tmppath = qemu_strdup(oldpath);
+ new_tmppath = qemu_strdup(newpath);
+ err = renameat(opfd, basename(old_tmppath),
+ npfd, basename(new_tmppath));
+ if (err == -1) {
+ serrno = errno;
+ }
+ close(npfd);
+ close(opfd);
+ qemu_free(old_tmppath);
+ qemu_free(new_tmppath);
errno = serrno;
} else {
- qemu_free(tmp);
+ char *tmp;
+ tmp = qemu_strdup(rpath(ctx, oldpath));
+
+ err = rename(tmp, rpath(ctx, newpath));
+ if (err == -1) {
+ int serrno = errno;
+ qemu_free(tmp);
+ errno = serrno;
+ } else {
+ qemu_free(tmp);
+ }
}
return err;
-
}
static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
{
- if ((credp->fc_uid == -1 && credp->fc_gid == -1) ||
- (fs_ctx->fs_sm == SM_PASSTHROUGH)) {
+ if (fs_ctx->fs_sm != SM_PASSTHROUGH &&
+ (credp->fc_uid == -1 && credp->fc_gid == -1)) {
+ return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
+ } else if (fs_ctx->fs_sm == SM_NONE) {
return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
} else if (fs_ctx->fs_sm == SM_MAPPED) {
return local_set_xattr(rpath(fs_ctx, path), credp);
- } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
- (fs_ctx->fs_sm == SM_NONE)) {
- return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
+ } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ int pfd, err, serrno = 0;
+ char *old_path;
+ pfd = get_dirfd(fs_ctx, path);
+ if (pfd < 0) {
+ return -1;
+ }
+ old_path = qemu_strdup(path);
+ err = fchownat(pfd, basename(old_path), credp->fc_uid, credp->fc_gid,
+ AT_SYMLINK_NOFOLLOW);
+ if (err == -1) {
+ serrno = errno;
+ }
+ qemu_free(old_path);
+ close(pfd);
+ errno = serrno;
+ return err;
}
return -1;
}
-static int local_utimensat(FsContext *s, const char *path,
- const struct timespec *buf)
+static int local_utimensat(FsContext *fs_ctx, const char *path,
+ const struct timespec *buf)
{
- return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+ if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ int fd, retval;
+ fd = passthrough_open(fs_ctx, path, O_RDONLY | O_NONBLOCK);
+ retval = futimens(fd, buf);
+ close(fd);
+ return retval;
+ } else {
+ return utimensat(AT_FDCWD, rpath(fs_ctx, path), buf,
+ AT_SYMLINK_NOFOLLOW);
+ }
}
-static int local_remove(FsContext *ctx, const char *path)
-{
- return remove(rpath(ctx, path));
+static int local_remove(FsContext *fs_ctx, const char *path)
+ {
+ if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+ int pfd, err, serrno, flags;
+ char *old_path;
+ struct stat stbuf;
+ pfd = get_dirfd(fs_ctx, path);
+ if (pfd < 0) {
+ return -1;
+ }
+ old_path = qemu_strdup(path);
+ err = fstatat(pfd, basename(old_path), &stbuf, AT_SYMLINK_NOFOLLOW);
+ if (err < 0) {
+ return -1;
+ }
+ serrno = flags = 0;
+ if ((stbuf.st_mode & S_IFMT) == S_IFDIR) {
+ flags = AT_REMOVEDIR;
+ } else {
+ flags = 0;
+ }
+ err = unlinkat(pfd, basename(old_path), flags);
+ if (err == -1) {
+ serrno = errno;
+ }
+ qemu_free(old_path);
+ close(pfd);
+ errno = serrno;
+ return err;
+ } else {
+ return remove(rpath(fs_ctx, path));
+ }
}
static int local_fsync(FsContext *ctx, int fd, int datasync)
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [V5 PATCH 8/8] virtio-9p: Chroot environment for other functions
2011-02-16 12:23 ` [Qemu-devel] [V5 PATCH 8/8] virtio-9p: Chroot environment for other functions M. Mohan Kumar
@ 2011-02-17 11:02 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2011-02-17 11:02 UTC (permalink / raw)
To: M. Mohan Kumar; +Cc: blauwirbel, qemu-devel
On Wed, Feb 16, 2011 at 12:23 PM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +/*
> + * Returns file descriptor of dirname(path)
> + * This fd can be used by *at functions
> + */
> +static int get_dirfd(FsContext *fs_ctx, const char *path)
> +{
> + V9fsFileObjectRequest request;
> + int fd, error = 0;
> + char *dpath = qemu_strdup(path);
> +
> + fill_request(&request, dirname(dpath), NULL);
> + request.data.type = T_OPEN;
> + fd = v9fs_request(fs_ctx, &request, &error);
> + if (fd < 0) {
> + errno = error;
> + }
man errno says:
"a function that succeeds is allowed to change errno"
Therefore you can't safely use errno to stash your error value and
call any code outside your control because it may overwrite errno.
Obliging callers to take these precautions is a bad idea because they
will forget.
The main feedback I have for this entire patchset is that these
functions introduce layers and layers of weird error handling. A
large fraction of the code is just propagating error codes. Sometimes
errno is used, sometimes an int * argument is used, sometime a
positive errno is returned although QEMU code normally returns a
negative errno. Then the fd_info struct has a flag to indicate the fd
is invalid when we could just set it to -1 to indicate this. It's
messy and hard for someone else to modify later without introducing
error handling bugs.
The method that fits most with QEMU's codebase is:
int foo(...) /* returns 0 on success, -errno on failure */
If you can refactor the code to unify error handling it will require
fewer lines and be simpler.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread