Re: [PATCH] kernel: automatically split user namespace extent

[Serge E. Hallyn]

On Wed, Dec 02, 2020 at 05:12:27PM +0100, Giuseppe Scrivano wrote:
> Hi Eric,
> 
> ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes:
> 
> > Nit: The tag should have been "userns:" rather than kernel.
> >
> > Giuseppe Scrivano <gscrivan@xxxxxxxxxx> writes:
> >
> >> writing to the id map fails when an extent overlaps multiple mappings
> >> in the parent user namespace, e.g.:
> >>
> >> $ cat /proc/self/uid_map
> >>          0       1000          1
> >>          1     100000      65536
> >> $ unshare -U sleep 100 &
> >> [1] 1029703
> >> $ printf "0 0 100\n" | tee /proc/$!/uid_map
> >> 0 0 100
> >> tee: /proc/1029703/uid_map: Operation not permitted
> >>
> >> To prevent it from happening, automatically split an extent so that
> >> each portion fits in one extent in the parent user namespace.
> >
> > I don't see anything fundamentally wrong with relaxing this
> > restriction, but more code does have more room for bugs to hide.
> >
> > What is the advantage of relaxing this restriction?
> 
> we are running rootless containers in a namespace created with
> newuidmap/newgidmap where the mappings look like:
> 
> $ cat /proc/self/uid_map
> 0       1000          1
> 1     110000      65536
> 
> users are allowed to create child user namespaces and specify the
> mappings to use.  Doing so, they often hit the issue that the mappings
> cannot overlap multiple extents in the parent user namespace.
> 
> The issue could be completely addressed in user space, but to me it
> looks like an implementation detail that user space should not know
> about.
> In addition, it would also be slower (additional read of the current
> uid_map and gid_map files) and must be implemented separately in each
> container runtime.
> 
> >> $ cat /proc/self/uid_map
> >>          0       1000          1
> >>          1     110000      65536
> >> $ unshare -U sleep 100 &
> >> [1] 1552
> >> $ printf "0 0 100\n" | tee /proc/$!/uid_map
> >> 0 0 100
> >> $ cat /proc/$!/uid_map
> >>          0          0          1
> >>          1          1         99
> >>
> >> Signed-off-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx>
> >> ---
> >>  kernel/user_namespace.c | 62 ++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 52 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> >> index 87804e0371fe..b5542be2bd0a 100644
> >> --- a/kernel/user_namespace.c
> >> +++ b/kernel/user_namespace.c
> >> @@ -706,6 +706,41 @@ const struct seq_operations proc_projid_seq_operations = {
> >>  	.show = projid_m_show,
> >>  };
> >>  
> >> +static void split_overlapping_mappings(struct uid_gid_map *parent_map,
> >> +				       struct uid_gid_extent *extent,
> >> +				       struct uid_gid_extent *overflow_extent)
> >> +{
> >> +	unsigned int idx;
> >> +
> >> +	overflow_extent->first = (u32) -1;
> >> +
> >> +	/* Split extent if it not fully contained in an extent from parent_map.  */
> >> +	for (idx = 0; idx < parent_map->nr_extents; idx++) {
> >
> > Ouch!
> >
> > For the larger tree we perform binary searches typically and
> > here you are walking every entry unconditionally.
> >
> > It looks like this makes the write O(N^2) from O(NlogN)
> > which for a user facing function is not desirable.
> >
> > I think something like insert_and_split_extent may be ok.
> > Incorporating your loop and the part that inserts an element.
> >
> > As written this almost doubles the complexity of the code,
> > as well as making it perform much worse.  Which is a problem.
> 
> I've attempted to implement the new functionality at input validation
> time to not touch the existing security checks.
> 
> I've thought the pattern for iterating the extents was fine as I've
> taken it from mappings_overlap (even if it is used differently on an
> unsorted array).
> 
> Thanks for the hint, I'll move the new logic when map_id_range_down() is
> used and I'll send a v2.

Hi,

sorry if I miseed it.  Did you ever send a v2?