[systemd-devel] [PATCH 1/2] Add detect_userns to detect uid/gid shifts

[Lennart Poettering]

On Thu, 08.01.15 14:27, Stéphane Graber (stgraber at ubuntu.com) wrote:

> This adds a new detect_userns function in virt.c which will check
> whether systemd is running in the host user namespace (single map of all
> available uids and gids) or is using a uid/gid map.
> 
> The check makes sure that uid_map and gid_map are both exactly equal to
> the default host map (assuming 32bit uid_t) for a process running in the
> host namespace.

Hmm, so I think detecting userns is useful, and we probably should add
that as one of the checks to detect_continer(), as a baseline
container implementation check or so.

However, for the OOM adjust setting I'd really keep things simple, and
just gracefully skip over things on any kind of EPERM, not just those
related to userns. I mean, one day selinux or appormor or some other
MAC might want to prohibit access to that too, and we shouldn't have
to whitelist every single one of them, given how minor and relatively
irrelevant the oom adjustment stuff is.

Since the fix for that is trivial I now made the check for it, see git
d5243d628624038567c576e9b69c1d775eb05a05, please test.

I'd still be interested in getting the detect_userns() stuff into
detect_container() as a fallback in some form, hence some comments on
that:

> ---
>  src/shared/virt.c | 22 ++++++++++++++++++++++
>  src/shared/virt.h |  1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/src/shared/virt.c b/src/shared/virt.c
> index f10baab..3d94e1f 100644
> --- a/src/shared/virt.c
> +++ b/src/shared/virt.c
> @@ -363,3 +363,25 @@ int detect_virtualization(const char **id) {
>  
>          return VIRTUALIZATION_NONE;
>  }
> +
> +/* Detect whether we run in a uid/gid shifted namespace */
> +int detect_userns(void) {
> +        int r;
> +        static const char host_id_map[] = "         0          0
>  4294967295";

Not a fan of "static const char[]" where a #define would do too. Also,
would prefer parsing the values with sscanf() into numbers rather than
checking the byte-by-byte string...

> +        char *uid_map = NULL;
> +        char *gid_map = NULL;

Something needs to free these, consider using "_cleanup_free_" for this.

> +
> +        /* Check if we are uid-shifted */
> +        r = read_one_line_file("/proc/self/uid_map", &uid_map);
> +        if (r == 0 && !streq(uid_map, host_id_map))
> +                return 1;
> +
> +        /* Check if we are gid-shifted */
> +        r = read_one_line_file("/proc/self/gid_map", &gid_map);
> +        if (r == 0 && !streq(gid_map, host_id_map))
> +                return 1;
> +
> +        /* If both uid_map and gid_map don't exist or if they both match
> +         * the full uid/gid range, then we're not inside a user namespace */
> +        return 0;
> +}

Also, I think the code should return errors when read_one_line_file()
fails, except when it gets ENOENT, since that is an indication that
userns is simply not enabled in the kernel.

Lennart

-- 
Lennart Poettering, Red Hat