[lxc-devel] [PATCH v2] safe_mount: Handle mounting proc and refactor

[Serge Hallyn]

Quoting Bogdan Purcareata (bogdan.purcareata at nxp.com):
> On 11.01.2016 20:59, Serge Hallyn wrote:
> > Quoting Bogdan Purcareata (bogdan.purcareata at nxp.com):
> >> The safe_mount primitive will mount the fs in the new container
> >> environment by using file descriptors referred in /proc/self/fd.
> >> However, when the mounted filesystem is proc itself, it will have
> >> been previously unmounted, therefore resulting in an error when
> >> searching for these file descriptors. This only happens when there's
> >> no container rootfs prefix (commonly with lxc-execute).
> >>
> >> Implement the support for this use case as well, by doing the mount
> >> based on the full path.
> >>
> >> Refactor the whole function in order to remove duplicated code checks
> >> and improve readability.
> >>
> >> Changes since v1:
> >> - In order to address CVE-2015-1335, still check if the destination is
> >> not a symlink. Do the mount only if the destination file descriptor
> >> exists.
> >>
> >> Signed-off-by: Bogdan Purcareata <bogdan.purcareata at nxp.com>
> >> ---
> >>   src/lxc/utils.c | 49 ++++++++++++++++++++++++++++---------------------
> >>   1 file changed, 28 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> >> index d9e769d..c53711a 100644
> >> --- a/src/lxc/utils.c
> >> +++ b/src/lxc/utils.c
> >> @@ -1644,9 +1644,9 @@ out:
> >>   int safe_mount(const char *src, const char *dest, const char *fstype,
> >>   		unsigned long flags, const void *data, const char *rootfs)
> >>   {
> >> -	int srcfd = -1, destfd, ret, saved_errno;
> >> +	int srcfd = -1, destfd = -1, ret = 0;
> >>   	char srcbuf[50], destbuf[50]; // only needs enough for /proc/self/fd/<fd>
> >> -	const char *mntsrc = src;
> >> +	const char *mntsrc = src, *mntdest = dest;
> >>   
> >>   	if (!rootfs)
> >>   		rootfs = "";
> >> @@ -1655,45 +1655,52 @@ int safe_mount(const char *src, const char *dest, const char *fstype,
> >>   	if (flags & MS_BIND && src && src[0] != '/') {
> >>   		INFO("this is a relative bind mount");
> >>   		srcfd = open_without_symlink(src, NULL);
> >> -		if (srcfd < 0)
> >> -			return srcfd;
> >> +		if (srcfd < 0) {
> >> +			ret = srcfd;
> >> +			goto out;
> >> +		}
> >>   		ret = snprintf(srcbuf, 50, "/proc/self/fd/%d", srcfd);
> >>   		if (ret < 0 || ret > 50) {
> >> -			close(srcfd);
> >>   			ERROR("Out of memory");
> >> -			return -EINVAL;
> >> +			ret = -EINVAL;
> >> +			goto out_src;
> >>   		}
> >>   		mntsrc = srcbuf;
> >>   	}
> >>   
> >>   	destfd = open_without_symlink(dest, rootfs);
> >>   	if (destfd < 0) {
> >> -		if (srcfd != -1)
> >> -			close(srcfd);
> >> -		return destfd;
> >> +		ret = destfd;
> >> +		goto out_src;
> >>   	}
> >>   
> >>   	ret = snprintf(destbuf, 50, "/proc/self/fd/%d", destfd);
> >>   	if (ret < 0 || ret > 50) {
> >> -		if (srcfd != -1)
> >> -			close(srcfd);
> >> -		close(destfd);
> >>   		ERROR("Out of memory");
> >> -		return -EINVAL;
> >> +		ret = -EINVAL;
> >> +		goto out_dest;
> >>   	}
> >>   
> >> -	ret = mount(mntsrc, destbuf, fstype, flags, data);
> >> -	saved_errno = errno;
> >> -	if (srcfd != -1)
> >> -		close(srcfd);
> >> -	close(destfd);
> >> +	/* make sure the destination descriptor exists */
> >> +	if (access(destbuf, F_OK) == 0)
> >> +		mntdest = destbuf;
> > First, if we're going to shortcut I'd prefer to say "if /proc/self
> > does not exist then skip this check" fo rnow.
> > But can we think of any way to still do this check?
> >
> > What exactly are the cases?
> >
> > 1. lxc-execute, lxc-init tries to mount /proc.  We should be able
> > to simply have lxc always mount /proc before the pivot_root, so
> > we can properly do this check.
> >
> > what use-cases will break if we demand /proc to exist in the
> > container?  (We can add an option to umount /proc in lxc-init,
> > but the directory would have to exist)
> That's my use case - the failing function is tmp_proc_mount, thus happening 

Wait, is your rootfs NULL or not?

We can handle that case specially.

mount_proc_if_needed() is only called from tmp_proc_mount(), which is only
called from lxc_setup().  If rootfs is NULL, then all bets are off anyway
and we can just call mount().  If rootfs is not NULL, then we're mounting
in a separate container rootfs and we should use safe_mount(), but we
expect the real /proc to then exist.