[PATCH v2 0/5] virtiofsd: Add a unprivileged passthrough mode

[PATCH v2 0/5] virtiofsd: Add a unprivileged passthrough mode

[Vivek Goyal]

Hi,

This is V2 of patches. Only change since last version is handling of
lock/pid file creation as per the comments from Daniel Berrange.

I can't think of any more changes needed. As a unpriviliged user
inside VM I can do simple operations like create/remove/read/write
files.

For more testing, I probably need a testsuite which runs as unpriviliged
user. pjdfstests needs to run as root and this does not work in this
setup as creation of files as root fails. (On host, daemon tries to
switch to root uid and that fails).

So as of now, I think these are the minimum changes needed to support
unprivileged passthrough mode of virtiofsd.

Thanks
Vivek

Vivek Goyal (5):
  virtiofsd: Add notion of unprivileged mode
  virtiofsd: create lock/pid file in per user cache dir
  virtiofsd: open /proc/self/fd/ in sandbox=NONE mode
  virtiofsd: Open lo->source while setting up root in sandbox=NONE mode
  virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

 tools/virtiofsd/fuse_virtio.c    | 15 ++++++++++++++-
 tools/virtiofsd/passthrough_ll.c | 29 ++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 4 deletions(-)

-- 
2.25.4

[Virtio-fs] [PATCH v2 0/5] virtiofsd: Add a unprivileged passthrough mode

[Vivek Goyal]

Hi,

This is V2 of patches. Only change since last version is handling of
lock/pid file creation as per the comments from Daniel Berrange.

I can't think of any more changes needed. As a unpriviliged user
inside VM I can do simple operations like create/remove/read/write
files.

For more testing, I probably need a testsuite which runs as unpriviliged
user. pjdfstests needs to run as root and this does not work in this
setup as creation of files as root fails. (On host, daemon tries to
switch to root uid and that fails).

So as of now, I think these are the minimum changes needed to support
unprivileged passthrough mode of virtiofsd.

Thanks
Vivek

Vivek Goyal (5):
  virtiofsd: Add notion of unprivileged mode
  virtiofsd: create lock/pid file in per user cache dir
  virtiofsd: open /proc/self/fd/ in sandbox=NONE mode
  virtiofsd: Open lo->source while setting up root in sandbox=NONE mode
  virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

 tools/virtiofsd/fuse_virtio.c    | 15 ++++++++++++++-
 tools/virtiofsd/passthrough_ll.c | 29 ++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 4 deletions(-)

-- 
2.25.4

[PATCH v2 1/5] virtiofsd: Add notion of unprivileged mode

[Vivek Goyal]

