From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:36492 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751731AbdBMUiV (ORCPT ); Mon, 13 Feb 2017 15:38:21 -0500 Date: Mon, 13 Feb 2017 15:38:20 -0500 From: Vivek Goyal To: Amir Goldstein Cc: linux-fsdevel , "Eric W. Biederman" , Al Viro , James Bottomley , Miklos Szeredi Subject: Re: [PATCH] vfs: Use upper filesystem inode in bprm_fill_uid() Message-ID: <20170213203820.GF2065@redhat.com> References: <20170213161834.GA2065@redhat.com> <20170213190626.GE2065@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Feb 13, 2017 at 10:01:09PM +0200, Amir Goldstein wrote: > On Mon, Feb 13, 2017 at 9:06 PM, Vivek Goyal 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 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