Re: [RFC PATCH v2 36/41] selftests: add tests for UUID and GUID ids

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests: uid_r: acl_v3() writes POSIX ACL xattr fields in host byte order
- [Medium] selftests/uid_r: fail instead of skip on ENOSYS in signalfd and ipc tests
- [Low] selftests: uid_r: FIXTURE_TEARDOWN() leaves mkdtemp() directories in /tmp
--

commit b236f646b555e600321fde8fdcedd3ac92eccb4e
Author: Jan Koester <jan.koester@tuxist.de>

selftests: add tests for UUID and GUID ids

This commit adds a kselftest suite for the *_r system calls and extended
kernel interfaces supporting UUID and GUID user and group identifiers. It
covers ID validation, credential switching, supplementary groups, filesystem
ownership on tmpfs, socket credentials, signalfd, SysV IPC, POSIX ACL v3
xattrs, /proc/self/status, and user namespace restrictions.

> diff --git a/tools/testing/selftests/uid_r/uid_r_test.c b/tools/testing/selftests/uid_r/uid_r_test.c
> new file mode 100644
> index 0000000000000..88397bf3dae7e
> --- /dev/null
> +++ b/tools/testing/selftests/uid_r/uid_r_test.c
> @@ -0,0 +1,535 @@
[ ... ]
> +/* a private tmpfs at a fresh directory, for file tests */
> +static void private_tmpfs(struct __test_metadata *_metadata, char *dir)
> +{
> +	strcpy(dir, "/tmp/uid_r.XXXXXX");
> +	ASSERT_EQ(0, unshare(CLONE_NEWNS));
> +	ASSERT_EQ(0, mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL));
> +	ASSERT_NE(NULL, mkdtemp(dir));
> +	ASSERT_EQ(0, mount("tmpfs", dir, "tmpfs", 0, "mode=1777"));
> +}
[ ... ]
> +FIXTURE(uid_r) {
> +	char dir[64];
> +};
> +
> +FIXTURE_SETUP(uid_r)
> +{
> +	id_r id;
> +
> +	if (geteuid() != 0)
> +		SKIP(return, "needs root");
> +	if (sys(__NR_getuid_r, (long)&id, 0, 0, 0, 0) == -ENOSYS)
> +		SKIP(return, "no *_r system calls");
> +	self->dir[0] = 0;
> +}
> +
> +FIXTURE_TEARDOWN(uid_r)
> +{
> +}

[Severity: Low]
Does FIXTURE_TEARDOWN(uid_r) leak the temporary /tmp/uid_r.XXXXXX mountpoint
directories created by private_tmpfs()?

In tools/testing/selftests/uid_r/uid_r_test.c, private_tmpfs() calls
unshare(CLONE_NEWNS) and makes mount propagation private, then calls
mkdtemp(dir) on the underlying /tmp filesystem before mounting tmpfs onto
dir.

When chown_r_statx, create_as_uuid_fsuid, posix_acl_v3, and user_namespace
call private_tmpfs(_metadata, self->dir), the private tmpfs mount is cleaned
up when the test child process exits, but because FIXTURE_TEARDOWN(uid_r) is
empty, the underlying self->dir directory in /tmp is never unmounted and
removed with rmdir(self->dir) when self->dir[0] is non-empty.