At startup if we are running as non-root user, then internally set
unpriviliged mode set. Also add a notion of sandbox NONE and set
that internally in unprivileged mode. setting up namespaces and
chroot() fails in unpriviliged mode.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e2fbc614fd..cd91c4a831 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -147,11 +147,13 @@ enum {
 enum {
     SANDBOX_NAMESPACE,
     SANDBOX_CHROOT,
+    SANDBOX_NONE,
 };
 
 struct lo_data {
     pthread_mutex_t mutex;
     int sandbox;
+    bool unprivileged;
     int debug;
     int writeback;
     int flock;
@@ -3288,6 +3290,12 @@ int main(int argc, char *argv[])
     lo_map_init(&lo.dirp_map);
     lo_map_init(&lo.fd_map);
 
+    if (geteuid() != 0) {
+       lo.unprivileged = true;
+       lo.sandbox = SANDBOX_NONE;
+       fuse_log(FUSE_LOG_DEBUG, "Running in unprivileged passthrough mode.\n");
+    }
+
     if (fuse_parse_cmdline(&args, &opts) != 0) {
         goto err_out1;
     }
-- 
2.25.4

[Virtio-fs] [PATCH v2 1/5] virtiofsd: Add notion of unprivileged mode

[Vivek Goyal]

At startup if we are running as non-root user, then internally set
unpriviliged mode set. Also add a notion of sandbox NONE and set
that internally in unprivileged mode. setting up namespaces and
chroot() fails in unpriviliged mode.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e2fbc614fd..cd91c4a831 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -147,11 +147,13 @@ enum {
 enum {
     SANDBOX_NAMESPACE,
     SANDBOX_CHROOT,
+    SANDBOX_NONE,
 };
 
 struct lo_data {
     pthread_mutex_t mutex;
     int sandbox;
+    bool unprivileged;
     int debug;
     int writeback;
     int flock;
@@ -3288,6 +3290,12 @@ int main(int argc, char *argv[])
     lo_map_init(&lo.dirp_map);
     lo_map_init(&lo.fd_map);
 
+    if (geteuid() != 0) {
+       lo.unprivileged = true;
+       lo.sandbox = SANDBOX_NONE;
+       fuse_log(FUSE_LOG_DEBUG, "Running in unprivileged passthrough mode.\n");
+    }
+
     if (fuse_parse_cmdline(&args, &opts) != 0) {
         goto err_out1;
     }
-- 
2.25.4

Re: [PATCH v2 1/5] virtiofsd: Add notion of unprivileged mode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> At startup if we are running as non-root user, then internally set
> unpriviliged mode set. Also add a notion of sandbox NONE and set
> that internally in unprivileged mode. setting up namespaces and
> chroot() fails in unpriviliged mode.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index e2fbc614fd..cd91c4a831 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -147,11 +147,13 @@ enum {
>  enum {
>      SANDBOX_NAMESPACE,
>      SANDBOX_CHROOT,
> +    SANDBOX_NONE,
>  };
>  
>  struct lo_data {
>      pthread_mutex_t mutex;
>      int sandbox;
> +    bool unprivileged;
>      int debug;
>      int writeback;
>      int flock;
> @@ -3288,6 +3290,12 @@ int main(int argc, char *argv[])
>      lo_map_init(&lo.dirp_map);
>      lo_map_init(&lo.fd_map);
>  
> +    if (geteuid() != 0) {
> +       lo.unprivileged = true;
> +       lo.sandbox = SANDBOX_NONE;
> +       fuse_log(FUSE_LOG_DEBUG, "Running in unprivileged passthrough mode.\n");
> +    }

I don't like this being automatic; to switch to a less secure mode, the
user should have to explicitly ask for it; we don't want people
accidentally ending up with less security.

Also, I'm not sure that checking for geteuid() != 0  is the right test -
wouldn't the current virtiofsd run with a non-root user as long
as it had the correct capabilities?

I was doing to suggest we match cloud-hypervisor where we added
--disable-sandbox; but with this we now have a trinary switch;
so sandbox=none is probably the right way.

Dave

>      if (fuse_parse_cmdline(&args, &opts) != 0) {
>          goto err_out1;
>      }
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v2 1/5] virtiofsd: Add notion of unprivileged mode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> At startup if we are running as non-root user, then internally set
> unpriviliged mode set. Also add a notion of sandbox NONE and set
> that internally in unprivileged mode. setting up namespaces and
> chroot() fails in unpriviliged mode.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index e2fbc614fd..cd91c4a831 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -147,11 +147,13 @@ enum {
>  enum {
>      SANDBOX_NAMESPACE,
>      SANDBOX_CHROOT,
> +    SANDBOX_NONE,
>  };
>  
>  struct lo_data {
>      pthread_mutex_t mutex;
>      int sandbox;
> +    bool unprivileged;
>      int debug;
>      int writeback;
>      int flock;
> @@ -3288,6 +3290,12 @@ int main(int argc, char *argv[])
>      lo_map_init(&lo.dirp_map);
>      lo_map_init(&lo.fd_map);
>  
> +    if (geteuid() != 0) {
> +       lo.unprivileged = true;
> +       lo.sandbox = SANDBOX_NONE;
> +       fuse_log(FUSE_LOG_DEBUG, "Running in unprivileged passthrough mode.\n");
> +    }

I don't like this being automatic; to switch to a less secure mode, the
user should have to explicitly ask for it; we don't want people
accidentally ending up with less security.

Also, I'm not sure that checking for geteuid() != 0  is the right test -
wouldn't the current virtiofsd run with a non-root user as long
as it had the correct capabilities?

I was doing to suggest we match cloud-hypervisor where we added
--disable-sandbox; but with this we now have a trinary switch;
so sandbox=none is probably the right way.

Dave

>      if (fuse_parse_cmdline(&args, &opts) != 0) {
>          goto err_out1;
>      }
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v2 2/5] virtiofsd: create lock/pid file in per user cache dir

[Vivek Goyal]

Right now we create lock/pid file in /usr/local/var/... and unprivliged
user does not have access to create files there.

Hence, in unprivileged mode, create this file in per user cache dir
as specified by environment variable XDG_RUNTIME_DIR.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 6b21a93841..1551a94757 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -972,8 +972,21 @@ static bool fv_socket_lock(struct fuse_session *se)
     g_autofree gchar *pidfile = NULL;
     g_autofree gchar *dir = NULL;
     Error *local_err = NULL;
+    gboolean unprivileged = false;
 
-    dir = qemu_get_local_state_pathname("run/virtiofsd");
+    if (geteuid() != 0)
+        unprivileged = true;
+
+    /*
+     * Unpriviliged users don't have access to /usr/local/var. Hence
+     * store lock/pid file in per user cache directory. Use environment
+     * variable XDG_RUNTIME_DIR.
+     */
+    if (unprivileged) {
+        dir = g_strdup_printf("%s/virtiofsd", g_get_user_runtime_dir());
+    } else {
+        dir = qemu_get_local_state_pathname("run/virtiofsd");
+    }
 
     if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
         fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
-- 
2.25.4

[Virtio-fs] [PATCH v2 2/5] virtiofsd: create lock/pid file in per user cache dir

[Vivek Goyal]

Right now we create lock/pid file in /usr/local/var/... and unprivliged
user does not have access to create files there.

Hence, in unprivileged mode, create this file in per user cache dir
as specified by environment variable XDG_RUNTIME_DIR.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 6b21a93841..1551a94757 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -972,8 +972,21 @@ static bool fv_socket_lock(struct fuse_session *se)
     g_autofree gchar *pidfile = NULL;
     g_autofree gchar *dir = NULL;
     Error *local_err = NULL;
+    gboolean unprivileged = false;
 
-    dir = qemu_get_local_state_pathname("run/virtiofsd");
+    if (geteuid() != 0)
+        unprivileged = true;
+
+    /*
+     * Unpriviliged users don't have access to /usr/local/var. Hence
+     * store lock/pid file in per user cache directory. Use environment
+     * variable XDG_RUNTIME_DIR.
+     */
+    if (unprivileged) {
+        dir = g_strdup_printf("%s/virtiofsd", g_get_user_runtime_dir());
+    } else {
+        dir = qemu_get_local_state_pathname("run/virtiofsd");
+    }
 
     if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
         fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
-- 
2.25.4

Re: [PATCH v2 2/5] virtiofsd: create lock/pid file in per user cache dir

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Right now we create lock/pid file in /usr/local/var/... and unprivliged
> user does not have access to create files there.

I *think* the /usr/local there is coming from the build config of your
qemu; but I'm not 100% sure whether it's just it's --preifx

> Hence, in unprivileged mode, create this file in per user cache dir
> as specified by environment variable XDG_RUNTIME_DIR.

Yes; it's interesting that qemu daemons are somewhat inconsistent in
this; most of them ask for a --pidfile to say where you want it;
but not all.

> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/fuse_virtio.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 6b21a93841..1551a94757 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -972,8 +972,21 @@ static bool fv_socket_lock(struct fuse_session *se)
>      g_autofree gchar *pidfile = NULL;
>      g_autofree gchar *dir = NULL;
>      Error *local_err = NULL;
> +    gboolean unprivileged = false;
>  
> -    dir = qemu_get_local_state_pathname("run/virtiofsd");
> +    if (geteuid() != 0)
> +        unprivileged = true;

Note qemu style guides need {'s on that.

> +    /*
> +     * Unpriviliged users don't have access to /usr/local/var. Hence
> +     * store lock/pid file in per user cache directory. Use environment
> +     * variable XDG_RUNTIME_DIR.
> +     */
> +    if (unprivileged) {
> +        dir = g_strdup_printf("%s/virtiofsd", g_get_user_runtime_dir());
> +    } else {
> +        dir = qemu_get_local_state_pathname("run/virtiofsd");
> +    }

Yeh that's OK, so with the  { fixed;

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

A few other possible thoughts:
  a) Just always use g_get_runtime_dir as the top; even for root - it
seems to come out as /root/.cache for root, which isn't terrible.
  b) Maybe put this code in qemu_get_local_state_pathname?

Dave


>  
>      if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
>          fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v2 2/5] virtiofsd: create lock/pid file in per user cache dir

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Right now we create lock/pid file in /usr/local/var/... and unprivliged
> user does not have access to create files there.

I *think* the /usr/local there is coming from the build config of your
qemu; but I'm not 100% sure whether it's just it's --preifx

> Hence, in unprivileged mode, create this file in per user cache dir
> as specified by environment variable XDG_RUNTIME_DIR.

Yes; it's interesting that qemu daemons are somewhat inconsistent in
this; most of them ask for a --pidfile to say where you want it;
but not all.

> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/fuse_virtio.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 6b21a93841..1551a94757 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -972,8 +972,21 @@ static bool fv_socket_lock(struct fuse_session *se)
>      g_autofree gchar *pidfile = NULL;
>      g_autofree gchar *dir = NULL;
>      Error *local_err = NULL;
> +    gboolean unprivileged = false;
>  
> -    dir = qemu_get_local_state_pathname("run/virtiofsd");
> +    if (geteuid() != 0)
> +        unprivileged = true;

Note qemu style guides need {'s on that.

> +    /*
> +     * Unpriviliged users don't have access to /usr/local/var. Hence
> +     * store lock/pid file in per user cache directory. Use environment
> +     * variable XDG_RUNTIME_DIR.
> +     */
> +    if (unprivileged) {
> +        dir = g_strdup_printf("%s/virtiofsd", g_get_user_runtime_dir());
> +    } else {
> +        dir = qemu_get_local_state_pathname("run/virtiofsd");
> +    }

Yeh that's OK, so with the  { fixed;

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

A few other possible thoughts:
  a) Just always use g_get_runtime_dir as the top; even for root - it
seems to come out as /root/.cache for root, which isn't terrible.
  b) Maybe put this code in qemu_get_local_state_pathname?

Dave


>  
>      if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
>          fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v2 3/5] virtiofsd: open /proc/self/fd/ in sandbox=NONE mode

[Vivek Goyal]

We need /proc/self/fd descriptor even in sandbox=NONE mode.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index cd91c4a831..76ef891105 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2969,6 +2969,15 @@ static void setup_capabilities(char *modcaps_in)
     pthread_mutex_unlock(&cap.mutex);
 }
 
+static void setup_none(struct lo_data *lo)
+{
+    lo->proc_self_fd = open("/proc/self/fd", O_PATH);
+    if (lo->proc_self_fd == -1) {
+        fuse_log(FUSE_LOG_ERR, "open(\"/proc/self/fd\", O_PATH): %m\n");
+        exit(1);
+    }
+}
+
 /*
  * Use chroot as a weaker sandbox for environments where the process is
  * launched without CAP_SYS_ADMIN.
@@ -3014,8 +3023,10 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
     if (lo->sandbox == SANDBOX_NAMESPACE) {
         setup_namespaces(lo, se);
         setup_mounts(lo->source);
-    } else {
+    } else if (lo->sandbox == SANDBOX_CHROOT) {
         setup_chroot(lo);
+    } else {
+        setup_none(lo);
     }
 
     setup_seccomp(enable_syslog);
-- 
2.25.4

[Virtio-fs] [PATCH v2 3/5] virtiofsd: open /proc/self/fd/ in sandbox=NONE mode

[Vivek Goyal]

We need /proc/self/fd descriptor even in sandbox=NONE mode.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index cd91c4a831..76ef891105 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2969,6 +2969,15 @@ static void setup_capabilities(char *modcaps_in)
     pthread_mutex_unlock(&cap.mutex);
 }
 
+static void setup_none(struct lo_data *lo)
+{
+    lo->proc_self_fd = open("/proc/self/fd", O_PATH);
+    if (lo->proc_self_fd == -1) {
+        fuse_log(FUSE_LOG_ERR, "open(\"/proc/self/fd\", O_PATH): %m\n");
+        exit(1);
+    }
+}
+
 /*
  * Use chroot as a weaker sandbox for environments where the process is
  * launched without CAP_SYS_ADMIN.
@@ -3014,8 +3023,10 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
     if (lo->sandbox == SANDBOX_NAMESPACE) {
         setup_namespaces(lo, se);
         setup_mounts(lo->source);
-    } else {
+    } else if (lo->sandbox == SANDBOX_CHROOT) {
         setup_chroot(lo);
+    } else {
+        setup_none(lo);
     }
 
     setup_seccomp(enable_syslog);
-- 
2.25.4

Re: [PATCH v2 3/5] virtiofsd: open /proc/self/fd/ in sandbox=NONE mode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> We need /proc/self/fd descriptor even in sandbox=NONE mode.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index cd91c4a831..76ef891105 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2969,6 +2969,15 @@ static void setup_capabilities(char *modcaps_in)
>      pthread_mutex_unlock(&cap.mutex);
>  }
>  
> +static void setup_none(struct lo_data *lo)

'setup_none' is not the most obvious name; setup_sandbox_none ?

> +{
> +    lo->proc_self_fd = open("/proc/self/fd", O_PATH);
> +    if (lo->proc_self_fd == -1) {
> +        fuse_log(FUSE_LOG_ERR, "open(\"/proc/self/fd\", O_PATH): %m\n");
> +        exit(1);
> +    }
> +}
> +
>  /*
>   * Use chroot as a weaker sandbox for environments where the process is
>   * launched without CAP_SYS_ADMIN.
> @@ -3014,8 +3023,10 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
>      if (lo->sandbox == SANDBOX_NAMESPACE) {
>          setup_namespaces(lo, se);
>          setup_mounts(lo->source);
> -    } else {
> +    } else if (lo->sandbox == SANDBOX_CHROOT) {
>          setup_chroot(lo);
> +    } else {
> +        setup_none(lo);
>      }
>  
>      setup_seccomp(enable_syslog);
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v2 3/5] virtiofsd: open /proc/self/fd/ in sandbox=NONE mode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> We need /proc/self/fd descriptor even in sandbox=NONE mode.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index cd91c4a831..76ef891105 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2969,6 +2969,15 @@ static void setup_capabilities(char *modcaps_in)
>      pthread_mutex_unlock(&cap.mutex);
>  }
>  
> +static void setup_none(struct lo_data *lo)

'setup_none' is not the most obvious name; setup_sandbox_none ?

> +{
> +    lo->proc_self_fd = open("/proc/self/fd", O_PATH);
> +    if (lo->proc_self_fd == -1) {
> +        fuse_log(FUSE_LOG_ERR, "open(\"/proc/self/fd\", O_PATH): %m\n");
> +        exit(1);
> +    }
> +}
> +
>  /*
>   * Use chroot as a weaker sandbox for environments where the process is
>   * launched without CAP_SYS_ADMIN.
> @@ -3014,8 +3023,10 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
>      if (lo->sandbox == SANDBOX_NAMESPACE) {
>          setup_namespaces(lo, se);
>          setup_mounts(lo->source);
> -    } else {
> +    } else if (lo->sandbox == SANDBOX_CHROOT) {
>          setup_chroot(lo);
> +    } else {
> +        setup_none(lo);
>      }
>  
>      setup_seccomp(enable_syslog);
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Vivek Goyal]

In sandbox=NONE mode, lo->source points to the directory which is being
exported. We have not done any chroot()/pivot_root(). So open lo->source.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 76ef891105..a6fa816b6c 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
     int fd, res;
     struct stat stat;
 
-    fd = open("/", O_PATH);
+    if (lo->sandbox == SANDBOX_NONE)
+        fd = open(lo->source, O_PATH);
+    else
+        fd = open("/", O_PATH);
     if (fd == -1) {
         fuse_log(FUSE_LOG_ERR, "open(%s, O_PATH): %m\n", lo->source);
         exit(1);
-- 
2.25.4

[Virtio-fs] [PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Vivek Goyal]

In sandbox=NONE mode, lo->source points to the directory which is being
exported. We have not done any chroot()/pivot_root(). So open lo->source.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 76ef891105..a6fa816b6c 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
     int fd, res;
     struct stat stat;
 
-    fd = open("/", O_PATH);
+    if (lo->sandbox == SANDBOX_NONE)
+        fd = open(lo->source, O_PATH);
+    else
+        fd = open("/", O_PATH);
     if (fd == -1) {
         fuse_log(FUSE_LOG_ERR, "open(%s, O_PATH): %m\n", lo->source);
         exit(1);
-- 
2.25.4

Re: [PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Stefan Hajnoczi]

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

On Thu, Jul 30, 2020 at 03:47:35PM -0400, Vivek Goyal wrote:
> In sandbox=NONE mode, lo->source points to the directory which is being
> exported. We have not done any chroot()/pivot_root(). So open lo->source.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 76ef891105..a6fa816b6c 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
>      int fd, res;
>      struct stat stat;
>  
> -    fd = open("/", O_PATH);
> +    if (lo->sandbox == SANDBOX_NONE)
> +        fd = open(lo->source, O_PATH);
> +    else
> +        fd = open("/", O_PATH);

Up until now virtiofsd has been able to assume that path traversal has
the shared directory as "/".

Now this is no longer true and it is necessary to audit all syscalls
that take path arguments. They must ensure that:
1. Path components are safe (no ".." or "/" allowed)
2. Symlinks are not followed.

Did you audit all syscalls made by passthrough_ll.c?

virtiofsd still needs to restrict the client to the shared directory for
two reasons:
1. The guest may not be trusted. An unprivileged sandbox=none mount can
   be used with a malicious guest.
2. If accidental escapes are possible then the guest could accidentally
   corrupt or delete files outside the shared directory.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Stefan Hajnoczi]

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

On Thu, Jul 30, 2020 at 03:47:35PM -0400, Vivek Goyal wrote:
> In sandbox=NONE mode, lo->source points to the directory which is being
> exported. We have not done any chroot()/pivot_root(). So open lo->source.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 76ef891105..a6fa816b6c 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
>      int fd, res;
>      struct stat stat;
>  
> -    fd = open("/", O_PATH);
> +    if (lo->sandbox == SANDBOX_NONE)
> +        fd = open(lo->source, O_PATH);
> +    else
> +        fd = open("/", O_PATH);

Up until now virtiofsd has been able to assume that path traversal has
the shared directory as "/".

Now this is no longer true and it is necessary to audit all syscalls
that take path arguments. They must ensure that:
1. Path components are safe (no ".." or "/" allowed)
2. Symlinks are not followed.

Did you audit all syscalls made by passthrough_ll.c?

virtiofsd still needs to restrict the client to the shared directory for
two reasons:
1. The guest may not be trusted. An unprivileged sandbox=none mount can
   be used with a malicious guest.
2. If accidental escapes are possible then the guest could accidentally
   corrupt or delete files outside the shared directory.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Vivek Goyal]

On Mon, Aug 03, 2020 at 10:54:59AM +0100, Stefan Hajnoczi wrote:
> On Thu, Jul 30, 2020 at 03:47:35PM -0400, Vivek Goyal wrote:
> > In sandbox=NONE mode, lo->source points to the directory which is being
> > exported. We have not done any chroot()/pivot_root(). So open lo->source.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 76ef891105..a6fa816b6c 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
> >      int fd, res;
> >      struct stat stat;
> >  
> > -    fd = open("/", O_PATH);
> > +    if (lo->sandbox == SANDBOX_NONE)
> > +        fd = open(lo->source, O_PATH);
> > +    else
> > +        fd = open("/", O_PATH);
> 
> Up until now virtiofsd has been able to assume that path traversal has
> the shared directory as "/".
> 
> Now this is no longer true and it is necessary to audit all syscalls
> that take path arguments. They must ensure that:
> 1. Path components are safe (no ".." or "/" allowed)
> 2. Symlinks are not followed.

This code does not change the path client is passing in and we are
already doing the checks on passed in paths/names. So existing checks
should work even for this case, isn't it?

lo_lookup() {
    if (strchr(name, '/')) {
        fuse_reply_err(req, EINVAL);
        return;
    }
}

lo_do_lookup() {
    /* Do not allow escaping root directory */
    if (dir == &lo->root && strcmp(name, "..") == 0) {
        name = ".";
    }
}

