Re: [PATCH v7 6/6] selftests/namespaces: test the CAP_SETFCAP rule for user namespaces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/namespaces: unclosed pipe ends and missing kill in setfcap_test
--

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

selftests/namespaces: test the CAP_SETFCAP rule for user namespaces

This commit adds kselftest coverage in tools/testing/selftests/namespaces/
for the CAP_SETFCAP rule governing user namespace UID mappings, file
capabilities, setns(), idmapped mounts, and ptrace attach checks.

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

[ ... ]

> +static long x_nonzero_root(void *a)
> +{
> +	int s1[2], s2[2], res[2], pid;
> +	long r = SETUP_ERROR;
> +	char c;
> +
> +	if (pipe(s1) || pipe(s2) || pipe(res))
> +		return SETUP_ERROR;
> +	pid = fork();
> +	if (!pid) {
> +		restrict_root();
> +		if (unshare(CLONE_NEWUSER) || write(s1[1], "u", 1) != 1 ||
> +		    read(s2[0], &c, 1) != 1)
> +			_exit(1);
> +		if (unshare(CLONE_NEWUSER))
> +			r = SETUP_ERROR;
> +		else
> +			r = write_map(0, "0 1000 1\n");
> +		if (write(res[1], &r, sizeof(r)) != sizeof(r))
> +			_exit(1);
> +		_exit(0);
> +	}
> +	if (read(s1[0], &c, 1) != 1 || map_both(pid, "1000 0 1\n") ||
> +	    write(s2[1], "g", 1) != 1 || read(res[0], &r, sizeof(r)) != sizeof(r))
> +		r = SETUP_ERROR;
> +	waitpid(pid, NULL, 0);
> +	return r;
> +}
> +
> +TEST_F(setfcap, map_ancestor_root_at_nonzero_uid)
> +{
> +	EXPECT_EQ(in_child(x_nonzero_root, NULL), -EPERM);
> +}
> +
> +/*
> + * R maps host uid 0 as uid 5 of its namespace and forks X, which gives up
> + * CAP_SETFCAP there and unshares again.  "5 5 1" then maps host uid 0 one
> + * level down, as uid 5.  X writes it itself, or R does from the parent and X
> + * writes a v3 xattr with rootid 5.
> + */
> +static long r_nests_x(void *a)
> +{
> +	int x_maps = *(int *)a;
> +	int s1[2], s2[2], res[2], pid;
> +	long r = SETUP_ERROR;
> +	char c;
> +
> +	if (unshare(CLONE_NEWUSER) || map_both(0, "5 0 1\n"))
> +		return SETUP_ERROR;
> +	if (pipe(s1) || pipe(s2) || pipe(res))
> +		return SETUP_ERROR;
> +	pid = fork();
> +	if (!pid) {
> +		restrict_root();
> +		if (unshare(CLONE_NEWUSER))
> +			r = SETUP_ERROR;
> +		else if (x_maps)
> +			r = write_map(0, "5 5 1\n");
> +		else if (write(s1[1], "u", 1) != 1 || read(s2[0], &c, 1) != 1)
> +			r = SETUP_ERROR;
> +		else
> +			r = set_fcap("/tmp/x", 5);
> +		if (write(res[1], &r, sizeof(r)) != sizeof(r))
> +			_exit(1);
> +		_exit(0);
> +	}
> +	if (!x_maps && (read(s1[0], &c, 1) != 1 || map_both(pid, "5 5 1\n") ||
> +			write(s2[1], "g", 1) != 1))
> +		r = SETUP_ERROR;
> +	else if (read(res[0], &r, sizeof(r)) != sizeof(r))
> +		r = SETUP_ERROR;
> +	kill(pid, SIGKILL);
> +	waitpid(pid, NULL, 0);
> +	return r;
> +}

[ ... ]

