Re: [PATCH] vfs: Use upper filesystem inode in bprm_fill_uid()

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Feb 13, 2017 at 10:01:09PM +0200, Amir Goldstein wrote:
> On Mon, Feb 13, 2017 at 9:06 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Mon, Feb 13, 2017 at 08:07:16PM +0200, Amir Goldstein wrote:
> >> On Mon, Feb 13, 2017 at 6:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > Right now bprm_fill_uid() uses inode fetched from file_inode(bprm->file).
> >> > This in turn returns inode of lower filesystem (in a stacked filesystem
> >> > setup).
> >> >
> >> > I was playing with modified patches of shiftfs posted by james bottomley
> >> > and realized that through shiftfs setuid bit does not take effect. And
> >> > reason being that we fetch uid/gid from inode of lower fs (and not from
> >> > shiftfs inode). And that results in following checks failing.
> >> >
> >> > /* We ignore suid/sgid if there are no mappings for them in the ns */
> >> > if (!kuid_has_mapping(bprm->cred->user_ns, uid) ||
> >> >     !kgid_has_mapping(bprm->cred->user_ns, gid))
> >> >         return;
> >> >
> >> > uid/gid fetched from lower fs inode might not be mapped inside the user
> >> > namespace of container. So we need to look at uid/gid fetched from
> >> > upper filesystem (shiftfs in this particular case) and these should be
> >> > mapped and setuid bit can take affect.
> >>
> >> This change is consistent with may_linkat() -> safe_hardlink_source()
> >> and with all instances of notify_change() in fs/open.c, but is not
> >> consistent with file_remove_privs() -> ...
> >> should_remove_suid()/notify_change()
> >> which also uses file_inode()
> >
> > I did a quick grep to figure out who is calling file_remove_privs(). Looks
> > like it is being called by underlying filesystem (ext4, xfs, ntfs, fuse).
> > If that's the case, we probably want to use the dentry and inode of
> > underlying filesystem and not recurse back to top level filesystem?
> >
> 
> No I think you are right and there is no problem with file_remove_privs().
> 
> Not sure about do_coredump(), though, which uses file_inode(cprm.file).
> Other places in vfs that check inode->i_uid are even harder to follow.

Hmm.., file_inode(cprm.file) might be a problem too with shiftfs. For
now I will post V2 of this patch and follow this core dump issue
separately.

Vivek