> 
> Did you audit all syscalls made by passthrough_ll.c?
> 
> virtiofsd still needs to restrict the client to the shared directory for
> two reasons:
> 1. The guest may not be trusted. An unprivileged sandbox=none mount can
>    be used with a malicious guest.
> 2. If accidental escapes are possible then the guest could accidentally
>    corrupt or delete files outside the shared directory.

Even if escape is possible, its no different than a malicious user
application running. Given sandbox=none can be used in case of
unpriviliged mode, that means user app can only affect files owned by
that user.

Given we are not doing chroot()/pivot_root(), key question will be
what additional path we are enabling using which one can escape
out of this shared directory.

Having said that, In case of unpriviliged mode, it feels that chroot()
is more of a nice to have kind of functionality. Even if guest manages
to break out of shared directory, impact is equivalent to user running a
malicious app directly. 

If doing chroot/pivot_root is must, then we need additional capabilities.
And that probably means we need to launch virtiofsd in a user namespace
with required caps. And that will fall back into the territory of
running virtiofsd in a user namespace. That indeed is an important
use case which needs to be solved.

Thanks
Vivek

Re: [Virtio-fs] [PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Vivek Goyal]

On Mon, Aug 03, 2020 at 10:54:59AM +0100, Stefan Hajnoczi wrote:
> On Thu, Jul 30, 2020 at 03:47:35PM -0400, Vivek Goyal wrote:
> > In sandbox=NONE mode, lo->source points to the directory which is being
> > exported. We have not done any chroot()/pivot_root(). So open lo->source.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 76ef891105..a6fa816b6c 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
> >      int fd, res;
> >      struct stat stat;
> >  
> > -    fd = open("/", O_PATH);
> > +    if (lo->sandbox == SANDBOX_NONE)
> > +        fd = open(lo->source, O_PATH);
> > +    else
> > +        fd = open("/", O_PATH);
> 
> Up until now virtiofsd has been able to assume that path traversal has
> the shared directory as "/".
> 
> Now this is no longer true and it is necessary to audit all syscalls
> that take path arguments. They must ensure that:
> 1. Path components are safe (no ".." or "/" allowed)
> 2. Symlinks are not followed.