[ ... ]
> +TEST_F(uid_r, signalfd_uid_r)
> +{
> +	id_r u = uuid(11), s0 = legacy(0), got;
> +	struct signalfd_siginfo ssi;
> +	sigset_t mask;
> +	int sfd;
> +
> +	sigemptyset(&mask);
> +	sigaddset(&mask, SIGUSR1);
> +	ASSERT_EQ(0, sigprocmask(SIG_BLOCK, &mask, NULL));
> +	sfd = signalfd(-1, &mask, 0);
> +	ASSERT_LE(0, sfd);
[ ... ]
> +TEST_F(uid_r, ipc_stat_r)
> +{
> +	id_r u = uuid(12), g = guid(13);
> +	struct ipc_perm_r_t pr;
> +	struct shmid_ds ds;
> +	int id;
> +
> +	memset(&pr, 0, sizeof(pr));
> +	pr.mode = 0640;
> +	pr.uid = u;
> +	pr.gid = g;
> +
> +	id = shmget(IPC_PRIVATE, 4096, IPC_CREAT | 0600);
> +	ASSERT_LE(0, id);

[Severity: Medium]
Should signalfd_uid_r and ipc_stat_r skip instead of failing when the kernel
is built without CONFIG_SIGNALFD or CONFIG_SYSVIPC?

In tools/testing/selftests/uid_r/uid_r_test.c, if CONFIG_SIGNALFD=n or
CONFIG_SYSVIPC=n, signalfd() in signalfd_uid_r and shmget() in ipc_stat_r
return -1 with errno set to ENOSYS, and ASSERT_LE() fails the tests outright
rather than calling SKIP().

Also, tools/testing/selftests/uid_r/config lists:

  CONFIG_TMPFS=y
  CONFIG_TMPFS_POSIX_ACL=y
  CONFIG_SYSVIPC=y
  CONFIG_SIGNALFD=y

Should tools/testing/selftests/uid_r/config also include CONFIG_USER_NS=y so
that the user_namespace test is not skipped on kernels built using the
selftest config fragment?

[ ... ]
> +/* user::rw- user:<u>:r-- group::--- mask::r-- other::--- in version 3 */
> +static size_t acl_v3(unsigned char *buf, const id_r *u)
> +{
> +	static const unsigned short ent[5][2] = {
> +		{ 0x01, 6 }, { 0x02, 4 }, { 0x04, 0 }, { 0x10, 4 }, { 0x20, 0 } };
> +	unsigned char *e = buf + 4;
> +	unsigned int ver = 3;
> +
> +	memcpy(buf, &ver, 4);
> +	for (int i = 0; i < 5; i++, e += 24) {
> +		unsigned int type = 0;
> +
> +		memset(e, 0, 24);
> +		memcpy(e, &ent[i][0], 2);
> +		memcpy(e + 2, &ent[i][1], 2);
> +		if (ent[i][0] == 0x02) {
> +			type = u->type;
> +			memcpy(e + 8, u->uuid, 16);
> +		}
> +		memcpy(e + 4, &type, 4);
> +	}
> +	return 4 + 5 * 24;
> +}
> +
> +TEST_F(uid_r, posix_acl_v3)
> +{
> +	unsigned char buf[256], got[256];
> +	id_r u = uuid(20), s0 = legacy(0);
> +	char path[96];
> +	size_t len;
> +	int fd;
> +
> +	private_tmpfs(_metadata, self->dir);
> +	snprintf(path, sizeof(path), "%s/acl", self->dir);
> +	fd = open(path, O_CREAT | O_WRONLY, 0600);
> +	ASSERT_LE(0, fd);
> +	close(fd);
> +	len = acl_v3(buf, &u);
> +	if (setxattr(path, "system.posix_acl_access", buf, len, 0) &&
> +	    errno == EOPNOTSUPP)
> +		SKIP(return, "tmpfs without POSIX ACLs");
> +	ASSERT_EQ(len, getxattr(path, "system.posix_acl_access", got, sizeof(got)));
> +	EXPECT_EQ(3, got[0]);

[Severity: Medium]
Won't acl_v3() and posix_acl_v3 fail or falsely skip on big-endian
architectures because the POSIX ACL xattr fields are serialized in host byte
order instead of little-endian?

In include/uapi/linux/posix_acl_xattr.h, struct posix_acl_xattr_header
defines a_version as __le32, and struct posix_acl_xattr_entry_r defines
e_tag and e_perm as __le16 and e_type as __le32. In fs/posix_acl.c,
posix_acl_fix_xattr_common() checks header->a_version against
cpu_to_le32(POSIX_ACL_XATTR_VERSION_R) and returns -EOPNOTSUPP on a
mismatch, while posix_acl_from_xattr_r() converts e_tag, e_perm, and e_type
using le16_to_cpu() and le32_to_cpu().

In tools/testing/selftests/uid_r/uid_r_test.c, acl_v3() copies host-endian
unsigned int ver, unsigned short ent[i][0] and ent[i][1], and unsigned int
type directly into buf with memcpy(). On big-endian systems, setxattr() in
posix_acl_v3 sees a byte-swapped a_version and fails with EOPNOTSUPP, which
posix_acl_v3 then treats as "tmpfs without POSIX ACLs" and skips even when
CONFIG_TMPFS_POSIX_ACL=y is enabled.

In addition, if setxattr() fails with an errno other than EOPNOTSUPP,
posix_acl_v3 does not assert that setxattr() succeeded before calling
getxattr().

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=36