Re: [PATCH v2 04/28] fsuidgid: add fsid mapping helpers

[Christian Brauner <christian.brauner@ubuntu.com>]

On Fri, Feb 14, 2020 at 08:11:36PM +0100, Jann Horn wrote:
> On Fri, Feb 14, 2020 at 7:37 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> > This adds a set of helpers to translate between kfsuid/kfsgid and their
> > userspace fsuid/fsgid counter parts relative to a given user namespace.
> >
> > - kuid_t make_kfsuid(struct user_namespace *from, uid_t fsuid)
> >   Maps a user-namespace fsuid pair into a kfsuid.
> >   If no fsuid mappings have been written it behaves identical to calling
> >   make_kuid(). This ensures backwards compatibility for workloads unaware
> >   or not in need of fsid mappings.
> [...]
> > +#ifdef CONFIG_USER_NS_FSID
> > +/**
> > + *     make_kfsuid - Map a user-namespace fsuid pair into a kuid.
> > + *     @ns:  User namespace that the fsuid is in
> > + *     @fsuid: User identifier
> > + *
> > + *     Maps a user-namespace fsuid pair into a kernel internal kfsuid,
> > + *     and returns that kfsuid.
> > + *
> > + *     When there is no mapping defined for the user-namespace kfsuid
> > + *     pair INVALID_UID is returned.  Callers are expected to test
> > + *     for and handle INVALID_UID being returned.  INVALID_UID
> > + *     may be tested for using uid_valid().
> > + */
> > +kuid_t make_kfsuid(struct user_namespace *ns, uid_t fsuid)
> > +{
> > +       unsigned extents = ns->fsuid_map.nr_extents;
> > +       smp_rmb();
> > +
> > +       /* Map the fsuid to a global kernel fsuid */
> > +       if (extents == 0)
> > +               return KUIDT_INIT(map_id_down(&ns->uid_map, fsuid));
> > +
> > +       return KUIDT_INIT(map_id_down(&ns->fsuid_map, fsuid));
> > +}
> > +EXPORT_SYMBOL(make_kfsuid);
> 
> What effect is this fallback going to have for nested namespaces?
> 
> Let's say we have an outer namespace N1 with this uid_map:
> 
>     0 100000 65535
> 
> and with this fsuid_map:
> 
>     0 300000 65535
> 
> Now from in there, a process that is not aware of the existence of
> fsuid mappings creates a new user namespace N2 with the following
> uid_map:
> 
>     0 1000 1
> 
> At this point, if a process in N2 does chown("foo", 0, 0), is that
> going to make "foo" owned by kuid 101000, which isn't even mapped in
> N1?

So Jann just made a clever suggestion that would solve this problem fsid
maps can only be written if the corresponding id mapping has been
written and fsid mappings will only have an effect once the
corresponding id mapping has been written. That sounds rather sane to
me.

Christian