This code does not change the path client is passing in and we are
already doing the checks on passed in paths/names. So existing checks
should work even for this case, isn't it?

lo_lookup() {
    if (strchr(name, '/')) {
        fuse_reply_err(req, EINVAL);
        return;
    }
}

lo_do_lookup() {
    /* Do not allow escaping root directory */
    if (dir == &lo->root && strcmp(name, "..") == 0) {
        name = ".";
    }
}

> 
> Did you audit all syscalls made by passthrough_ll.c?
> 
> virtiofsd still needs to restrict the client to the shared directory for
> two reasons:
> 1. The guest may not be trusted. An unprivileged sandbox=none mount can
>    be used with a malicious guest.
> 2. If accidental escapes are possible then the guest could accidentally
>    corrupt or delete files outside the shared directory.

Even if escape is possible, its no different than a malicious user
application running. Given sandbox=none can be used in case of
unpriviliged mode, that means user app can only affect files owned by
that user.

Given we are not doing chroot()/pivot_root(), key question will be
what additional path we are enabling using which one can escape
out of this shared directory.

Having said that, In case of unpriviliged mode, it feels that chroot()
is more of a nice to have kind of functionality. Even if guest manages
to break out of shared directory, impact is equivalent to user running a
malicious app directly. 

If doing chroot/pivot_root is must, then we need additional capabilities.
And that probably means we need to launch virtiofsd in a user namespace
with required caps. And that will fall back into the territory of
running virtiofsd in a user namespace. That indeed is an important
use case which needs to be solved.

