[igt-dev] [PATCH i-g-t v2] ts/core_setmaster: new test for drop/set master semantics

* [igt-dev] [PATCH i-g-t v2] ts/core_setmaster: new test for drop/set master semantics
@ 2020-02-19 14:24 Emil Velikov
  2020-02-19 15:41 ` [igt-dev] ✗ GitLab.Pipeline: failure for ts/core_setmaster: new test for drop/set master semantics (rev2) Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Emil Velikov @ 2020-02-19 14:24 UTC (permalink / raw)
  To: igt-dev

From: Emil Velikov <emil.velikov@collabora.com>

This test adds three distinct subtests:
 - drop/set master as root
 - drop/set master as non-root
 - drop/set master for a shared fd

Currently the second subtest will fail, with kernel patch to address
that has been submitted.

v2: Add to the autotools build

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 tests/Makefile.sources |   1 +
 tests/core_setmaster.c | 182 +++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 3 files changed, 184 insertions(+)
 create mode 100644 tests/core_setmaster.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index b87d6333..5da36a91 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -18,6 +18,7 @@ TESTS_progs = \
 	core_getclient \
 	core_getstats \
 	core_getversion \
+	core_setmaster \
 	core_setmaster_vs_auth \
 	debugfs_test \
 	dmabuf \
diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c
new file mode 100644
index 00000000..2447c077
--- /dev/null
+++ b/tests/core_setmaster.c
@@ -0,0 +1,182 @@
+/*
+ * Copyright © 2020 Collabora, Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Emil Velikov <emil.l.velikov@gmail.com>
+ *
+ */
+
+/*
+ * Testcase: Check that drop/setMaster behaves correctly wrt root/user access
+ *
+ * Test checks if the ioctls succeed or fail, depending if the applications was
+ * run with root, user privileges or if we have separate privileged arbitrator.
+ */
+
+#include "igt.h"
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+
+IGT_TEST_DESCRIPTION("Check that drop/setMaster behaves correctly wrt root/user"
+		     " access");
+
+static bool is_master(int fd)
+{
+	/* FIXME: replace with drmIsMaster once we bumped libdrm version */
+	return drmAuthMagic(fd, 0) != -EACCES;
+}
+
+static void check_drop_set(void)
+{
+	int master;
+
+	master = __drm_open_driver(DRIVER_ANY);
+
+	/* Double-check if open has failed */
+	igt_assert_neq(master, -1);
+
+	/* At this point we're master capable due to:
+	 * - being root - always
+	 * - normal user - as the only drm only drm client (on this VT)
+	 */
+	igt_assert_eq(is_master(master), true);
+
+	/* If we have SYS_CAP_ADMIN we're in the textbook best-case scenario.
+	 *
+	 * Otherwise newer kernels allow the application to drop/revoke its
+	 * master capability and request it again later.
+	 *
+	 * In this case, we address two types of issues:
+	 * - the application no longer need suid-root (or equivalent) which
+	 * was otherwise required _solely_ for these two ioctls
+	 * - plenty of applications ignore (or discard) the result of the
+	 * calls all together.
+	 */
+	igt_assert_eq(drmDropMaster(master), 0);
+	igt_assert_eq(drmSetMaster(master), 0);
+
+	close(master);
+}
+
+static unsigned tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
+{
+	char path[256];
+	struct stat st;
+	unsigned i;
+
+	for (i = 0; i < max_perm; i++) {
+		snprintf(path, sizeof(path), "/dev/dri/card%u", i);
+
+		/* Existing userspace assumes there's no gaps, do the same. */
+		if (stat(path, &st) != 0)
+			break;
+
+		if (save) {
+			/* Save and toggle */
+			saved_perm[i] = st.st_mode & (S_IROTH | S_IWOTH);
+			st.st_mode |= S_IROTH | S_IWOTH;
+		} else {
+			/* Clear and restore */
+			st.st_mode &= ~(S_IROTH | S_IWOTH);
+			st.st_mode |= saved_perm[i];
+		}
+
+		/* In the extremely unlikely case of this failing, there't not
+		 * much we can do.
+		 */
+		chmod(path, st.st_mode);
+	}
+	return i;
+}
+
+
+igt_main
+{
+	igt_subtest("master-drop-set-root") {
+		check_drop_set();
+	}
+
+
+	igt_subtest("master-drop-set-user") {
+		uint8_t saved_perm[255];
+		unsigned num;
+
+		/* Upon dropping root we end up as random user, which
+		 * a) is not in the video group, and
+		 * b) lacks logind ACL, thus
+		 * any open() fill fail.
+		 *
+		 * As such, save the state of original other rw permissions
+		 * and toggle them on.
+		 */
+		num = tweak_perm(saved_perm, ARRAY_SIZE(saved_perm), true);
+
+		igt_fork(child, 1) {
+			igt_drop_root();
+			check_drop_set();
+		}
+		igt_waitchildren();
+
+		/* Restore the orignal permissions */
+		tweak_perm(saved_perm, num, false);
+	}
+
+	igt_subtest("master-drop-set-shared-fd") {
+		int master;
+
+		master = __drm_open_driver(DRIVER_ANY);
+
+		/* Double-check if open has failed */
+		igt_assert_neq(master, -1);
+
+		igt_assert_eq(is_master(master), true);
+		igt_fork(child, 1) {
+			igt_drop_root();
+
+			/* Dropping root privileges should not alter the
+			 * master capability of the fd */
+			igt_assert_eq(is_master(master), true);
+
+			/* Even though we've got the master capable fd, we're
+			 * a different process (kernel struct pid *) than the
+			 * one which opened the device node.
+			 *
+			 * This ensures that existing workcases of separate
+			 * (privileged) arbitrator still work. For example:
+			 * - logind + X/Wayland compositor
+			 * - weston-launch + weston
+			 */
+			igt_assert_eq(drmDropMaster(master), -1);
+			igt_assert_eq(errno, EACCES);
+			igt_assert_eq(drmSetMaster(master), -1);
+			igt_assert_eq(errno, EACCES);
+
+			close(master);
+		}
+		igt_waitchildren();
+
+		close(master);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index fa0103e3..d8fb9f3e 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -3,6 +3,7 @@ test_progs = [
 	'core_getclient',
 	'core_getstats',
 	'core_getversion',
+	'core_setmaster',
 	'core_setmaster_vs_auth',
 	'debugfs_test',
 	'dmabuf',
-- 
2.25.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 12+ messages in thread