[PATCH user-cr] add -t option to mount new devpts

[PATCH user-cr] add -t option to mount new devpts

[Serge E. Hallyn]

Trivial patch, and I'm not sure whether we want this or want to
do it this way.  But it saves me having to do it during my restart.sh
wrapper shell-script.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 restart.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/restart.c b/restart.c
index 063e973..03c1850 100644
--- a/restart.c
+++ b/restart.c
@@ -30,6 +30,7 @@
 #include <asm/unistd.h>
 #include <sys/syscall.h>
 #include <sys/prctl.h>
+#include <sys/mount.h>
 
 #include <linux/sched.h>
 #include <linux/checkpoint.h>
@@ -79,6 +80,7 @@ static char usage_str[] =
 "  -l,--logfile=FILE     write error and debug data to FILE (default=none)\n"
 "     --logfile-fd=FD    write error and debug data to file desctiptor FD\n"
 "     --inspect          inspect image on-the-fly for error records\n"
+"  -t,--pty		 start in a new devpts namespace to support ptys\n"
 "  -v,--verbose          verbose output\n"
 "  -d,--debug            debugging output\n"
 "     --warn-COND        warn on condition COND, but proceed anyways\n"
@@ -365,6 +367,7 @@ struct args {
 	long warn;
 	long fail;
 	int keep_lsm;
+	int pty;
 };
 
 #define CKPT_COND_PIDZERO  0x1
@@ -444,9 +447,10 @@ static void parse_args(struct args *args, int argc, char *argv[])
 		{ "debug",	no_argument,		NULL, 'd' },
 		{ "warn-pidzero",	no_argument,	NULL, 9 },
 		{ "fail-pidzero",	no_argument,	NULL, 10 },
+		{ "pty", no_argument,			NULL, 't'},
 		{ NULL,		0,			NULL, 0 }
 	};
-	static char optc[] = "hdvkpPwWF:r:i:l:";
+	static char optc[] = "hdvkpPwWF:r:i:l:t";
 
 	int optind;
 	int sig;
@@ -456,6 +460,7 @@ static void parse_args(struct args *args, int argc, char *argv[])
 	args->wait = 1;
 	args->infd = -1;
 	args->logfd = -1;