Thanks
Vivek

Re: [PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Stefan Hajnoczi]

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

On Mon, Aug 03, 2020 at 09:57:15AM -0400, Vivek Goyal wrote:
> On Mon, Aug 03, 2020 at 10:54:59AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jul 30, 2020 at 03:47:35PM -0400, Vivek Goyal wrote:
> > > In sandbox=NONE mode, lo->source points to the directory which is being
> > > exported. We have not done any chroot()/pivot_root(). So open lo->source.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 76ef891105..a6fa816b6c 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
> > >      int fd, res;
> > >      struct stat stat;
> > >  
> > > -    fd = open("/", O_PATH);
> > > +    if (lo->sandbox == SANDBOX_NONE)
> > > +        fd = open(lo->source, O_PATH);
> > > +    else
> > > +        fd = open("/", O_PATH);
> > 
> > Up until now virtiofsd has been able to assume that path traversal has
> > the shared directory as "/".
> > 
> > Now this is no longer true and it is necessary to audit all syscalls
> > that take path arguments. They must ensure that:
> > 1. Path components are safe (no ".." or "/" allowed)
> > 2. Symlinks are not followed.
> 
> This code does not change the path client is passing in and we are
> already doing the checks on passed in paths/names. So existing checks
> should work even for this case, isn't it?
> 
> lo_lookup() {
>     if (strchr(name, '/')) {
>         fuse_reply_err(req, EINVAL);
>         return;
>     }
> }
> 
> lo_do_lookup() {
>     /* Do not allow escaping root directory */
>     if (dir == &lo->root && strcmp(name, "..") == 0) {
>         name = ".";
>     }
> }

