From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751399AbdJWIC2 (ORCPT ); Mon, 23 Oct 2017 04:02:28 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:55972 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750802AbdJWIC1 (ORCPT ); Mon, 23 Oct 2017 04:02:27 -0400 X-Google-Smtp-Source: ABhQp+TkiqzsEKFZZd7cFWHTgoRc56k3fZDFhvcXzd+12zCFVvNegUj64UJ+5ucRMqyrIK26NhHz9Q== Date: Mon, 23 Oct 2017 10:02:23 +0200 From: Tycho Andersen To: Christian Brauner Cc: linux-kernel@vger.kernel.org, serge@hallyn.com, stgraber@ubuntu.com, Eric Biederman Subject: Re: [PATCH 2/2 v5] user namespaces: bump idmap limits to 340 Message-ID: <20171023080223.d4hmqq6tbi43wfoj@docker> References: <20171022184036.29403-1-christian.brauner@ubuntu.com> <20171022184036.29403-2-christian.brauner@ubuntu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171022184036.29403-2-christian.brauner@ubuntu.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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