> +static long dropped_writes_inherited_fd(void *a)
> +{
> +	int fd, pid, st;
> +	long r = SETUP_ERROR;
> +	int p[2];
> +
> +	if (unshare(CLONE_NEWUSER))
> +		return SETUP_ERROR;
> +	fd = open("/proc/self/uid_map", O_WRONLY);
> +	if (fd < 0 || pipe(p))
> +		return SETUP_ERROR;
> +	pid = fork();
> +	if (!pid) {
> +		drop_setfcap_keep_bset();
> +		r = wr(fd, "0 0 1\n");
> +		if (write(p[1], &r, sizeof(r)) != sizeof(r))
> +			_exit(1);
> +		_exit(0);
> +	}
> +	if (read(p[0], &r, sizeof(r)) != sizeof(r))
> +		r = SETUP_ERROR;
> +	waitpid(pid, &st, 0);
> +	return r;
> +}

[ ... ]

> +static long x_unshares_r_maps(void *a)
> +{
> +	const struct parent_maps *m = a;
> +	int s1[2], s2[2], res[2], pid;
> +	long r = SETUP_ERROR;
> +	char c;
> +
> +	if (pipe(s1) || pipe(s2) || pipe(res))
> +		return SETUP_ERROR;
> +	pid = fork();
> +	if (!pid) {
> +		restrict_root();
> +		if (unshare(CLONE_NEWUSER) || write(s1[1], "u", 1) != 1 ||
> +		    read(s2[0], &c, 1) != 1)
> +			_exit(1);
> +		if (m->own_tmpfs) {
> +			if (unshare(CLONE_NEWNS) ||
> +			    mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) ||
> +			    mount("tmpfs", "/mnt", "tmpfs", 0, NULL) ||
> +			    new_file("/mnt/f", 0))
> +				r = SETUP_ERROR;
> +			else
> +				r = set_fcap("/mnt/f", m->rootid);
> +		} else {
> +			r = set_fcap("/tmp/x", m->rootid);
> +		}
> +		if (write(res[1], &r, sizeof(r)) != sizeof(r))
> +			_exit(1);
> +		_exit(0);
> +	}
> +	if (read(s1[0], &c, 1) != 1 || map_both(pid, m->map) ||
> +	    write(s2[1], "g", 1) != 1 || read(res[0], &r, sizeof(r)) != sizeof(r))
> +		r = SETUP_ERROR;
> +	kill(pid, SIGKILL);
> +	waitpid(pid, NULL, 0);
> +	return r;
> +}

[ ... ]

> +TEST_F(setfcap, fcap_v2_without_uid0_from_parent)
> +{
> +	int ready[2], go[2], mapped[2], pid;
> +	struct holder h;
> +	char c;
> +
> +	ASSERT_EQ(pipe(ready), 0);
> +	ASSERT_EQ(pipe(go), 0);
> +	ASSERT_EQ(pipe(mapped), 0);
> +	pid = fork();
> +	ASSERT_GE(pid, 0);
> +	if (!pid) {
> +		/* host uid 0 is uid 1000 in here, nobody is uid 0 */
> +		if (unshare(CLONE_NEWUSER) || write(ready[1], "u", 1) != 1 ||
> +		    read(mapped[0], &c, 1) != 1 || unshare(CLONE_NEWNS) ||
> +		    mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) ||
> +		    mount("tmpfs", "/mnt", "tmpfs", 0, NULL) ||
> +		    new_file("/mnt/f", 1000) || write(ready[1], "m", 1) != 1)
> +			_exit(1);
> +		if (read(go[0], &c, 1) < 0)
> +			_exit(1);
> +		_exit(0);
> +	}
> +	h.pid = pid;
> +	h.go = go[1];
> +	ASSERT_EQ(read(ready[0], &c, 1), 1);
> +	ASSERT_EQ(map_both(pid, "1000 0 1\n"), 0);
> +	ASSERT_EQ(write(mapped[1], "g", 1), 1);
> +	ASSERT_EQ(read(ready[0], &c, 1), 1);
> +	EXPECT_EQ(in_child(r_v2_without_uid0_from_parent, &h), -EINVAL);
> +	reap_holder(h);
> +}

[ ... ]