Yes, hopefully paths are already checked and syscalls do not follow
symlinks. However, we wouldn't have noticed if either of those were
missing since the pivot_root(2) ensured that path traversal stays within
the shared directory.

Now that a layer of protection has been removed it's necessary to check
again whether everything is really alright.

> 
> > 
> > Did you audit all syscalls made by passthrough_ll.c?
> > 
> > virtiofsd still needs to restrict the client to the shared directory for
> > two reasons:
> > 1. The guest may not be trusted. An unprivileged sandbox=none mount can
> >    be used with a malicious guest.
> > 2. If accidental escapes are possible then the guest could accidentally
> >    corrupt or delete files outside the shared directory.
> 
> Even if escape is possible, its no different than a malicious user
> application running. Given sandbox=none can be used in case of
> unpriviliged mode, that means user app can only affect files owned by
> that user.

Users may run untrusted guests. Those guests should not gain access to
the user's files outside the shared directory.

> If doing chroot/pivot_root is must, then we need additional capabilities.

I think it's not a must, but just an extra layer of security. What I
don't know 100% is whether virtiofsd accidentally relies on that extra
layer of security today since it never ran without it :).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH v2 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Stefan Hajnoczi]

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

On Mon, Aug 03, 2020 at 09:57:15AM -0400, Vivek Goyal wrote:
> On Mon, Aug 03, 2020 at 10:54:59AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jul 30, 2020 at 03:47:35PM -0400, Vivek Goyal wrote:
> > > In sandbox=NONE mode, lo->source points to the directory which is being
> > > exported. We have not done any chroot()/pivot_root(). So open lo->source.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 76ef891105..a6fa816b6c 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
> > >      int fd, res;
> > >      struct stat stat;
> > >  
> > > -    fd = open("/", O_PATH);
> > > +    if (lo->sandbox == SANDBOX_NONE)
> > > +        fd = open(lo->source, O_PATH);
> > > +    else
> > > +        fd = open("/", O_PATH);
> > 
> > Up until now virtiofsd has been able to assume that path traversal has
> > the shared directory as "/".
> > 
> > Now this is no longer true and it is necessary to audit all syscalls
> > that take path arguments. They must ensure that:
> > 1. Path components are safe (no ".." or "/" allowed)
> > 2. Symlinks are not followed.
> 
> This code does not change the path client is passing in and we are
> already doing the checks on passed in paths/names. So existing checks
> should work even for this case, isn't it?
> 
> lo_lookup() {
>     if (strchr(name, '/')) {
>         fuse_reply_err(req, EINVAL);
>         return;
>     }
> }
> 
> lo_do_lookup() {
>     /* Do not allow escaping root directory */
>     if (dir == &lo->root && strcmp(name, "..") == 0) {
>         name = ".";
>     }
> }

