mbox series

[RFC,0/1] tools/virtiofsd: don't create temporary directory in /

Message ID 20201001061519.636959-1-jfreimann@redhat.com
Headers show
Series tools/virtiofsd: don't create temporary directory in / | expand

Message

Jens Freimann Oct. 1, 2020, 6:15 a.m. UTC 
When running a Kata container with virtiofs in OpenShift/k8s I get a
"Operation not permitted" error from a mkdtemp() call in virtiofsd
because it is trying to create a directory like /virtiofsd.11RAND

To avoid this change in virtiofsd, I've tried to set the TMPDIR
environment variable for the virtiofsd process, hoping that mkdtemp()
would use it, but it does not. Looking at glibc code it seems to be used
by tmpfile() etc. only. 

I'm sending this as an RFC because:
Maybe just prepending "/tmp" is not generic enough and we should make it
somehow configurable or use $TMPDIR. Also there might be security
implications I'm not aware of.
The process is running with container_kvm_t context which also needs
a change to be allowed to create files in tmpfs to make it work.



Jens Freimann (1):
  tools/virtiofsd: create tmpdir in /tmp

 tools/virtiofsd/passthrough_ll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jens Freimann Oct. 5, 2020, 1:58 p.m. UTC | #1 
On Thu, Oct 01, 2020 at 08:15:18AM +0200, Jens Freimann wrote:
>I'm sending this as an RFC because:

>Maybe just prepending "/tmp" is not generic enough and we should make it

>somehow configurable or use $TMPDIR. Also there might be security

>implications I'm not aware of.

>The process is running with container_kvm_t context which also needs

>a change to be allowed to create files in tmpfs to make it work.


Fabiano had the idea to use a glib function to create the
temporary directory. It would be good because it uses the $TMPDIR env
variable. 

But before we decide about glib or not: the change is in the call
chain of setup_sandbox() and there was a question what other implications
that has. What do you think?

regards,
Jens
Stefan Hajnoczi Oct. 6, 2020, 10 a.m. UTC | #2 
On Thu, Oct 01, 2020 at 08:15:19AM +0200, Jens Freimann wrote:
> mkdtemp() will try to create a current directory in the working
> directory of the process. In this case it's trying to create it in /.
> This is a problem when the process doesn't have write access there.
> 
> This patch changes the template string and prepends "/tmp" which is
> typically writable.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 0b229ebd57..f79bcce0d7 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2393,7 +2393,7 @@ static void setup_wait_parent_capabilities(void)
>  static void setup_namespaces(struct lo_data *lo, struct fuse_session *se)
>  {
>      pid_t child;
> -    char template[] = "virtiofsd-XXXXXX";
> +    char template[] = "/tmp/virtiofsd-XXXXXX";

Hi Jens,
Let's get rid of the temporary directory completely. I have sent a patch
that bind-mounts /proc/self/fd on top of /proc (which we don't need
anymore).

Stefan