Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Zorro Lang <zlang@redhat.com>]

On Thu, Apr 14, 2022 at 08:50:07AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 13, 2022 at 11:44:01PM +0800, Zorro Lang wrote:
> > On Wed, Apr 13, 2022 at 10:58:41AM +0300, Amir Goldstein wrote:
> > > On Wed, Apr 13, 2022 at 1:18 AM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Mon, Apr 11, 2022 at 03:54:42PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > >
> > > > > fallocate changes file contents, so make sure that we drop privileges
> > > > > and file capabilities after each fallocate operation.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > >  tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/834.out |   33 +++++++++++++
> > > > >  tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/835.out |   33 +++++++++++++
> > > > >  tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/836.out |   33 +++++++++++++
> > > > >  tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/837.out |   33 +++++++++++++
> > > > >  tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/838.out |   33 +++++++++++++
> > > > >  tests/generic/839     |   77 ++++++++++++++++++++++++++++++
> > > > >  tests/generic/839.out |   13 +++++
> > > > >  12 files changed, 890 insertions(+)
> > > > >  create mode 100755 tests/generic/834
> > > > >  create mode 100644 tests/generic/834.out
> > > > >  create mode 100755 tests/generic/835
> > > > >  create mode 100644 tests/generic/835.out
> > > > >  create mode 100755 tests/generic/836
> > > > >  create mode 100644 tests/generic/836.out
> > > > >  create mode 100755 tests/generic/837
> > > > >  create mode 100644 tests/generic/837.out
> > > > >  create mode 100755 tests/generic/838
> > > > >  create mode 100644 tests/generic/838.out
> > > > >  create mode 100755 tests/generic/839
> > > > >  create mode 100755 tests/generic/839.out
> > > > >
> > > > >
> > > > > diff --git a/tests/generic/834 b/tests/generic/834
> > > > > new file mode 100755
> > > > > index 00000000..9302137b
> > > > > --- /dev/null
> > > > > +++ b/tests/generic/834
> > > > > @@ -0,0 +1,127 @@
> > > > > +#! /bin/bash
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > > > +#
> > > > > +# FS QA Test No. 834
> > > > > +#
> > > > > +# Functional test for dropping suid and sgid bits as part of a fallocate.
> > > > > +#
> > > > > +. ./common/preamble
> > > > > +_begin_fstest auto clone quick
> > > > > +
> > > > > +# Override the default cleanup function.
> > > > > +_cleanup()
> > > > > +{
> > > > > +     cd /
> > > > > +     rm -r -f $tmp.* $junk_dir
> > > > > +}
> > > > > +
> > > > > +# Import common functions.
> > > > > +. ./common/filter
> > > > > +. ./common/reflink
> > > > > +
> > > > > +# real QA test starts here
> > > > > +
> > > > > +# Modify as appropriate.
> > > > > +_supported_fs xfs btrfs ext4
> > > >
> > > > So we have more cases will break downstream XFS testing :)
> > > 
> > > Funny you should mention that.
> > > I was going to propose an RFC for something like:
> > > 
> > > _fixed_by_kernel_commit fbe7e5200365 "xfs: fallocate() should call
> > > file_modified()"
> > > 
> > > The first thing that could be done with this standard annotation is print a
> > > hint on failure, like LTP does:
> > > 
> > > HINT: You _MAY_ be missing kernel fixes:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbe7e5200365
> > 
> > I think it's not difficult to implement this behavior in xfstests. Generally if
> > a case covers a known bug, we record the patch commit in case description.
> 
> It's not hard, but it's a treewide change to identify all the fstests
> that are regression fixes (or at least mention a commit hash) and well
> beyond the scope of adding tests for a new fallocate security behavior.
> 
> In fact, it's an *entirely new project*.  One that I don't have time to
> take on myself as a condition for getting *this* patch merged.

Hi Darrick, that's another story, you don't need to worry about that in this case :)
I'd like to ack this patch, but hope to move it from generic/ to shared/ . Maybe
Eryu can help to move it, or I can do that after I get the push permission.

The reason why I intend moving it to shared is:
Although we are trying to get rid of tests/shared/, but the tests/shared/ still help to
remind us what cases are still not real generic cases. We'll try to help all shared
cases to be generic. When the time is ready, I'd like to move this case to generic/
and change _supported_fs from "xfs btrfs ext4" to "generic".

Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks,
Zorro