[libvirt] [PATCH 7/7] Set a security context on /dev and /dev/pts mounts

[Eric Blake]

On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> To allow the container to access /dev and /dev/pts when under
> sVirt, set an explicit mount option. Also set a max size on
> the /dev mount to prevent DOS on memory usage
> 
> * src/lxc/lxc_container.c: Set /dev mount context
> * src/lxc/lxc_controller.c: Set /dev/pts mount context
> ---
>  src/lxc/lxc_container.c  |   75 +++++++++++++++++++++++++++++++++++----------
>  src/lxc/lxc_controller.c |   43 +++++++++++++++++++++++---
>  2 files changed, 96 insertions(+), 22 deletions(-)
> 

> +        } else {
> +#endif
> +            /*
> +             * tmpfs is limited to 64kb, since we only have device nodes in there
> +             * and don't want to DOS the entire OS RAM usage
> +             */
> +            if (virAsprintf(&opts, "mode=755,size=65536%%%s%s%s",

Ouch.  size=65536% is _not_ what you want; you either want size=65536 or
something like size=10%.

> +                            con ? ",context=\"" : "",
> +                            con ? (const char *)con : "",
> +                            con ? "\"" : "") < 0) {

I would have split this:

if (virAsprintf(&opts, "mode=755,size=65536") < 0 ||
    (con && virAsprintf(&opts, ",context=\"%s\"",
                        (const char *)con) < 0)) {

> +                virReportOOMError();
> +                goto cleanup;
> +            }
> +#if HAVE_SELINUX
> +        }
> +#endif

You don't need this second #if.  That is, instead of writing:

#if HAVE_SELINUX
    if (condition) {
        goto cleanup;
    } else {
#endif
        stuff;
#if HAVE_SELINUX
    }
#endif

I would have done:

#if HAVE_SELINUX
    if (condition) {
        goto cleanup;
    }
#endif
    stuff;

> @@ -1373,16 +1380,42 @@ lxcControllerRun(virDomainDefPtr def,
>              goto cleanup;
>          }
>  
> -        /* XXX should we support gid=X for X!=5 for distros which use
> -         * a different gid for tty?  */
> -        VIR_DEBUG("Mounting 'devpts' on %s", devpts);
> -        if (mount("devpts", devpts, "devpts", 0,
> -                  "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) {
> +#if HAVE_SELINUX
> +        if (getfilecon(root->src, &con) < 0 &&
> +            errno != ENOTSUP) {
> +            virReportSystemError(errno,
> +                                 _("Failed to query file context on %s"),
> +                                 root->src);
> +            goto cleanup;
> +        } else {
> +#endif
> +            /*
> +             * tmpfs is limited to 64kb, since we only have device nodes in there
> +             * and don't want to DOS the entire OS RAM usage
> +             */

Is this comment really relative to the devpts mount point?

> +            /* XXX should we support gid=X for X!=5 for distros which use
> +             * a different gid for tty?  */
> +            if (virAsprintf(&opts, "newinstance,ptmxmode=0666,mode=0620,gid=5%s%s%s",
> +                            con ? ",context=\"" : "",
> +                            con ? (const char *)con : "",
> +                            con ? "\"" : "") < 0) {
> +                virReportOOMError();
> +                goto cleanup;
> +            }
> +#if HAVE_SELINUX
> +        }
> +#endif

Same formatting nit about not needing a second #if.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120124/8a37ca34/attachment-0001.sig>