+	args->pty = 0;
 
 	while (1) {
 		int c = getopt_long(argc, argv, optc, opts, &optind);
@@ -469,6 +474,9 @@ static void parse_args(struct args *args, int argc, char *argv[])
 		case 'v':
 			global_verbose = 1;
 			break;
+		case 't':
+			args->pty = 1;
+			break;
 		case 5:  /* --inspect */
 			args->inspect = 1;
 			break;
@@ -786,6 +794,31 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
+	/* private devpts namespace? */
+	if (args.pty) {
+		struct stat ptystat;
+		/* make sure /dev/ptmx is a link else we'll just break */
+		ret = lstat("/dev/ptmx", &ptystat);
+		if (ret) {
+			perror("stat /dev/ptmx");
+			exit(1);
+		}
+		if ((ptystat.st_mode & S_IFMT) != S_IFLNK) {
+			printf("Error: /dev/ptmx must be a link to /dev/pts/ptmx\n");
+			exit(1);
+		}
+		ret = unshare(CLONE_NEWNS);
+		if (ret) {
+			perror("unshare mounts ns (for -pty)");
+			exit(1);
+		}
+		ret = mount("pts", "/dev/pts", "devpts", 0, "newinstance");
+		if (ret) {
+			perror("mount -t devpts -o newinstance");
+			exit(1);
+		}
+	}
+
 	/* self-restart ends here: */
 	if (args.self) {
 		restart(getpid(), STDIN_FILENO, RESTART_TASKSELF, args.logfd);
-- 
1.6.1.1

Re: [PATCH user-cr] add -t option to mount new devpts

[Oren Laadan]

Sorry for the late response ...

Serge E. Hallyn wrote:
> Trivial patch, and I'm not sure whether we want this or want to
> do it this way.  But it saves me having to do it during my restart.sh
> wrapper shell-script.

This looks useful.

I wonder if it makes sense to generalize that to allow the user
to request any mount (and multiple mounts), e.g.
	restart --mount="......" --mount="......." ...

With this switch, 'restart' will create a new mntns and do the
mounts in it.

We can then add shortcuts, like --mount-ptys.

However, I'm concerned about the security implications: ideally
'restart' will be setuid executable, so it must be prudent in
accepting such generic requests as 'mount'.

This last argument is also valid if we stay with this patch,
because it is racy (time of check to time of use).

> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  restart.c |   35 ++++++++++++++++++++++++++++++++++-
>  1 files changed, 34 insertions(+), 1 deletions(-)
> 
> diff --git a/restart.c b/restart.c
> index 063e973..03c1850 100644
> --- a/restart.c
> +++ b/restart.c
> @@ -30,6 +30,7 @@
>  #include <asm/unistd.h>
>  #include <sys/syscall.h>
>  #include <sys/prctl.h>
> +#include <sys/mount.h>
>  
>  #include <linux/sched.h>
>  #include <linux/checkpoint.h>
> @@ -79,6 +80,7 @@ static char usage_str[] =
>  "  -l,--logfile=FILE     write error and debug data to FILE (default=none)\n"
>  "     --logfile-fd=FD    write error and debug data to file desctiptor FD\n"
>  "     --inspect          inspect image on-the-fly for error records\n"
> +"  -t,--pty		 start in a new devpts namespace to support ptys\n"
>  "  -v,--verbose          verbose output\n"
>  "  -d,--debug            debugging output\n"
>  "     --warn-COND        warn on condition COND, but proceed anyways\n"
> @@ -365,6 +367,7 @@ struct args {
>  	long warn;
>  	long fail;
>  	int keep_lsm;
> +	int pty;
>  };
>  
>  #define CKPT_COND_PIDZERO  0x1
> @@ -444,9 +447,10 @@ static void parse_args(struct args *args, int argc, char *argv[])
>  		{ "debug",	no_argument,		NULL, 'd' },
>  		{ "warn-pidzero",	no_argument,	NULL, 9 },
>  		{ "fail-pidzero",	no_argument,	NULL, 10 },
> +		{ "pty", no_argument,			NULL, 't'},
>  		{ NULL,		0,			NULL, 0 }
>  	};
> -	static char optc[] = "hdvkpPwWF:r:i:l:";
> +	static char optc[] = "hdvkpPwWF:r:i:l:t";
>  
>  	int optind;
>  	int sig;
> @@ -456,6 +460,7 @@ static void parse_args(struct args *args, int argc, char *argv[])
>  	args->wait = 1;
>  	args->infd = -1;
>  	args->logfd = -1;
> +	args->pty = 0;
>  
>  	while (1) {
>  		int c = getopt_long(argc, argv, optc, opts, &optind);
> @@ -469,6 +474,9 @@ static void parse_args(struct args *args, int argc, char *argv[])
>  		case 'v':
>  			global_verbose = 1;
>  			break;
> +		case 't':
> +			args->pty = 1;
> +			break;
>  		case 5:  /* --inspect */
>  			args->inspect = 1;
>  			break;
> @@ -786,6 +794,31 @@ int main(int argc, char *argv[])
>  		exit(1);
>  	}
>  
> +	/* private devpts namespace? */
> +	if (args.pty) {
> +		struct stat ptystat;
> +		/* make sure /dev/ptmx is a link else we'll just break */
> +		ret = lstat("/dev/ptmx", &ptystat);
> +		if (ret) {
> +			perror("stat /dev/ptmx");
> +			exit(1);
> +		}
> +		if ((ptystat.st_mode & S_IFMT) != S_IFLNK) {
> +			printf("Error: /dev/ptmx must be a link to /dev/pts/ptmx\n");
> +			exit(1);
> +		}

Do we really need these two tests ?  Wouldn't the mount below
fail anyway in these cases ?

> +		ret = unshare(CLONE_NEWNS);
> +		if (ret) {
> +			perror("unshare mounts ns (for -pty)");
> +			exit(1);
> +		}
> +		ret = mount("pts", "/dev/pts", "devpts", 0, "newinstance");
> +		if (ret) {
> +			perror("mount -t devpts -o newinstance");
> +			exit(1);
> +		}
> +	}
> +
>  	/* self-restart ends here: */
>  	if (args.self) {
>  		restart(getpid(), STDIN_FILENO, RESTART_TASKSELF, args.logfd);

Thanks,

Oren.

Re: [PATCH user-cr] add -t option to mount new devpts

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> Sorry for the late response ...
> 
> Serge E. Hallyn wrote:
> >Trivial patch, and I'm not sure whether we want this or want to
> >do it this way.  But it saves me having to do it during my restart.sh
> >wrapper shell-script.
> 
> This looks useful.
> 
> I wonder if it makes sense to generalize that to allow the user
> to request any mount (and multiple mounts), e.g.
> 	restart --mount="......" --mount="......." ...

Or just a --fstab=some_file option?

> With this switch, 'restart' will create a new mntns and do the
> mounts in it.
> 
> We can then add shortcuts, like --mount-ptys.
> 
> However, I'm concerned about the security implications: ideally
> 'restart' will be setuid executable, so it must be prudent in
> accepting such generic requests as 'mount'.

Hmm, we could use our own mount binary, which is willing to
mount things like devpts and proc as root (since that's completely
private) but otherwise runs the mounts as the original user?  Eventually
do that on top of Miklos' unprivileged-mounts patchset, which will allow
bind mounts on top of dirs/files to which the user has write access.  In
the meantime other mounts would only be allowed if the user was
originally root.

So basically our mount binary would be run with euid=0 and
ruid=suid=N where N is the original userid.

> This last argument is also valid if we stay with this patch,
> because it is racy (time of check to time of use).

Yeah I'm not sure we can solve this right now.  We might be best
off saying that only ruid=0 can do the mounts, and mention something
about setuid-root restart.c can trust TPM-signed checkpoint images.

-serge