On Wed, Jun 15, 2022 at 11:28:26AM +0200, Christian Brauner wrote:
> When group ownership is changed a caller whose fsuid owns the inode can
> change the group of the inode to any group they are a member of. When
> searching through the caller's groups we failed to use the gid mapped
> according to the idmapped mount otherwise we fail to change ownership.
> Add a test for this.
>
> Cc: Seth Forshee <sforshee@xxxxxxxxxxxxxxxx>
> Cc: Amir Goldstein <amir73il@xxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Aleksa Sarai <cyphar@xxxxxxxxxx>
> Cc: <fstests@xxxxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> ---
> /* v2 */
> - Zorro Lang <zlang@xxxxxxxxxx>:
> - various minor fixes
>
> - Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>:
> - Expand test to also cover overlayfs on top of idmapped mounts.
>
> /* v3 */
> - Amir Goldstein <amir73il@xxxxxxxxx>:
> - Switch from calls to the mount binary to _overlay_mount_dirs helper.
>
> - Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>:
> - Expand test to also cover non-idmapped mounts.
> ---
We really make this test case become more complicated than its v1 version,
for covering overlayfs on top of idmapped mounts.
I'm wondering if it's worth splitting this case to two cases, one similar
as v1 to cover the original regression test, the other expand for overlayfs,
although that will cause some duplicate testing steps use?
If most of you prefer this v3 version, I'd like to get more review points.
Thanks,
Zorro
> tests/generic/692 | 195 ++++++++++++++++++++++++++++++++++++++++++
> tests/generic/692.out | 49 +++++++++++
> 2 files changed, 244 insertions(+)
> create mode 100755 tests/generic/692
> create mode 100644 tests/generic/692.out
>
> diff --git a/tests/generic/692 b/tests/generic/692
> new file mode 100755
> index 00000000..20579334
> --- /dev/null
> +++ b/tests/generic/692
> @@ -0,0 +1,195 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Christian Brauner (Microsoft). All Rights Reserved.
> +#
> +# FS QA Test 692
> +#
> +# Test that users can changed group ownership of a file they own to a group
> +# they are a member of.
> +#
> +# Regression test for commit:
> +#
> +# 168f91289340 ("fs: account for group membership")
> +#
> +. ./common/preamble
> +. ./common/overlay
> +_begin_fstest auto quick perms attr idmapped mount
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> + cd /
> + $UMOUNT_PROG $SCRATCH_MNT/target-mnt 2>/dev/null
> + $UMOUNT_PROG $SCRATCH_MNT/ovl-merge 2>/dev/null
> + $UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
> + rm -r -f $tmp.*
> +}
> +
> +# real QA test starts here
> +_supported_fs ^overlay
> +_require_extra_fs overlay
> +_supports_filetype $SCRATCH_MNT || _notrun "overlayfs test requires d_type"
> +_require_scratch
> +_require_chown
> +_require_idmapped_mounts
> +_require_test_program "vfs/mount-idmapped"
> +_require_user fsgqa2
> +_require_group fsgqa2
> +# Do this SECOND so that qa_user is fsgqa, and _user_do uses that account
> +_require_user fsgqa
> +_require_group fsgqa
> +
> +_scratch_mkfs >> $seqres.full
> +_scratch_mount
> +
> +user_foo=`id -u fsgqa`
> +group_foo=`id -g fsgqa`
> +user_bar=`id -u fsgqa2`
> +group_bar=`id -g fsgqa2`
> +
> +setup_tree()
> +{
> + mkdir -p $SCRATCH_MNT/source-mnt
> + chmod 0777 $SCRATCH_MNT/source-mnt
> + touch $SCRATCH_MNT/source-mnt/file1
> + chown 65534:65534 $SCRATCH_MNT
> + chown 65534:65534 $SCRATCH_MNT/source-mnt
> + chown 65534:65535 $SCRATCH_MNT/source-mnt/file1
> +
> + mkdir -p $SCRATCH_MNT/target-mnt
> + chmod 0777 $SCRATCH_MNT/target-mnt
> +}
> +
> +# Setup an idmapped mount where uid and gid 65534 are mapped to fsgqa and uid
> +# and gid 65535 are mapped to fsgqa2.
> +setup_idmapped_mnt()
> +{
> + $here/src/vfs/mount-idmapped \
> + --map-mount=u:65534:$user_foo:1 \
> + --map-mount=g:65534:$group_foo:1 \
> + --map-mount=u:65535:$user_bar:1 \
> + --map-mount=g:65535:$group_bar:1 \
> + $SCRATCH_MNT/source-mnt $SCRATCH_MNT/target-mnt
> +}
> +
> +# We've created a layout where fsgqa owns the target file but the group of the
> +# target file is owned by another group. We now test that user fsgqa can change
> +# the group ownership of the file to a group they control. In this case to the
> +# fsgqa group.
> +change_group_ownership()
> +{
> + local path="$1"
> +
> + stat -c '%U:%G' $path
> + _user_do "id -u --name; id -g --name; chgrp $group_foo $path"
> + stat -c '%U:%G' $path
> + _user_do "id -u --name; id -g --name; chgrp $group_bar $path > /dev/null 2>&1"
> + stat -c '%U:%G' $path
> +}
> +
> +reset_ownership()
> +{
> + local path="$SCRATCH_MNT/source-mnt/file1"
> +
> + echo ""
> + echo "reset ownership"
> + chown 65534:65534 $path
> + stat -c '%u:%g' $path
> + chown 65534:65535 $path
> + stat -c '%u:%g' $path
> +}
> +
> +run_base_test()
> +{
> + mkdir -p $SCRATCH_MNT/source-mnt
> + chmod 0777 $SCRATCH_MNT/source-mnt
> + touch $SCRATCH_MNT/source-mnt/file1
> + chown $user_foo:$group_foo $SCRATCH_MNT
> + chown $user_foo:$group_foo $SCRATCH_MNT/source-mnt
> + chown $user_foo:$group_bar $SCRATCH_MNT/source-mnt/file1
> +
> + echo ""
> + echo "base test"
> + change_group_ownership "$SCRATCH_MNT/source-mnt/file1"
> + rm -rf "$SCRATCH_MNT/source-mnt"
> +}
> +
> +# Basic test as explained in the comment for change_group_ownership().
> +run_idmapped_test()
> +{
> + echo ""
> + echo "base idmapped test"
> + change_group_ownership "$SCRATCH_MNT/target-mnt/file1"
> + reset_ownership
> +}
> +
> +lower="$SCRATCH_MNT/target-mnt"
> +upper="$SCRATCH_MNT/ovl-upper"
> +work="$SCRATCH_MNT/ovl-work"
> +merge="$SCRATCH_MNT/ovl-merge"
> +
> +# Prepare overlayfs with metacopy turned off.
> +setup_overlayfs_idmapped_lower_metacopy_off()
> +{
> + mkdir -p $upper $work $merge
> + _overlay_mount_dirs $lower $upper $work \
> + overlay $merge -ometacopy=off || \
> + _notrun "overlayfs doesn't support idmappped layers"
> +}
> +
> +# Prepare overlayfs with metacopy turned on.
> +setup_overlayfs_idmapped_lower_metacopy_on()
> +{
> + mkdir -p $upper $work $merge
> + _overlay_mount_dirs $lower $upper $work overlay $merge -ometacopy=on
> +}
> +
> +reset_overlayfs()
> +{
> + $UMOUNT_PROG $SCRATCH_MNT/ovl-merge 2>/dev/null
> + rm -rf $upper $work $merge
> +}
> +
> +# Overlayfs can be mounted on top of idmapped layers. Make sure that the basic
> +# test explained in the comment for change_group_ownership() passes with
> +# overlayfs mounted on top of it.
> +# This tests overlayfs with metacopy turned off, i.e., changing a file copies
> +# up data and metadata.
> +run_overlayfs_idmapped_lower_metacopy_off()
> +{
> + echo ""
> + echo "overlayfs idmapped lower metacopy off"
> + change_group_ownership "$SCRATCH_MNT/ovl-merge/file1"
> + reset_overlayfs
> + reset_ownership
> +}
> +
> +# Overlayfs can be mounted on top of idmapped layers. Make sure that the basic
> +# test explained in the comment for change_group_ownership() passes with
> +# overlayfs mounted on top of it.
> +# This tests overlayfs with metacopy turned on, i.e., changing a file tries to
> +# only copy up metadata.
> +run_overlayfs_idmapped_lower_metacopy_on()
> +{
> + echo ""
> + echo "overlayfs idmapped lower metacopy on"
> + change_group_ownership "$SCRATCH_MNT/ovl-merge/file1"
> + reset_overlayfs
> + reset_ownership
> +}
> +
> +run_base_test
> +
> +setup_tree
> +setup_idmapped_mnt
> +run_idmapped_test
> +
> +setup_overlayfs_idmapped_lower_metacopy_off
> +run_overlayfs_idmapped_lower_metacopy_off
> +
> +setup_overlayfs_idmapped_lower_metacopy_on
> +run_overlayfs_idmapped_lower_metacopy_on
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/692.out b/tests/generic/692.out
> new file mode 100644
> index 00000000..1cb01f8e
> --- /dev/null
> +++ b/tests/generic/692.out
> @@ -0,0 +1,49 @@
> +QA output created by 692
> +
> +base test
> +fsgqa:fsgqa2
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +
> +base idmapped test
> +fsgqa:fsgqa2
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +
> +reset ownership
> +65534:65534
> +65534:65535
> +
> +overlayfs idmapped lower metacopy off
> +fsgqa:fsgqa2
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +
> +reset ownership
> +65534:65534
> +65534:65535
> +
> +overlayfs idmapped lower metacopy on
> +fsgqa:fsgqa2
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +fsgqa
> +fsgqa
> +fsgqa:fsgqa
> +
> +reset ownership
> +65534:65534
> +65534:65535
>
> base-commit: 568ac9fffeb6afec03e5d6c9936617232fd7fc6d
> --
> 2.34.1
>
