| Message ID | 20230502-agenda-regeln-04d2573bd0fd@brauner (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: use vfs setgid helper | expand |
> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: > > We've aligned setgid behavior over multiple kernel releases. The details > can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of > git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and > commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of > git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). > Consistent setgid stripping behavior is now encapsulated in the > setattr_should_drop_sgid() helper which is used by all filesystems that > strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is > raised in e.g., chown_common() and is subject to the > setattr_should_drop_sgid() check to determine whether the setgid bit can > be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it > will cause notify_change() to strip it even if the caller had the > necessary privileges to retain it. Ensure that nfsd only raises > ATR_KILL_SGID if the caller lacks the necessary privileges to retain the > setgid bit. > > Without this patch the setgid stripping tests in LTP will fail: > >> As you can see, the problem is S_ISGID (0002000) was dropped on a >> non-group-executable file while chown was invoked by super-user, while > > [...] > >> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > > [...] > >> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > > With this patch all tests pass. > > Reported-by: Sherry Yang <sherry.yang@oracle.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> There are some similar fstests failures this fix might address. I've applied this patch to the nfsd-fixes tree for broader testing. Thanks, Christian and Sherry! > --- > ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02 > INFO: ltp-pan reported all tests PASS > LTP Version: 20230127-112-gf41e8a2fa > > ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02 > INFO: ltp-pan reported all tests PASS > LTP Version: 20230127-112-gf41e8a2fa > > ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms > FSTYP -- nfs > PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023 > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch > MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch > Passed all 41 tests > --- > fs/nfsd/vfs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index bb9d47172162..c4ef24c5ffd0 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) > iap->ia_mode &= ~S_ISGID; > } else { > /* set ATTR_KILL_* bits and let VFS handle it */ > - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID); > + iap->ia_valid |= ATTR_KILL_SUID; > + iap->ia_valid |= > + setattr_should_drop_sgid(&nop_mnt_idmap, inode); > } > } > } > -- > 2.34.1 > -- Chuck Lever
> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: > >> >> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: >> >> We've aligned setgid behavior over multiple kernel releases. The details >> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of >> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and >> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of >> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). >> Consistent setgid stripping behavior is now encapsulated in the >> setattr_should_drop_sgid() helper which is used by all filesystems that >> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is >> raised in e.g., chown_common() and is subject to the >> setattr_should_drop_sgid() check to determine whether the setgid bit can >> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it >> will cause notify_change() to strip it even if the caller had the >> necessary privileges to retain it. Ensure that nfsd only raises >> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the >> setgid bit. >> >> Without this patch the setgid stripping tests in LTP will fail: >> >>> As you can see, the problem is S_ISGID (0002000) was dropped on a >>> non-group-executable file while chown was invoked by super-user, while >> >> [...] >> >>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >> >> [...] >> >>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >> >> With this patch all tests pass. >> >> Reported-by: Sherry Yang <sherry.yang@oracle.com> >> Signed-off-by: Christian Brauner <brauner@kernel.org> > > There are some similar fstests failures this fix might address. > > I've applied this patch to the nfsd-fixes tree for broader > testing. ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! Did I apply this patch to the wrong kernel? > Thanks, Christian and Sherry! > > >> --- >> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02 >> INFO: ltp-pan reported all tests PASS >> LTP Version: 20230127-112-gf41e8a2fa >> >> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02 >> INFO: ltp-pan reported all tests PASS >> LTP Version: 20230127-112-gf41e8a2fa >> >> ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms >> FSTYP -- nfs >> PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023 >> MKFS_OPTIONS -- 127.0.0.1:/nfsscratch >> MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch >> Passed all 41 tests >> --- >> fs/nfsd/vfs.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >> index bb9d47172162..c4ef24c5ffd0 100644 >> --- a/fs/nfsd/vfs.c >> +++ b/fs/nfsd/vfs.c >> @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) >> iap->ia_mode &= ~S_ISGID; >> } else { >> /* set ATTR_KILL_* bits and let VFS handle it */ >> - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID); >> + iap->ia_valid |= ATTR_KILL_SUID; >> + iap->ia_valid |= >> + setattr_should_drop_sgid(&nop_mnt_idmap, inode); >> } >> } >> } >> -- >> 2.34.1 >> > > -- > Chuck Lever -- Chuck Lever
On Tue, 2023-05-02 at 15:36 +0200, Christian Brauner wrote: > We've aligned setgid behavior over multiple kernel releases. The details > can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of > git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and > commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of > git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). > Consistent setgid stripping behavior is now encapsulated in the > setattr_should_drop_sgid() helper which is used by all filesystems that > strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is > raised in e.g., chown_common() and is subject to the > setattr_should_drop_sgid() check to determine whether the setgid bit can > be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it > will cause notify_change() to strip it even if the caller had the > necessary privileges to retain it. Ensure that nfsd only raises > ATR_KILL_SGID if the caller lacks the necessary privileges to retain the > setgid bit. > > Without this patch the setgid stripping tests in LTP will fail: > > > As you can see, the problem is S_ISGID (0002000) was dropped on a > > non-group-executable file while chown was invoked by super-user, while > > [...] > > > fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > > [...] > > > chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > > With this patch all tests pass. > > Reported-by: Sherry Yang <sherry.yang@oracle.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02 > INFO: ltp-pan reported all tests PASS > LTP Version: 20230127-112-gf41e8a2fa > > ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02 > INFO: ltp-pan reported all tests PASS > LTP Version: 20230127-112-gf41e8a2fa > > ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms > FSTYP -- nfs > PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023 > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch > MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch > Passed all 41 tests > --- > fs/nfsd/vfs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index bb9d47172162..c4ef24c5ffd0 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) > iap->ia_mode &= ~S_ISGID; > } else { > /* set ATTR_KILL_* bits and let VFS handle it */ > - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID); > + iap->ia_valid |= ATTR_KILL_SUID; > + iap->ia_valid |= > + setattr_should_drop_sgid(&nop_mnt_idmap, inode); > } > } > } Looks good to me. The thread should have assumed the user's creds by this point. Reviewed-by: Jeff Layton <jlayton@kernel.org>
> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: > > > >> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: >> >>> >>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: >>> >>> We've aligned setgid behavior over multiple kernel releases. The details >>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of >>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and >>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of >>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). >>> Consistent setgid stripping behavior is now encapsulated in the >>> setattr_should_drop_sgid() helper which is used by all filesystems that >>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is >>> raised in e.g., chown_common() and is subject to the >>> setattr_should_drop_sgid() check to determine whether the setgid bit can >>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it >>> will cause notify_change() to strip it even if the caller had the >>> necessary privileges to retain it. Ensure that nfsd only raises >>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the >>> setgid bit. >>> >>> Without this patch the setgid stripping tests in LTP will fail: >>> >>>> As you can see, the problem is S_ISGID (0002000) was dropped on a >>>> non-group-executable file while chown was invoked by super-user, while >>> >>> [...] >>> >>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>> >>> [...] >>> >>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>> >>> With this patch all tests pass. >>> >>> Reported-by: Sherry Yang <sherry.yang@oracle.com> >>> Signed-off-by: Christian Brauner <brauner@kernel.org> >> >> There are some similar fstests failures this fix might address. >> >> I've applied this patch to the nfsd-fixes tree for broader >> testing. > > ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! > > Did I apply this patch to the wrong kernel? setattr_should_drop_sgid() is not available to callers built as modules. It needs an EXPORT_SYMBOL or _GPL. >> Thanks, Christian and Sherry! >> >> >>> --- >>> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02 >>> INFO: ltp-pan reported all tests PASS >>> LTP Version: 20230127-112-gf41e8a2fa >>> >>> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02 >>> INFO: ltp-pan reported all tests PASS >>> LTP Version: 20230127-112-gf41e8a2fa >>> >>> ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms >>> FSTYP -- nfs >>> PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023 >>> MKFS_OPTIONS -- 127.0.0.1:/nfsscratch >>> MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch >>> Passed all 41 tests >>> --- >>> fs/nfsd/vfs.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >>> index bb9d47172162..c4ef24c5ffd0 100644 >>> --- a/fs/nfsd/vfs.c >>> +++ b/fs/nfsd/vfs.c >>> @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) >>> iap->ia_mode &= ~S_ISGID; >>> } else { >>> /* set ATTR_KILL_* bits and let VFS handle it */ >>> - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID); >>> + iap->ia_valid |= ATTR_KILL_SUID; >>> + iap->ia_valid |= >>> + setattr_should_drop_sgid(&nop_mnt_idmap, inode); >>> } >>> } >>> } >>> -- >>> 2.34.1 >>> >> >> -- >> Chuck Lever > > > -- > Chuck Lever -- Chuck Lever
On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote: > > > > On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > > > >> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: > >> > >>> > >>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: > >>> > >>> We've aligned setgid behavior over multiple kernel releases. The details > >>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of > >>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and > >>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of > >>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). > >>> Consistent setgid stripping behavior is now encapsulated in the > >>> setattr_should_drop_sgid() helper which is used by all filesystems that > >>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is > >>> raised in e.g., chown_common() and is subject to the > >>> setattr_should_drop_sgid() check to determine whether the setgid bit can > >>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it > >>> will cause notify_change() to strip it even if the caller had the > >>> necessary privileges to retain it. Ensure that nfsd only raises > >>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the > >>> setgid bit. > >>> > >>> Without this patch the setgid stripping tests in LTP will fail: > >>> > >>>> As you can see, the problem is S_ISGID (0002000) was dropped on a > >>>> non-group-executable file while chown was invoked by super-user, while > >>> > >>> [...] > >>> > >>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > >>> > >>> [...] > >>> > >>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > >>> > >>> With this patch all tests pass. > >>> > >>> Reported-by: Sherry Yang <sherry.yang@oracle.com> > >>> Signed-off-by: Christian Brauner <brauner@kernel.org> > >> > >> There are some similar fstests failures this fix might address. > >> > >> I've applied this patch to the nfsd-fixes tree for broader > >> testing. > > > > ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! > > > > Did I apply this patch to the wrong kernel? > > setattr_should_drop_sgid() is not available to callers built as > modules. It needs an EXPORT_SYMBOL or _GPL. Hey Chuck, Thanks for taking a look! The required export is part of commit 4f704d9a835 ("nfs: use vfs setgid helper") which is in current mainline as of Monday this week which was part of my v6.4/vfs.misc PR that was merged. So this should all work fine on mainline. Seems I didn't use --base which is why that info was missing. Thanks! Christian
Hi Christian, On 03/05/23 12:30 pm, Christian Brauner wrote: > On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote: >> >> >>> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>> >>> >>> >>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>>> >>>>> >>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: >>>>> >>>>> We've aligned setgid behavior over multiple kernel releases. The details >>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of >>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and >>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of >>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). >>>>> Consistent setgid stripping behavior is now encapsulated in the >>>>> setattr_should_drop_sgid() helper which is used by all filesystems that >>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is >>>>> raised in e.g., chown_common() and is subject to the >>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can >>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it >>>>> will cause notify_change() to strip it even if the caller had the >>>>> necessary privileges to retain it. Ensure that nfsd only raises >>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the >>>>> setgid bit. >>>>> >>>>> Without this patch the setgid stripping tests in LTP will fail: >>>>> >>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a >>>>>> non-group-executable file while chown was invoked by super-user, while >>>>> >>>>> [...] >>>>> >>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>> >>>>> [...] >>>>> >>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>> >>>>> With this patch all tests pass. >>>>> >>>>> Reported-by: Sherry Yang <sherry.yang@oracle.com> >>>>> Signed-off-by: Christian Brauner <brauner@kernel.org> >>>> We had a very similar report from kernel-test-robot. https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@intel.com/ Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks") And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y So would it be better to tag it to stable by adding "Cc: <stable@vger.kernel.org>" ? Thanks, Harshit >>>> There are some similar fstests failures this fix might address. >>>> >>>> I've applied this patch to the nfsd-fixes tree for broader >>>> testing. >>> >>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! >>> >>> Did I apply this patch to the wrong kernel? >> >> setattr_should_drop_sgid() is not available to callers built as >> modules. It needs an EXPORT_SYMBOL or _GPL. > > Hey Chuck, > > Thanks for taking a look! > The required export is part of > commit 4f704d9a835 ("nfs: use vfs setgid helper") > which is in current mainline as of Monday this week which was part of my > v6.4/vfs.misc PR that was merged. > > So this should all work fine on mainline. Seems I didn't use --base > which is why that info was missing. > > Thanks! > Christian > >
> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > > Hi Christian, > > On 03/05/23 12:30 pm, Christian Brauner wrote: >> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote: >>> >>> >>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>>> >>>> >>>> >>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>>>> >>>>>> >>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: >>>>>> >>>>>> We've aligned setgid behavior over multiple kernel releases. The details >>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and >>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). >>>>>> Consistent setgid stripping behavior is now encapsulated in the >>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that >>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is >>>>>> raised in e.g., chown_common() and is subject to the >>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can >>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it >>>>>> will cause notify_change() to strip it even if the caller had the >>>>>> necessary privileges to retain it. Ensure that nfsd only raises >>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the >>>>>> setgid bit. >>>>>> >>>>>> Without this patch the setgid stripping tests in LTP will fail: >>>>>> >>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a >>>>>>> non-group-executable file while chown was invoked by super-user, while >>>>>> >>>>>> [...] >>>>>> >>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>>> >>>>>> [...] >>>>>> >>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>>> >>>>>> With this patch all tests pass. >>>>>> >>>>>> Reported-by: Sherry Yang <sherry.yang@oracle.com> >>>>>> Signed-off-by: Christian Brauner <brauner@kernel.org> >>>>> > > We had a very similar report from kernel-test-robot. > > https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@intel.com/ > > Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks") > > And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y > > So would it be better to tag it to stable by adding "Cc: <stable@vger.kernel.org>" ? > > Thanks, > Harshit > >>>>> There are some similar fstests failures this fix might address. >>>>> >>>>> I've applied this patch to the nfsd-fixes tree for broader >>>>> testing. >>>> >>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! >>>> >>>> Did I apply this patch to the wrong kernel? >>> >>> setattr_should_drop_sgid() is not available to callers built as >>> modules. It needs an EXPORT_SYMBOL or _GPL. >> Hey Chuck, >> Thanks for taking a look! >> The required export is part of >> commit 4f704d9a835 ("nfs: use vfs setgid helper") >> which is in current mainline as of Monday this week which was part of my >> v6.4/vfs.misc PR that was merged. >> So this should all work fine on mainline. Seems I didn't use --base >> which is why that info was missing. >> Thanks! >> Christian I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by and a Cc: stable. -- Chuck Lever
On Wed, May 03, 2023 at 02:05:48PM +0000, Chuck Lever III wrote: > > > > On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > > > > Hi Christian, > > > > On 03/05/23 12:30 pm, Christian Brauner wrote: > >> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote: > >>> > >>> > >>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: > >>>> > >>>> > >>>> > >>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: > >>>>> > >>>>>> > >>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: > >>>>>> > >>>>>> We've aligned setgid behavior over multiple kernel releases. The details > >>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of > >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and > >>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of > >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). > >>>>>> Consistent setgid stripping behavior is now encapsulated in the > >>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that > >>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is > >>>>>> raised in e.g., chown_common() and is subject to the > >>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can > >>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it > >>>>>> will cause notify_change() to strip it even if the caller had the > >>>>>> necessary privileges to retain it. Ensure that nfsd only raises > >>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the > >>>>>> setgid bit. > >>>>>> > >>>>>> Without this patch the setgid stripping tests in LTP will fail: > >>>>>> > >>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a > >>>>>>> non-group-executable file while chown was invoked by super-user, while > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 > >>>>>> > >>>>>> With this patch all tests pass. > >>>>>> > >>>>>> Reported-by: Sherry Yang <sherry.yang@oracle.com> > >>>>>> Signed-off-by: Christian Brauner <brauner@kernel.org> > >>>>> > > > > We had a very similar report from kernel-test-robot. > > > > https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@intel.com/ > > > > Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks") > > > > And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y > > > > So would it be better to tag it to stable by adding "Cc: <stable@vger.kernel.org>" ? > > > > Thanks, > > Harshit > > > >>>>> There are some similar fstests failures this fix might address. > >>>>> > >>>>> I've applied this patch to the nfsd-fixes tree for broader > >>>>> testing. > >>>> > >>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! > >>>> > >>>> Did I apply this patch to the wrong kernel? > >>> > >>> setattr_should_drop_sgid() is not available to callers built as > >>> modules. It needs an EXPORT_SYMBOL or _GPL. > >> Hey Chuck, > >> Thanks for taking a look! > >> The required export is part of > >> commit 4f704d9a835 ("nfs: use vfs setgid helper") > >> which is in current mainline as of Monday this week which was part of my > >> v6.4/vfs.misc PR that was merged. > >> So this should all work fine on mainline. Seems I didn't use --base > >> which is why that info was missing. > >> Thanks! > >> Christian > > I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by > and a Cc: stable. Thanks all! Appreciate the Cc: stable fixup.
> On May 3, 2023, at 7:05 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: > > > >> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: >> >> Hi Christian, >> >> On 03/05/23 12:30 pm, Christian Brauner wrote: >>> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote: >>>> >>>> >>>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>>>> >>>>> >>>>> >>>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>>>>> >>>>>>> >>>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: >>>>>>> >>>>>>> We've aligned setgid behavior over multiple kernel releases. The details >>>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of >>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and >>>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of >>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). >>>>>>> Consistent setgid stripping behavior is now encapsulated in the >>>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that >>>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is >>>>>>> raised in e.g., chown_common() and is subject to the >>>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can >>>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it >>>>>>> will cause notify_change() to strip it even if the caller had the >>>>>>> necessary privileges to retain it. Ensure that nfsd only raises >>>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the >>>>>>> setgid bit. >>>>>>> >>>>>>> Without this patch the setgid stripping tests in LTP will fail: >>>>>>> >>>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a >>>>>>>> non-group-executable file while chown was invoked by super-user, while >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>>>> >>>>>>> With this patch all tests pass. >>>>>>> >>>>>>> Reported-by: Sherry Yang <sherry.yang@oracle.com> >>>>>>> Signed-off-by: Christian Brauner <brauner@kernel.org> >>>>>> >> >> We had a very similar report from kernel-test-robot. >> >> https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@intel.com/ >> >> Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks") >> >> And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y >> >> So would it be better to tag it to stable by adding "Cc: <stable@vger.kernel.org>" ? >> >> Thanks, >> Harshit >> >>>>>> There are some similar fstests failures this fix might address. >>>>>> >>>>>> I've applied this patch to the nfsd-fixes tree for broader >>>>>> testing. >>>>> >>>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! >>>>> >>>>> Did I apply this patch to the wrong kernel? >>>> >>>> setattr_should_drop_sgid() is not available to callers built as >>>> modules. It needs an EXPORT_SYMBOL or _GPL. >>> Hey Chuck, >>> Thanks for taking a look! >>> The required export is part of >>> commit 4f704d9a835 ("nfs: use vfs setgid helper") >>> which is in current mainline as of Monday this week which was part of my >>> v6.4/vfs.misc PR that was merged. >>> So this should all work fine on mainline. Seems I didn't use --base >>> which is why that info was missing. >>> Thanks! >>> Christian > > I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by > and a Cc: stable. Hi Chuck, Just following up to see if you know when this patch will go to linux mainline and linux-stable? Thanks, Sherry > > > -- > Chuck Lever > >
> On May 15, 2023, at 12:34 PM, Sherry Yang <sherry.yang@oracle.com> wrote: > > > >> On May 3, 2023, at 7:05 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: >> >> >> >>> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: >>> >>> Hi Christian, >>> >>> On 03/05/23 12:30 pm, Christian Brauner wrote: >>>> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote: >>>>> >>>>> >>>>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote: >>>>>>>> >>>>>>>> We've aligned setgid behavior over multiple kernel releases. The details >>>>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of >>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and >>>>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of >>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). >>>>>>>> Consistent setgid stripping behavior is now encapsulated in the >>>>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that >>>>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is >>>>>>>> raised in e.g., chown_common() and is subject to the >>>>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can >>>>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it >>>>>>>> will cause notify_change() to strip it even if the caller had the >>>>>>>> necessary privileges to retain it. Ensure that nfsd only raises >>>>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the >>>>>>>> setgid bit. >>>>>>>> >>>>>>>> Without this patch the setgid stripping tests in LTP will fail: >>>>>>>> >>>>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a >>>>>>>>> non-group-executable file while chown was invoked by super-user, while >>>>>>>> >>>>>>>> [...] >>>>>>>> >>>>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>>>>> >>>>>>>> [...] >>>>>>>> >>>>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 >>>>>>>> >>>>>>>> With this patch all tests pass. >>>>>>>> >>>>>>>> Reported-by: Sherry Yang <sherry.yang@oracle.com> >>>>>>>> Signed-off-by: Christian Brauner <brauner@kernel.org> >>>>>>> >>> >>> We had a very similar report from kernel-test-robot. >>> >>> https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@intel.com/ >>> >>> Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks") >>> >>> And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y >>> >>> So would it be better to tag it to stable by adding "Cc: <stable@vger.kernel.org>" ? >>> >>> Thanks, >>> Harshit >>> >>>>>>> There are some similar fstests failures this fix might address. >>>>>>> >>>>>>> I've applied this patch to the nfsd-fixes tree for broader >>>>>>> testing. >>>>>> >>>>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined! >>>>>> >>>>>> Did I apply this patch to the wrong kernel? >>>>> >>>>> setattr_should_drop_sgid() is not available to callers built as >>>>> modules. It needs an EXPORT_SYMBOL or _GPL. >>>> Hey Chuck, >>>> Thanks for taking a look! >>>> The required export is part of >>>> commit 4f704d9a835 ("nfs: use vfs setgid helper") >>>> which is in current mainline as of Monday this week which was part of my >>>> v6.4/vfs.misc PR that was merged. >>>> So this should all work fine on mainline. Seems I didn't use --base >>>> which is why that info was missing. >>>> Thanks! >>>> Christian >> >> I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by >> and a Cc: stable. > > Hi Chuck, > > Just following up to see if you know when this patch will go to linux mainline and linux-stable? Howdy- mentioned above: applied to nfsd-next for the v6.5 merge window, which will open in several weeks. I added a Cc: stable, so once it is in 6.5-rc it will be automatically backported into the stable kernels. -- Chuck Lever
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index bb9d47172162..c4ef24c5ffd0 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) iap->ia_mode &= ~S_ISGID; } else { /* set ATTR_KILL_* bits and let VFS handle it */ - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID); + iap->ia_valid |= ATTR_KILL_SUID; + iap->ia_valid |= + setattr_should_drop_sgid(&nop_mnt_idmap, inode); } } }
We've aligned setgid behavior over multiple kernel releases. The details can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux"). Consistent setgid stripping behavior is now encapsulated in the setattr_should_drop_sgid() helper which is used by all filesystems that strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is raised in e.g., chown_common() and is subject to the setattr_should_drop_sgid() check to determine whether the setgid bit can be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it will cause notify_change() to strip it even if the caller had the necessary privileges to retain it. Ensure that nfsd only raises ATR_KILL_SGID if the caller lacks the necessary privileges to retain the setgid bit. Without this patch the setgid stripping tests in LTP will fail: > As you can see, the problem is S_ISGID (0002000) was dropped on a > non-group-executable file while chown was invoked by super-user, while [...] > fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 [...] > chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700 With this patch all tests pass. Reported-by: Sherry Yang <sherry.yang@oracle.com> Signed-off-by: Christian Brauner <brauner@kernel.org> --- ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02 INFO: ltp-pan reported all tests PASS LTP Version: 20230127-112-gf41e8a2fa ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02 INFO: ltp-pan reported all tests PASS LTP Version: 20230127-112-gf41e8a2fa ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms FSTYP -- nfs PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023 MKFS_OPTIONS -- 127.0.0.1:/nfsscratch MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch Passed all 41 tests --- fs/nfsd/vfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)