Re: [Xen-devel] [PATCH v2 4/6] tools/dm_restrict: Unshare mount and IPC namespaces on Linux

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

George Dunlap writes ("[PATCH v2 4/6] tools/dm_restrict: Unshare mount and IPC 
namespaces on Linux"):
> QEMU running under Xen doesn't need mount or IPC functionality.
> Create and enter separate namespaces for each of these before
> executing QEMU, so that in the event that other restrictions fail, the
> process won't be able to even name system mount points or exsting
> non-file-based IPC descriptors to attempt to attack them.

> Unsharing is something a process can only do to itself (it would
> seem); so add an os-specific "dm_preexec_restrict()" hook just before
> we exec() the device model.
...
> +
> +void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
> +{
> +    return;
> +}

This patch is the point of divergence between Linux dom0's and FreeBSD
dom0's.

IMO it's not good enough to simply talk about some restrictions as
being Linux-only.  Rather, the restrictions are supposed to work
together as a whole, and the whole syscall API available to qemu needs
to be considered.  That would need to be done separately for FreeBSD.

I think that this means we should explicitly write down that the qemu
depriv implementation is incomplete on FreeBSD.

> +/* 
> + * Called after forking but before executing the local devicemodel.
> + * Must call _exit(-1) on failure, printing an error message to
> + * stderrfd if available.

IMO it would be better if the single call site handled the errors.  I
think that can be done in a standard way.  So this function should
probably be expected to call a LOG macro and then return ERROR_FAIL,
on failure.

> +void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
> +{
> +    int rc;
> +
> +    /* Unshare mount and IPC namespaces.  These are unused by QEMU. */
> +    rc = unshare(CLONE_NEWNS | CLONE_NEWIPC);

Variable name should be r.  rc is used for libxl error codes.
See tools/libxl/CODING_STYLE.

> +    if (rc < 0) {
> +        char *msg = GCSPRINTF("libxl: Mount and IPC namespace unfailed with 
> error %d\n",
> +                              errno);

Please do not print numeric errno values.  I think you can use LOG:

 |       .... The child may not
 |  * make any further calls to libxl infrastructure, except for memory
 |  * allocation and logging.    ...

(from libxl_internal.h's discussion of libxl__ev_child_fork)

> +# TEST: Namespace unsharing
> +#
> +# Read /proc/<dmpid>/ns/<namespace> and make sure it's not equal to
> +# the current processes' value
> +for nsname in ipc mnt; do
> +    echo -n "Unshare namespace $nsname: "
> +    dmns=$(readlink /proc/$dmpid/ns/$nsname)
> +    myns=$(readlink /proc/self/ns/$nsname)
> +
> +    if [[ "$dmns" == "$myns" ]] ; then
> +     echo "FAILED"
> +     failed="true"
> +    else
> +     echo "PASSED"
> +    fi
> +done

I wonder if maybe this output should be in subunit v1 format.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel