[PATCH] nfsd: use vfs setgid helper

[PATCH] nfsd: use vfs setgid helper

[Christian Brauner]

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);
 		}
 	}
 }
-- 
2.34.1

Re: [PATCH] nfsd: use vfs setgid helper

[Chuck Lever III]

> 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

Re: [PATCH] nfsd: use vfs setgid helper

[Chuck Lever III]

> 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

Re: [PATCH] nfsd: use vfs setgid helper

[Chuck Lever III]

> 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

Re: [PATCH] nfsd: use vfs setgid helper

[Christian Brauner]

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

Re: [PATCH] nfsd: use vfs setgid helper

[Harshit Mogalapalli]

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

Re: [PATCH] nfsd: use vfs setgid helper

[Chuck Lever III]

> 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

Re: [PATCH] nfsd: use vfs setgid helper

[Christian Brauner]

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.

Re: [PATCH] nfsd: use vfs setgid helper

[Sherry Yang]

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

Re: [PATCH] nfsd: use vfs setgid helper

[Chuck Lever III]

> 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

Re: [PATCH] nfsd: use vfs setgid helper

[Jeff Layton]

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>