Yes, hopefully paths are already checked and syscalls do not follow
symlinks. However, we wouldn't have noticed if either of those were
missing since the pivot_root(2) ensured that path traversal stays within
the shared directory.

Now that a layer of protection has been removed it's necessary to check
again whether everything is really alright.

> 
> > 
> > Did you audit all syscalls made by passthrough_ll.c?
> > 
> > virtiofsd still needs to restrict the client to the shared directory for
> > two reasons:
> > 1. The guest may not be trusted. An unprivileged sandbox=none mount can
> >    be used with a malicious guest.
> > 2. If accidental escapes are possible then the guest could accidentally
> >    corrupt or delete files outside the shared directory.
> 
> Even if escape is possible, its no different than a malicious user
> application running. Given sandbox=none can be used in case of
> unpriviliged mode, that means user app can only affect files owned by
> that user.

Users may run untrusted guests. Those guests should not gain access to
the user's files outside the shared directory.

> If doing chroot/pivot_root is must, then we need additional capabilities.

I think it's not a must, but just an extra layer of security. What I
don't know 100% is whether virtiofsd accidentally relies on that extra
layer of security today since it never ran without it :).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 5/5] virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

[Vivek Goyal]

setup_capabilites() tries to give some of the required capabilities
to act as a full fledged file server in priviliged mode. In unpriviliged
mode we can't get those capabilities and setup_capabilities() will fail.

So don't setup capabilities when sandbox=NONE.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index a6fa816b6c..1a0b24cbf2 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3030,7 +3030,8 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
     }
 
     setup_seccomp(enable_syslog);