> +static int idmap_case(const struct idmap_case *c, long *r)
> +{
> +	struct holder ns, im;
> +	int pid, p[2], st;
> +
> +	ns = spawn_mapped(c->ns_kind, c->ns_map);
> +	if (ns.pid <= 0)
> +		return -1;
> +	im = c->idmap_map ? spawn_mapped(HOLDER_ROOT, c->idmap_map) : ns;
> +	if (im.pid <= 0 || new_file("/tmp/x", c->file_uid))
> +		return -1;
> +	if (idmap_tmp(im.pid)) {
> +		reap_holder(ns);
> +		if (c->idmap_map)
> +			reap_holder(im);
> +		return 1;
> +	}
> +	if (pipe(p))
> +		return -1;
> +	/* join from a child of the namespace's holder's sibling: pass the pid */
> +	pid = fork();
> +	if (!pid) {
> +		long res;
> +
> +		if (c->restricted)
> +			restrict_root();
> +		res = join(ns.pid) ? SETUP_ERROR : set_fcap("/mnt/x", 0);
> +		if (write(p[1], &res, sizeof(res)) != sizeof(res))
> +			_exit(1);
> +		_exit(0);
> +	}
> +	if (read(p[0], r, sizeof(*r)) != sizeof(*r))
> +		*r = SETUP_ERROR;
> +	waitpid(pid, &st, 0);
> +	umount2("/mnt", MNT_DETACH);
> +	reap_holder(ns);
> +	if (c->idmap_map)
> +		reap_holder(im);
> +	return 0;
> +}

[Severity: Medium]
Can these test helpers hang in read() or waitpid() and leak pipe file
descriptors when a setup step fails?

Unlike in_child() and spawn_holder(), the helpers x_nonzero_root(),
r_nests_x(), dropped_writes_inherited_fd(), x_unshares_r_maps(),
fcap_v2_without_uid0_from_parent, and idmap_case() do not close the unused
pipe ends in the parent and child after fork(), nor do they close the
remaining pipe descriptors before returning:

1. Because the parent keeps the write ends (s1[1], res[1], p[1], ready[1])
open in its own file descriptor table, if the child exits early on a setup
failure (such as in restrict_root(), drop_setfcap_keep_bset(),
unshare(CLONE_NEWUSER), mount(), or new_file()), the parent's blocking
read() on s1[0], res[0], p[0], or ready[0] never sees EOF and blocks until
the test runner times out instead of returning SETUP_ERROR.

2. In r_nests_x() when x_maps is 0, if unshare(CLONE_NEWUSER) fails in the
child, the child sets r = SETUP_ERROR, skips writing to s1[1], writes r to
res[1], and exits:

	if (!pid) {
		restrict_root();
		if (unshare(CLONE_NEWUSER))
			r = SETUP_ERROR;
		else if (x_maps)
			r = write_map(0, "5 5 1\n");
		else if (write(s1[1], "u", 1) != 1 || read(s2[0], &c, 1) != 1)
			r = SETUP_ERROR;

Meanwhile the parent is blocked on read(s1[0], &c, 1) with s1[1] still open
in the parent process.

3. In x_nonzero_root(), if map_both(pid, "1000 0 1\n") fails in the parent,
short-circuit evaluation skips write(s2[1], "g", 1) and calls
waitpid(pid, NULL, 0) without killing the child or closing s2[1] (unlike
r_nests_x() and x_unshares_r_maps() which call kill(pid, SIGKILL) before
waitpid()), leaving waitpid() blocked while the child waits on
read(s2[0], &c, 1).

4. In fcap_v2_without_uid0_from_parent, the child keeps mapped[1] and go[1]
open, so if a parent ASSERT_EQ() fails and exits before reap_holder(), the
child stays blocked on read(mapped[0], &c, 1) or read(go[0], &c, 1).

Should these helpers close the unused pipe ends after fork(), close the
remaining pipe descriptors before returning, and ensure x_nonzero_root()
kills the child before waitpid() on error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-b4-setfcap-userns-v7-0-bf8a576f7259@toxicpanda.com?part=6