On Mon, Oct 23, 2017 at 10:02:23AM +0200, Tycho Andersen wrote:
> 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.
Ah, I somehow was convinced that only kmemdup() simply returned NULL. Thanks,
Tycho.
>
> > 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().
I don't think this is right. First, I don't care about struct uid_gid_extent to
be zeroed. What I care about is struct uid_gid_map new_map to be zeroed. This
needs to be done otherwise when the condition if (map->nr_extents != 0) is true
further down below you'd jump to unitialized memory in the cleanup code below.
Also, any modification of struct uid_gid_map_new new_map needs to be done while
holding the mutex lock. So I think keeping the memset() here is ok.
>
> > 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.
Yeah, that sounds right. Thanks!
>
> Tycho
