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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
> >>>>>
> >>>>>>
> >>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>>> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> >>>>>
> >
> > We had a very similar report from kernel-test-robot.
> >
> > https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@xxxxxxxxx/
> >
> > 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@xxxxxxxxxxxxxxx>" ?
> >
> > 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.
