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@xxxxxxxxxx> 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@xxxxxxxxxx> 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
