On Sun, Oct 22, 2017 at 08:40:36PM +0200, Christian Brauner wrote:
> +/**
> + * insert_extent - Safely insert a new idmap extent into struct uid_gid_map.
> + * Takes care to allocate a 4K block of memory if the number of mappings
> exceeds
> + * UID_GID_MAP_MAX_BASE_EXTENTS.
> + */
> +static int insert_extent(struct uid_gid_map *map, struct uid_gid_extent
> *extent)
> +{
> + if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS) {
> + map->extent[map->nr_extents].first = extent->first;
> + map->extent[map->nr_extents].lower_first = extent->lower_first;
> + map->extent[map->nr_extents].count = extent->count;
> + return 0;
> + }
> +
> + if (map->nr_extents == UID_GID_MAP_MAX_BASE_EXTENTS) {
> + struct uid_gid_extent *forward;
> +
> + /* Allocate memory for 340 mappings. */
> + forward = kmalloc(sizeof(struct uid_gid_extent) *
> + UID_GID_MAP_MAX_EXTENTS, GFP_KERNEL);
> + if (IS_ERR(forward))
I think you have the same issue here as in v4, kmalloc doesn't return
-ENOMEM, it returns NULL on failure.
> static ssize_t map_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos,
> int cap_setid,
> @@ -648,7 +911,7 @@ static ssize_t map_write(struct file *file, const char
> __user *buf,
> struct user_namespace *ns = seq->private;
> struct uid_gid_map new_map;
> unsigned idx;
> - struct uid_gid_extent *extent = NULL;
> + struct uid_gid_extent extent;
struct uid_gid_extent = {}; maybe? Then you can drop the memset().
> char *kbuf = NULL, *pos, *next_line;
> ssize_t ret = -EINVAL;
>
> @@ -673,6 +936,8 @@ static ssize_t map_write(struct file *file, const char
> __user *buf,
> */
> mutex_lock(&userns_state_mutex);
>
> + memset(&new_map, 0, sizeof(struct uid_gid_map));
> +
> ret = -EPERM;
> /* Only allow one successful write to the map */
> if (map->nr_extents != 0)
> @@ -700,9 +965,7 @@ static ssize_t map_write(struct file *file, const char
> __user *buf,
> /* Parse the user data */
> ret = -EINVAL;
> pos = kbuf;
> - new_map.nr_extents = 0;
> for (; pos; pos = next_line) {
> - extent = &new_map.extent[new_map.nr_extents];
>
> /* Find the end of line and ensure I don't look past it */
> next_line = strchr(pos, '\n');
> @@ -714,17 +977,17 @@ static ssize_t map_write(struct file *file, const char
> __user *buf,
> }
>
> pos = skip_spaces(pos);
> - extent->first = simple_strtoul(pos, &pos, 10);
> + extent.first = simple_strtoul(pos, &pos, 10);
> if (!isspace(*pos))
> goto out;
>
> pos = skip_spaces(pos);
> - extent->lower_first = simple_strtoul(pos, &pos, 10);
> + extent.lower_first = simple_strtoul(pos, &pos, 10);
> if (!isspace(*pos))
> goto out;
>
> pos = skip_spaces(pos);
> - extent->count = simple_strtoul(pos, &pos, 10);
> + extent.count = simple_strtoul(pos, &pos, 10);
> if (*pos && !isspace(*pos))
> goto out;
>
> @@ -734,29 +997,33 @@ static ssize_t map_write(struct file *file, const char
> __user *buf,
> goto out;
>
> /* Verify we have been given valid starting values */
> - if ((extent->first == (u32) -1) ||
> - (extent->lower_first == (u32) -1))
> + if ((extent.first == (u32) -1) ||
> + (extent.lower_first == (u32) -1))
> goto out;
>
> /* Verify count is not zero and does not cause the
> * extent to wrap
> */
> - if ((extent->first + extent->count) <= extent->first)
> + if ((extent.first + extent.count) <= extent.first)
> goto out;
> - if ((extent->lower_first + extent->count) <=
> - extent->lower_first)
> + if ((extent.lower_first + extent.count) <=
> + extent.lower_first)
> goto out;
>
> /* Do the ranges in extent overlap any previous extents? */
> - if (mappings_overlap(&new_map, extent))
> + if (mappings_overlap(&new_map, &extent))
> goto out;
>
> - new_map.nr_extents++;
> -
> - /* Fail if the file contains too many extents */
> - if ((new_map.nr_extents == UID_GID_MAP_MAX_EXTENTS) &&
> + if ((new_map.nr_extents + 1) == UID_GID_MAP_MAX_EXTENTS &&
> (next_line != NULL))
> goto out;
> +
> + ret = insert_extent(&new_map, &extent);
> + if (ret < 0)
> + goto out;
> + ret = -EINVAL;
> +
> + new_map.nr_extents++;
> }
> /* Be very certaint the new map actually exists */
> if (new_map.nr_extents == 0)
> @@ -767,16 +1034,26 @@ static ssize_t map_write(struct file *file, const char
> __user *buf,
> if (!new_idmap_permitted(file, ns, cap_setid, &new_map))
> goto out;
>
> + ret = sort_idmaps(&new_map);
> + if (ret < 0)
> + goto out;
> +
> + ret = -EPERM;
> /* Map the lower ids from the parent user namespace to the
> * kernel global id space.
> */
> for (idx = 0; idx < new_map.nr_extents; idx++) {
> + struct uid_gid_extent *e;
> u32 lower_first;
> - extent = &new_map.extent[idx];
> +
> + if (new_map.nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> + e = &new_map.extent[idx];
> + else
> + e = &new_map.forward[idx];
>
> lower_first = map_id_range_down(parent_map,
> - extent->lower_first,
> - extent->count);
> + e->lower_first,
> + e->count);
>
> /* Fail if we can not map the specified extent to
> * the kernel global id space.
> @@ -784,18 +1061,34 @@ static ssize_t map_write(struct file *file, const char
> __user *buf,
> if (lower_first == (u32) -1)
> goto out;
>
> - extent->lower_first = lower_first;
> + e->lower_first = lower_first;
> }
>
> /* Install the map */
> - memcpy(map->extent, new_map.extent,
> - new_map.nr_extents*sizeof(new_map.extent[0]));
> + if (new_map.nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS) {
> + memcpy(map->extent, new_map.extent,
> + new_map.nr_extents * sizeof(new_map.extent[0]));
> + } else {
> + map->forward = new_map.forward;
> + map->reverse = new_map.reverse;
> + }
> smp_wmb();
> map->nr_extents = new_map.nr_extents;
>
> *ppos = count;
> ret = count;
> out:
> + if (ret < 0 && new_map.nr_extents > UID_GID_MAP_MAX_BASE_EXTENTS) {
> + if (new_map.forward)
> + kfree(new_map.forward);
It's safe to pass null to kfree(), so I think you can just do,
kfree(new_map.forward);
and in the other places in this function.
Tycho