-    setup_capabilities(g_strdup(lo->modcaps));
+    if (lo->sandbox != SANDBOX_NONE)
+       setup_capabilities(g_strdup(lo->modcaps));
 }
 
 /* Set the maximum number of open file descriptors */
-- 
2.25.4

[Virtio-fs] [PATCH v2 5/5] virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

[Vivek Goyal]

setup_capabilites() tries to give some of the required capabilities
to act as a full fledged file server in priviliged mode. In unpriviliged
mode we can't get those capabilities and setup_capabilities() will fail.

So don't setup capabilities when sandbox=NONE.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index a6fa816b6c..1a0b24cbf2 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3030,7 +3030,8 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
     }
 
     setup_seccomp(enable_syslog);
-    setup_capabilities(g_strdup(lo->modcaps));
+    if (lo->sandbox != SANDBOX_NONE)
+       setup_capabilities(g_strdup(lo->modcaps));
 }
 
 /* Set the maximum number of open file descriptors */
-- 
2.25.4

Re: [PATCH v2 5/5] virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> setup_capabilites() tries to give some of the required capabilities
> to act as a full fledged file server in priviliged mode. In unpriviliged
> mode we can't get those capabilities and setup_capabilities() will fail.
> 
> So don't setup capabilities when sandbox=NONE.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index a6fa816b6c..1a0b24cbf2 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3030,7 +3030,8 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
>      }
>  
>      setup_seccomp(enable_syslog);
> -    setup_capabilities(g_strdup(lo->modcaps));
> +    if (lo->sandbox != SANDBOX_NONE)
> +       setup_capabilities(g_strdup(lo->modcaps));
>  }

I'd rather keep capabilities and sandboxing separate.
Since I already added modcaps=  how about just letting that
take a varient as  '-o modcaps=keep'

Dave

>  /* Set the maximum number of open file descriptors */
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v2 5/5] virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> setup_capabilites() tries to give some of the required capabilities
> to act as a full fledged file server in priviliged mode. In unpriviliged
> mode we can't get those capabilities and setup_capabilities() will fail.
> 
> So don't setup capabilities when sandbox=NONE.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index a6fa816b6c..1a0b24cbf2 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3030,7 +3030,8 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
>      }
>  
>      setup_seccomp(enable_syslog);
> -    setup_capabilities(g_strdup(lo->modcaps));
> +    if (lo->sandbox != SANDBOX_NONE)
> +       setup_capabilities(g_strdup(lo->modcaps));
>  }

I'd rather keep capabilities and sandboxing separate.
Since I already added modcaps=  how about just letting that
take a varient as  '-o modcaps=keep'

Dave

>  /* Set the maximum number of open file descriptors */
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 0/5] virtiofsd: Add a unprivileged passthrough mode

[Stefan Hajnoczi]

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

On Thu, Jul 30, 2020 at 03:47:31PM -0400, Vivek Goyal wrote:
> Hi,
> 
> This is V2 of patches. Only change since last version is handling of
> lock/pid file creation as per the comments from Daniel Berrange.
> 
> I can't think of any more changes needed. As a unpriviliged user
> inside VM I can do simple operations like create/remove/read/write
> files.
> 
> For more testing, I probably need a testsuite which runs as unpriviliged
> user. pjdfstests needs to run as root and this does not work in this
> setup as creation of files as root fails. (On host, daemon tries to
> switch to root uid and that fails).
> 
> So as of now, I think these are the minimum changes needed to support
> unprivileged passthrough mode of virtiofsd.

Ideas for testing user file I/O:

  $ git clone https://git.qemu.org/git/qemu.git
  $ find qemu -name \*.c
  $ grep -r vhost_user_fs qemu
  $ rm -rf qemu

  $ pip install --user Django
  $ .local/bin/django-admin

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH v2 0/5] virtiofsd: Add a unprivileged passthrough mode

[Stefan Hajnoczi]

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

On Thu, Jul 30, 2020 at 03:47:31PM -0400, Vivek Goyal wrote:
> Hi,
> 
> This is V2 of patches. Only change since last version is handling of
> lock/pid file creation as per the comments from Daniel Berrange.
> 
> I can't think of any more changes needed. As a unpriviliged user
> inside VM I can do simple operations like create/remove/read/write
> files.
> 
> For more testing, I probably need a testsuite which runs as unpriviliged
> user. pjdfstests needs to run as root and this does not work in this
> setup as creation of files as root fails. (On host, daemon tries to
> switch to root uid and that fails).
> 
> So as of now, I think these are the minimum changes needed to support
> unprivileged passthrough mode of virtiofsd.

Ideas for testing user file I/O:

  $ git clone https://git.qemu.org/git/qemu.git
  $ find qemu -name \*.c
  $ grep -r vhost_user_fs qemu
  $ rm -rf qemu

  $ pip install --user Django
  $ .local/bin/django-admin

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]