Re: [PATCH] generic: update setgid tests

[Christian Brauner <brauner@kernel.org>]

On Tue, Jan 03, 2023 at 09:47:37AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 03, 2023 at 04:28:20PM +0100, Christian Brauner wrote:
> > Over mutiple kernel releases we have reworked setgid inheritance
> > significantly due to long-standing security issues, security issues that
> > were reintroduced after they were fixed, and the subtle and difficult
> > inheritance rules that plagued individual filesystems. We have lifted
> > setgid inheritance into the VFS proper in earlier patches. Starting with
> > kernel v6.2 we have made setgid inheritance consistent between the write
> > and setattr (ch{mod,own}) paths.
> > 
> > The gist of the requirement is that in order to inherit the setgid bit
> > the user needs to be in the group of the file or have CAP_FSETID in
> > their user namespace. Otherwise the setgid bit will be dropped
> > irregardless of the file's executability. Change the tests accordingly
> > and annotate them with the commits that changed the behavior.
> > 
> > Note, that only with v6.2 setgid inheritance works correctly for
> > overlayfs in the write path. Before this the setgid bit was always
> > retained.
> > 
> > Link: https://lore.kernel.org/linux-ext4/CAOQ4uxhmCgyorYVtD6=n=khqwUc=MPbZs+y=sqt09XbGoNm_tA@mail.gmail.com
> > Link: https://lore.kernel.org/linux-fsdevel/20221212112053.99208-1-brauner@kernel.org
> > Link: https://lore.kernel.org/linux-fsdevel/20221122142010.zchf2jz2oymx55qi@wittgenstein
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: Zorro Lang <zlang@redhat.com>
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > ---
> >  tests/generic/673     | 12 +++++++++---
> >  tests/generic/673.out |  8 ++++----
> >  tests/generic/683     | 11 ++++++++---
> >  tests/generic/683.out |  8 ++++----
> >  tests/generic/684     | 11 ++++++++---
> >  tests/generic/684.out |  8 ++++----
> >  tests/generic/685     | 11 ++++++++---
> >  tests/generic/685.out |  8 ++++----
> >  8 files changed, 49 insertions(+), 28 deletions(-)
> > 
> > diff --git a/tests/generic/673 b/tests/generic/673
> > index 6d1f49ea..1d8e4184 100755
> > --- a/tests/generic/673
> > +++ b/tests/generic/673
> > @@ -23,6 +23,12 @@ _require_scratch_reflink
> >  _scratch_mkfs >> $seqres.full
> >  _scratch_mount
> >  _require_congruent_file_oplen $SCRATCH_MNT 1048576
> > +
> > +# Due to multiple security issues and potential for subtle bugs around setgid
> > +# inheritance the rules in the write and chmod/chown paths have been made
> > +# consistent and are enforced by the VFS since kernel 6.2.
> > +_fixed_in_kernel_version "v6.2"
> 
> Uh... will this new behavior get ported to the LTS kernels too, or are

I plan on backporting this but I would like to wait for a little while
longer into the v6.2 cycle before doing this as everyone is just coming
out of the holiday season trying to catch up.

> we simply changing the kernel behavior here?

Yes, this is a behavioral change. I made sure to mention the risks and
reasons involved on the list along the patches and in the pull requests
that were merged.

> 
> Also, this patch does correct the new test failures that I saw as soon
> as I pulled 6.2-rc1, but it doesn't fix the finsert and fcollapse tests:

Indeed I have missed them because they were skipped:

generic/686       [not run] xfs_io finsert  failed (old kernel/wrong fs?)
generic/687       [not run] xfs_io fcollapse  failed (old kernel/wrong fs?)

I'll update the series accordingly. Thanks for pointing that out.

Christian