[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] lxc: make the pivot_root more robust.



On Mon, Apr 06, 2009 at 02:13:04PM -0500, Serge E. Hallyn wrote:
> libvirt/lxc is broken on F11.  The pivot_root() call returns
> -EINVAL.  The below is one way we can fix it.  I'm also sending
> another patch which takes the simpler approach of using chroot.
> However, chroot is trivially escapable (see for instance
> http://www.linuxsecurity.com/content/view/117632/49/).  I don't
> know whether the typical libvirt user would care.  If so, then
> the below patch is probably the way to go.

We intended to cover 2 use cases for LXC in libvirt

 - Resource control / isolation 
 - Secure separation of applications

For the second case, we really need to use pivot_root(), though
it in itself is not sufficient for security of course. We also
have a TODO item to make use of private devpts mounts in this
bit of code now that's in 2.6.29 kernels.

> 
> >From 26cac415771a2d9712af0e1ce60a0bcb41b44665 Mon Sep 17 00:00:00 2001
> From: root <root localhost localdomain>
> Date: Sat, 4 Apr 2009 22:49:20 -0400
> Subject: [PATCH 1/1] lxc: make the pivot_root more robust.
> 
> The libvirt lxc driver uses pivot_root instead of chroot, because
> the latter is trivially escapable.  However, the pivot_root(2)
> system call can fail for several subtle reasons.  Depending upon
> your distro init sequence you may get lucky and have the old
> recipe work, but on a Fedora 11 standard install, for instance,
> it will fail.
> 
> Do a few more steps to make pivot_root hopefully always
> succeed.  We mark / as MS_PRIVATE, create an empty tmpfs,
> and bind-mount the container root onto /new in that fs.
> In this way, we ensure two reasons for pivot_root to fail -
> namely old_root->parent being MS_SHARED and old_root and
> new_root being on the same fs - won't happen.
> 
> Signed-off-by: Serge Hallyn <serue us ibm com>
> ---
>  src/lxc_container.c |  108 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 85 insertions(+), 23 deletions(-)
> 
> diff --git a/src/lxc_container.c b/src/lxc_container.c
> index 3f17b8d..d3959f6 100644
> --- a/src/lxc_container.c
> +++ b/src/lxc_container.c
> @@ -264,50 +264,113 @@ static int lxcContainerChildMountSort(const void *a, const void *b)
>    return strcmp(*sb, *sa);
>  }
>  
> +#ifndef MS_REC
> +#define MS_REC          16384
> +#endif
> +
> +#ifndef MNT_DETACH
> +#define MNT_DETACH      0x00000002
> +#endif
> +
> +#ifndef MS_PRIVATE
> +#define MS_PRIVATE              1<<18
> +#endif
> +
>  static int lxcContainerPivotRoot(virDomainFSDefPtr root)
>  {
>      int rc;
> -    char *oldroot;
> +    char *oldroot = NULL, *newroot = NULL;
>  
> -    /* First step is to ensure the new root itself is
> -       a mount point */
> -    if (mount(root->src, root->src, NULL, MS_BIND, NULL) < 0) {
> -        virReportSystemError(NULL, errno,
> -                             _("failed to bind new root %s"),
> -                             root->src);
> -        return -1;
> +    /* root->parent must be private, so make / private. */
> +    if (mount("", "/", NULL, MS_PRIVATE|MS_REC, NULL) < 0) {
> +        virReportSystemError(NULL, errno, "%s",
> +                             _("failed to make root private"));
> +        goto err;
>      }
>  
>      if (virAsprintf(&oldroot, "%s/.oldroot", root->src) < 0) {
>          virReportOOMError(NULL);
> -        return -1;
> +        goto err;
>      }
>  
>      if ((rc = virFileMakePath(oldroot)) < 0) {
>          virReportSystemError(NULL, rc,
>                               _("failed to create %s"),
>                               oldroot);
> -        VIR_FREE(oldroot);
> -        return -1;
> +        goto err;
> +    }
> +
> +    /* Create a tmpfs root since old and new roots must be
> +     * on separate filesystems */
> +    if (mount("", oldroot, "tmpfs", 0, NULL) < 0) {
> +        virReportSystemError(NULL, errno,
> +                             _("failed to mount empty tmpfs at %s"),
> +                             oldroot);
> +        goto err;
> +    }
> +	
> +    /* Create a directory called 'new' in tmpfs */
> +    if (virAsprintf(&newroot, "%s/new", oldroot) < 0) {
> +        virReportOOMError(NULL);
> +        goto err;
> +    }
> +
> +    if ((rc = virFileMakePath(newroot)) < 0) {
> +        virReportSystemError(NULL, rc,
> +                             _("failed to create %s"),
> +                             newroot);
> +        goto err;
> +    }
> +
> +    /* ... and mount our root onto it */
> +    if (mount(root->src, newroot, NULL, MS_BIND|MS_REC, NULL) < 0) {
> +        virReportSystemError(NULL, errno,
> +                             _("failed to bind new root %s into tmpfs"),
> +                             root->src);
> +        goto err;
> +    }
> +
> +    /* Now we chroot into the tmpfs, then pivot into the
> +     * root->src bind-mounted onto '/new' */
> +    if (chroot(oldroot) < 0) {
> +        virReportSystemError(NULL, errno, "%s",
> +                             _("failed to chroot into tmpfs"));
> +        goto err;
> +    }
> +
> +    if (chdir("/new") < 0) {
> +        virReportSystemError(NULL, errno, "%s",
> +                             _("failed to chdir into /new on tmpfs"));
> +        goto err;
>      }
>  
>      /* The old root directory will live at /.oldroot after
>       * this and will soon be unmounted completely */
> -    if (pivot_root(root->src, oldroot) < 0) {
> -        virReportSystemError(NULL, errno,
> -                             _("failed to pivot root %s to %s"),
> -                             oldroot, root->src);
> -        VIR_FREE(oldroot);
> -        return -1;
> +    if (pivot_root(".", ".oldroot") < 0) {
> +        virReportSystemError(NULL, errno, "%s",
> +                             _("failed to pivot root"));
> +        goto err;
>      }
> -    VIR_FREE(oldroot);
>  
>      /* CWD is undefined after pivot_root, so go to / */
> -    if (chdir("/") < 0) {
> -        return -1;
> +    if (chdir("/") < 0)
> +        goto err;
> +
> +    if (umount2(".oldroot", MNT_DETACH) < 0) {
> +        virReportSystemError(NULL, errno, "%s",
> +                             _("failed to lazily unmount old root"));
> +        goto err;
>      }
>  
> +    VIR_FREE(oldroot);
> +    VIR_FREE(newroot);
> +
>      return 0;
> +
> +err:
> +    if (oldroot) VIR_FREE(oldroot);
> +    if (newroot) VIR_FREE(newroot);
> +    return -1;
>  }
>  
>  static int lxcContainerPopulateDevices(void)
> @@ -349,10 +412,9 @@ static int lxcContainerPopulateDevices(void)
>                               _("cannot create /dev/pts"));
>          return -1;
>      }
> -    if (mount("/.oldroot/dev/pts", "/dev/pts", NULL,
> -              MS_MOVE, NULL) < 0) {
> +    if (mount("devpts", "/dev/pts", "devpts", 0, NULL) < 0) {
>          virReportSystemError(NULL, errno, "%s",
> -                             _("failed to move /dev/pts into container"));
> +                             _("failed to mount /dev/pts in container"));
>          return -1;
>      }


ACK

This looks much more robust that my  original code

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]