Re: [PATCH v9 3/3] selftests/namespaces: test the CAP_SETFCAP rule for user namespaces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] selftests/namespaces: host /mnt creation and false skip in
  idmap_tmp()
- [Low] selftests/namespaces: add setfcap_test to .gitignore
--

commit 38eae5440ed3d06c4f6383c08c8a0e11e61d724f
Author: Josef Bacik <josef@toxicpanda.com>

selftests/namespaces: test the CAP_SETFCAP rule for user namespaces

This commit adds kselftests verifying that a uid 0 task without CAP_SETFCAP
cannot regain the ability to set root file capabilities via setns() or
ptrace into a user namespace, while preserving valid namespace mappings,
idmapped mounts, and file capability operations.

> diff --git a/tools/testing/selftests/namespaces/Makefile b/tools/testing/selftests/namespaces/Makefile
> index 9b22d12708e1a..0455b6544869d 100644
> --- a/tools/testing/selftests/namespaces/Makefile
> +++ b/tools/testing/selftests/namespaces/Makefile
> @@ -14,7 +14,8 @@ TEST_GEN_PROGS := nsid_test \
>  		  stress_test \
>  		  listns_pagination_bug \
>  		  regression_pidfd_setns_test \
> -		  uid_map_range_test
> +		  uid_map_range_test \
> +		  setfcap_test
>  
>  include ../lib.mk
>  

[Severity: Low]
Should setfcap_test also be added to
tools/testing/selftests/namespaces/.gitignore so in-tree builds do not leave
an untracked binary?

> diff --git a/tools/testing/selftests/namespaces/setfcap_test.c b/tools/testing/selftests/namespaces/setfcap_test.c
> new file mode 100644
> index 0000000000000..aee0b1d6f329a
> --- /dev/null
> +++ b/tools/testing/selftests/namespaces/setfcap_test.c

[ ... ]

> +/* /tmp idmapped with the user namespace of pid, at /mnt */
> +static int idmap_tmp(int pid)
> +{
> +	struct mount_attr attr = { .attr_set = MOUNT_ATTR_IDMAP };
> +	char path[64];
> +	int tfd, nsfd;
> +
> +	mkdir("/mnt", 0755);
> +	snprintf(path, sizeof(path), "/proc/%d/ns/user", pid);
> +	nsfd = open(path, O_RDONLY);
> +	tfd = syscall(__NR_open_tree, AT_FDCWD, "/tmp",
> +		      OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
> +	if (nsfd < 0 || tfd < 0)
> +		return -1;
> +	attr.userns_fd = nsfd;
> +	if (syscall(__NR_mount_setattr, tfd, "", AT_EMPTY_PATH, &attr,
> +		    sizeof(attr)))
> +		return -1;
> +	if (syscall(__NR_move_mount, tfd, "", AT_FDCWD, "/mnt",
> +		    MOVE_MOUNT_F_EMPTY_PATH))
> +		return -1;
> +	close(tfd);
> +	close(nsfd);
> +	return 0;
> +}
> +
> +/* Each test: private mounts, a tmpfs on /tmp with copies of this program */
> +static void prepare(struct __test_metadata *_metadata)
> +{
> +	ASSERT_EQ(unshare(CLONE_NEWNS), 0);
> +	ASSERT_EQ(mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL), 0);
> +	ASSERT_EQ(mount("tmpfs", "/tmp", "tmpfs", 0, "mode=1777"), 0);
> +	ASSERT_EQ(new_file("/tmp/x", 0), 0);
> +	ASSERT_EQ(new_file("/tmp/u", 1000), 0);
> +	mkdir("/mnt", 0755);
> +}

[ ... ]

> +/* returns 1 to skip: no idmapped mounts on tmpfs */
> +static int idmap_case(const struct idmap_case *c, long *r)
> +{
> +	struct holder ns, im = { -1, -1 };
> +	struct idmap_join j;
> +	int ret = -1;
> +
> +	ns = spawn_mapped(HOLDER_ROOT, c->ns_map);
> +	if (ns.pid <= 0)
> +		return -1;
> +	if (c->idmap_map) {
> +		im = spawn_mapped(HOLDER_ROOT, c->idmap_map);
> +		if (im.pid <= 0)
> +			goto out;
> +	}
> +	if (new_file("/tmp/x", c->file_uid))
> +		goto out;
> +	if (idmap_tmp(c->idmap_map ? im.pid : ns.pid)) {
> +		ret = 1;
> +		goto out;
> +	}

[Severity: Medium]
Should prepare() and idmap_tmp() place the mountpoint inside the private
/tmp tmpfs instead of at /mnt, and should idmap_tmp() distinguish
mount_setattr() failures from other setup errors?

prepare() only mounts a tmpfs on /tmp, so mkdir("/mnt", 0755) creates /mnt
on the host root filesystem, or fails on a read-only root filesystem where
/mnt does not exist.  In addition, idmap_case() treats any error from
idmap_tmp() as ret = 1, causing TEST_F(idmap, fcap) to report a skip ("no
idmapped mounts on tmpfs") when open(), open_tree(), or move_mount() fails.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-b4-setfcap-userns-v9-0-0d3f00976ce2@toxicpanda.com?part=3