[igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

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

As the inline comment says, this test checks that the kernel allows
unauthenticated master with render capable, RENDER_ALLOW ioctls.

The kernel commit has extra details why.

v2:

- drop RUN_AS_ROOT guard
- call check_auth() on the {,un}authenticated device
- check the device is PRIME (import) capable
- check the device has render node
- tweak expectations based on above three
- elaborate why we care only about -EACCES

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

diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
new file mode 100644
index 00000000..82dd2ce9
--- /dev/null
+++ b/tests/core_unauth_vs_render.c
@@ -0,0 +1,182 @@
+/*
+ * Copyright 2018 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.velikov@collabora.com>
+ */
+
+/*
+ * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
+ * DRM_RENDER_ALLOW ioctls.
+ */
+
+#include "igt.h"
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <signal.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include <sys/poll.h>
+#include <sys/resource.h>
+#include <sys/sysmacros.h>
+#include "drm.h"
+
+#ifdef __linux__
+# include <sys/syscall.h>
+#else
+# include <pthread.h>
+#endif
+
+/* Checks whether the thread id is the current thread */
+static bool
+is_local_tid(pid_t tid)
+{
+#ifndef __linux__
+	return pthread_self() == tid;
+#else
+	/* On Linux systems, drmGetClient() would return the thread ID instead
+	   of the actual process ID */
+	return syscall(SYS_gettid) == tid;
+#endif
+}
+
+
+static bool check_auth(int fd)
+{
+	pid_t client_pid;
+	int i, auth, pid, uid;
+	unsigned long magic, iocs;
+	bool is_authenticated = false;
+
+	client_pid = getpid();
+	for (i = 0; !is_authenticated; i++) {
+		if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
+			break;
+		is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
+	}
+	return is_authenticated;
+}
+
+
+static bool has_prime_import(int fd)
+{
+	uint64_t value;
+
+	if (drmGetCap(fd, DRM_CAP_PRIME, &value))
+		return false;
+
+	return value & DRM_PRIME_CAP_IMPORT;
+}
+
+static bool has_render_node(int fd)
+{
+	char node_name[80];
+	struct stat sbuf;
+
+	if (fstat(fd, &sbuf))
+		return false;
+
+	sprintf(node_name, "/dev/dri/renderD%d", minor(sbuf.st_rdev) | 0x80);
+	if (stat(node_name, &sbuf))
+		return false;
+
+	return true;
+}
+
+IGT_TEST_DESCRIPTION("Call drmPrimeFDToHandle() from unauthenticated master doesn't return -EACCES.");
+
+static void test_unauth_vs_render(int master)
+{
+	int slave;
+	int prime_fd = -1;
+	uint32_t handle;
+
+	/*
+	 * The second open() happens without CAP_SYS_ADMIN, thus it will NOT
+	 * be authenticated.
+	 */
+	igt_info("Openning card node from a non-priv. user.\n");
+	igt_info("On failure, double-check the node permissions\n");
+	/* FIXME: relate to the master given and fix all of IGT */
+	slave = drm_open_driver(DRIVER_ANY);
+
+	igt_require(slave >= 0);
+	igt_assert(check_auth(slave) == false);
+
+	/* Issuing the following ioctl will fail, no doubt about it. */
+	igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
+
+	/*
+	 * Updated kernels allow render capable, unauthenticated master to
+	 * issue DRM_AUTH ioctls (like the above), as long as they are
+	 * annotated as DRM_RENDER_ALLOW - just like FD2HANDLE above.
+	 *
+	 * Otherwise, errno is set to -EACCES
+	 *
+	 * Note: We are _not_ interested in the FD2HANDLE specific errno. Those
+	 * should be checked other standalone tests.
+	 */
+	bool imp = has_prime_import(slave);
+	bool rend = has_render_node(slave);
+	igt_info("import %d rend %d\n", imp, rend);
+	if (has_prime_import(slave) && has_render_node(slave))
+		igt_assert(errno != EACCES);
+
+	else
+		igt_assert(errno == EACCES);
+
+	close(slave);
+}
+
+/*
+ * IGT is executed as root, although that may(?) change in the future.
+ * Thus we need to drop the privileges so that the second open() results in a
+ * client which is not unauthenticated. Running as normal user circumvents that.
+ *
+ * In both cases, we need to ensure the file permissions of the node are
+ * sufficient.
+ */
+
+igt_main
+{
+	int master;
+
+	igt_fixture
+		master = drm_open_driver(DRIVER_ANY);
+
+	igt_assert(check_auth(master) == true);
+
+	igt_subtest("unauth-vs-render") {
+		igt_fork(child, 1) {
+			igt_drop_root();
+			test_unauth_vs_render(master);
+		}
+		igt_waitchildren();
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index 0f12df26..e5200b36 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -1,5 +1,6 @@
 test_progs = [
 	'core_auth',
+	'core_unauth_vs_render',
 	'core_getclient',
 	'core_getstats',
 	'core_getversion',
-- 
2.20.1

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

[igt-dev] ✗ Fi.CI.BAT: failure for tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling (rev2)

[Patchwork]

== Series Details ==

Series: tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling (rev2)
URL   : https://patchwork.freedesktop.org/series/55149/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code


OK:       266
FAIL:       1
SKIP:       0
TIMEOUT:    0


The output from the failed tests:

 19/267 testcase check: core_unauth_vs_render   FAIL     0.35 s

--- command ---
/home/cidrm/igt-gpu-tools/tests/igt_command_line.sh core_unauth_vs_render
--- stdout ---
tests/core_unauth_vs_render:
  Checking invalid option handling...
  Checking valid option handling...
  Checking subtest enumeration...
FAIL: tests/core_unauth_vs_render
--- stderr ---
(core_unauth_vs_render:31889) CRITICAL: Test assertion failure function __real_main166, file ../tests/core_unauth_vs_render.c:173:
(core_unauth_vs_render:31889) CRITICAL: Failed assertion: check_auth(master) == true
(core_unauth_vs_render:31889) CRITICAL: Last errno: 9, Bad file descriptor
Test core_unauth_vs_render failed.
**** DEBUG ****
(core_unauth_vs_render:31889) CRITICAL: Test assertion failure function __real_main166, file ../tests/core_unauth_vs_render.c:173:
(core_unauth_vs_render:31889) CRITICAL: Failed assertion: check_auth(master) == true
(core_unauth_vs_render:31889) CRITICAL: Last errno: 9, Bad file descriptor
****  END  ****
core_unauth_vs_render: ../lib/igt_core.c:1143: igt_fail: Assertion `igt_can_fail()' failed.
Received signal SIGABRT.
Stack trace: 
 #0 [fatal_sig_handler+0xd5]
 #1 [killpg+0x40]
 #2 [gsignal+0xc7]
 #3 [abort+0x141]
 #4 [uselocale+0x33a]
 #5 [__assert_fail+0x42]
 #6 [igt_fail+0xe4]
 #7 [__igt_fail_assert+0x198]
 #8 [__real_main166+0x91]
 #9 [main+0x44]
 #10 [__libc_start_main+0xe7]
 #11 [_start+0x2a]
-------

Full log written to /home/cidrm/igt-gpu-tools/build/meson-logs/testlog.txt
FAILED: meson-test 
/usr/bin/python3 -u /usr/bin/meson test --no-rebuild --print-errorlogs
ninja: build stopped: subcommand failed.

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Petri Latvala]

On Wed, Feb 06, 2019 at 01:18:28PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> As the inline comment says, this test checks that the kernel allows
> unauthenticated master with render capable, RENDER_ALLOW ioctls.
> 
> The kernel commit has extra details why.
> 
> v2:
> 
> - drop RUN_AS_ROOT guard
> - call check_auth() on the {,un}authenticated device
> - check the device is PRIME (import) capable
> - check the device has render node
> - tweak expectations based on above three
> - elaborate why we care only about -EACCES
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  tests/core_unauth_vs_render.c | 182 ++++++++++++++++++++++++++++++++++
>  tests/meson.build             |   1 +
>  2 files changed, 183 insertions(+)
>  create mode 100644 tests/core_unauth_vs_render.c
> 
> diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> new file mode 100644
> index 00000000..82dd2ce9
> --- /dev/null
> +++ b/tests/core_unauth_vs_render.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright 2018 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.velikov@collabora.com>
> + */
> +
> +/*
> + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> + * DRM_RENDER_ALLOW ioctls.
> + */
> +
> +#include "igt.h"
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include <sys/poll.h>
> +#include <sys/resource.h>
> +#include <sys/sysmacros.h>
> +#include "drm.h"
> +
> +#ifdef __linux__
> +# include <sys/syscall.h>
> +#else
> +# include <pthread.h>
> +#endif
> +
> +/* Checks whether the thread id is the current thread */
> +static bool
> +is_local_tid(pid_t tid)
> +{
> +#ifndef __linux__
> +	return pthread_self() == tid;
> +#else
> +	/* On Linux systems, drmGetClient() would return the thread ID instead
> +	   of the actual process ID */
> +	return syscall(SYS_gettid) == tid;
> +#endif
> +}
> +
> +
> +static bool check_auth(int fd)
> +{
> +	pid_t client_pid;
> +	int i, auth, pid, uid;
> +	unsigned long magic, iocs;
> +	bool is_authenticated = false;
> +
> +	client_pid = getpid();
> +	for (i = 0; !is_authenticated; i++) {
> +		if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
> +			break;
> +		is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
> +	}
> +	return is_authenticated;
> +}
> +
> +
> +static bool has_prime_import(int fd)
> +{
> +	uint64_t value;
> +
> +	if (drmGetCap(fd, DRM_CAP_PRIME, &value))
> +		return false;
> +
> +	return value & DRM_PRIME_CAP_IMPORT;
> +}
> +
> +static bool has_render_node(int fd)
> +{
> +	char node_name[80];
> +	struct stat sbuf;
> +
> +	if (fstat(fd, &sbuf))
> +		return false;
> +
> +	sprintf(node_name, "/dev/dri/renderD%d", minor(sbuf.st_rdev) | 0x80);
> +	if (stat(node_name, &sbuf))
> +		return false;
> +
> +	return true;
> +}
> +
> +IGT_TEST_DESCRIPTION("Call drmPrimeFDToHandle() from unauthenticated master doesn't return -EACCES.");
> +
> +static void test_unauth_vs_render(int master)
> +{
> +	int slave;
> +	int prime_fd = -1;
> +	uint32_t handle;
> +
> +	/*
> +	 * The second open() happens without CAP_SYS_ADMIN, thus it will NOT
> +	 * be authenticated.
> +	 */
> +	igt_info("Openning card node from a non-priv. user.\n");
> +	igt_info("On failure, double-check the node permissions\n");
> +	/* FIXME: relate to the master given and fix all of IGT */
> +	slave = drm_open_driver(DRIVER_ANY);
> +
> +	igt_require(slave >= 0);
> +	igt_assert(check_auth(slave) == false);
> +
> +	/* Issuing the following ioctl will fail, no doubt about it. */
> +	igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> +
> +	/*
> +	 * Updated kernels allow render capable, unauthenticated master to
> +	 * issue DRM_AUTH ioctls (like the above), as long as they are
> +	 * annotated as DRM_RENDER_ALLOW - just like FD2HANDLE above.
> +	 *
> +	 * Otherwise, errno is set to -EACCES
> +	 *
> +	 * Note: We are _not_ interested in the FD2HANDLE specific errno. Those
> +	 * should be checked other standalone tests.
> +	 */
> +	bool imp = has_prime_import(slave);
> +	bool rend = has_render_node(slave);
> +	igt_info("import %d rend %d\n", imp, rend);
> +	if (has_prime_import(slave) && has_render_node(slave))
> +		igt_assert(errno != EACCES);
> +
> +	else
> +		igt_assert(errno == EACCES);
> +
> +	close(slave);
> +}
> +
> +/*
> + * IGT is executed as root, although that may(?) change in the future.
> + * Thus we need to drop the privileges so that the second open() results in a
> + * client which is not unauthenticated. Running as normal user circumvents that.
> + *
> + * In both cases, we need to ensure the file permissions of the node are
> + * sufficient.
> + */
> +
> +igt_main
> +{
> +	int master;
> +
> +	igt_fixture
> +		master = drm_open_driver(DRIVER_ANY);
> +
> +	igt_assert(check_auth(master) == true);


You can't use igt_assert outside of igt_fixture/igt_subtest*.


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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

Hi Petri,

Thanks for the feedback.

On Thu, 7 Feb 2019 at 08:59, Petri Latvala <petri.latvala@intel.com> wrote:

> > +     igt_info("Openning card node from a non-priv. user.\n");
> > +     igt_info("On failure, double-check the node permissions\n");
> > +     /* FIXME: relate to the master given and fix all of IGT */
> > +     slave = drm_open_driver(DRIVER_ANY);
> > +


> > +     bool imp = has_prime_import(slave);
> > +     bool rend = has_render_node(slave);
> > +     igt_info("import %d rend %d\n", imp, rend);
This debug will be dropped with next version.

> > +     if (has_prime_import(slave) && has_render_node(slave))
> > +             igt_assert(errno != EACCES);
> > +
> > +     else
> > +             igt_assert(errno == EACCES);
And this should be:

if (has_prime_import(slave)
   igt_skip(...)

if (has_render_node(slave))
    igt_assert(errno != EACCES);
else
    igt_assert(errno == EACCES);

The only problem is that igt_skip() does not work with igt_fork. Any
suggestions?

Fwiw, this problem inspired the igt_info() around drm_open_master(),
since the latter ends up calling igt_skip().

Somewhat orthogonal, it seems more intuitive and cleaner to have the
high-level decision of igt_skip/pass/assert in the test itself.
Although that's a topic for another time.

> > +igt_main
> > +{
> > +     int master;
> > +
> > +     igt_fixture
> > +             master = drm_open_driver(DRIVER_ANY);
> > +
> > +     igt_assert(check_auth(master) == true);
>
>
> You can't use igt_assert outside of igt_fixture/igt_subtest*.
>
Thanks will fix. Guess that explains why the CI failed here.
I wonder why this igt_assert() works just just fine on my local machine.

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Petri Latvala]

On Thu, Feb 07, 2019 at 12:00:14PM +0000, Emil Velikov wrote:
> Thanks will fix. Guess that explains why the CI failed here.
> I wonder why this igt_assert() works just just fine on my local machine.

The failing operation was --list-subtests.


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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Wed, Feb 06, 2019 at 01:18:28PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> As the inline comment says, this test checks that the kernel allows
> unauthenticated master with render capable, RENDER_ALLOW ioctls.
> 
> The kernel commit has extra details why.
> 
> v2:
> 
> - drop RUN_AS_ROOT guard
> - call check_auth() on the {,un}authenticated device
> - check the device is PRIME (import) capable
> - check the device has render node
> - tweak expectations based on above three
> - elaborate why we care only about -EACCES
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  tests/core_unauth_vs_render.c | 182 ++++++++++++++++++++++++++++++++++
>  tests/meson.build             |   1 +
>  2 files changed, 183 insertions(+)
>  create mode 100644 tests/core_unauth_vs_render.c
> 
> diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> new file mode 100644
> index 00000000..82dd2ce9
> --- /dev/null
> +++ b/tests/core_unauth_vs_render.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright 2018 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.velikov@collabora.com>
> + */
> +
> +/*
> + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> + * DRM_RENDER_ALLOW ioctls.
> + */
> +
> +#include "igt.h"
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include <sys/poll.h>
> +#include <sys/resource.h>
> +#include <sys/sysmacros.h>
> +#include "drm.h"
> +
> +#ifdef __linux__
> +# include <sys/syscall.h>
> +#else
> +# include <pthread.h>
> +#endif
> +
> +/* Checks whether the thread id is the current thread */
> +static bool
> +is_local_tid(pid_t tid)
> +{
> +#ifndef __linux__
> +	return pthread_self() == tid;
> +#else
> +	/* On Linux systems, drmGetClient() would return the thread ID instead
> +	   of the actual process ID */
> +	return syscall(SYS_gettid) == tid;
> +#endif
> +}
> +
> +
> +static bool check_auth(int fd)
> +{
> +	pid_t client_pid;
> +	int i, auth, pid, uid;
> +	unsigned long magic, iocs;
> +	bool is_authenticated = false;
> +
> +	client_pid = getpid();
> +	for (i = 0; !is_authenticated; i++) {
> +		if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
> +			break;
> +		is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
> +	}
> +	return is_authenticated;
> +}

btw the core_auth merger landed, so you can stuff your new subtest in
there now.

> +
> +
> +static bool has_prime_import(int fd)
> +{
> +	uint64_t value;
> +
> +	if (drmGetCap(fd, DRM_CAP_PRIME, &value))
> +		return false;
> +
> +	return value & DRM_PRIME_CAP_IMPORT;
> +}
> +
> +static bool has_render_node(int fd)
> +{
> +	char node_name[80];
> +	struct stat sbuf;
> +
> +	if (fstat(fd, &sbuf))
> +		return false;
> +
> +	sprintf(node_name, "/dev/dri/renderD%d", minor(sbuf.st_rdev) | 0x80);
> +	if (stat(node_name, &sbuf))
> +		return false;
> +
> +	return true;
> +}
> +
> +IGT_TEST_DESCRIPTION("Call drmPrimeFDToHandle() from unauthenticated master doesn't return -EACCES.");
> +
> +static void test_unauth_vs_render(int master)
> +{
> +	int slave;
> +	int prime_fd = -1;
> +	uint32_t handle;
> +
> +	/*
> +	 * The second open() happens without CAP_SYS_ADMIN, thus it will NOT
> +	 * be authenticated.
> +	 */
> +	igt_info("Openning card node from a non-priv. user.\n");
> +	igt_info("On failure, double-check the node permissions\n");
> +	/* FIXME: relate to the master given and fix all of IGT */
> +	slave = drm_open_driver(DRIVER_ANY);
> +
> +	igt_require(slave >= 0);

igt_require/skip need to be outside of igt_fork. But this one here should
be an igt_assert I think, and the testcase needs to somehow make sure that
it will succeed. I think the namespace trick is probably the best option,
but that means open-coding the clone stuff, or rewriting igt_fork. I think
I need to look into that a bit ...

> +	igt_assert(check_auth(slave) == false);
> +
> +	/* Issuing the following ioctl will fail, no doubt about it. */
> +	igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);

Hm, I'd run this first on master and make sure we get -EBADF as errno.
Just to make sure the ioctl call we're doing does get through the
drm_ioctl() layers.

> +
> +	/*
> +	 * Updated kernels allow render capable, unauthenticated master to
> +	 * issue DRM_AUTH ioctls (like the above), as long as they are
> +	 * annotated as DRM_RENDER_ALLOW - just like FD2HANDLE above.
> +	 *
> +	 * Otherwise, errno is set to -EACCES
> +	 *
> +	 * Note: We are _not_ interested in the FD2HANDLE specific errno. Those
> +	 * should be checked other standalone tests.
> +	 */
> +	bool imp = has_prime_import(slave);

Hm I think has_prime_import should be an igt_require (and outside of the
igt_fork).

> +	bool rend = has_render_node(slave);
> +	igt_info("import %d rend %d\n", imp, rend);
> +	if (has_prime_import(slave) && has_render_node(slave))
> +		igt_assert(errno != EACCES);

Still think we should check for the errno we expect here (i.e. EBADF, if
we filter out !has_prime_import earlier).
-Daniel


> +
> +	else
> +		igt_assert(errno == EACCES);
> +
> +	close(slave);
> +}
> +
> +/*
> + * IGT is executed as root, although that may(?) change in the future.
> + * Thus we need to drop the privileges so that the second open() results in a
> + * client which is not unauthenticated. Running as normal user circumvents that.
> + *
> + * In both cases, we need to ensure the file permissions of the node are
> + * sufficient.
> + */
> +
> +igt_main
> +{
> +	int master;
> +
> +	igt_fixture
> +		master = drm_open_driver(DRIVER_ANY);
> +
> +	igt_assert(check_auth(master) == true);
> +
> +	igt_subtest("unauth-vs-render") {
> +		igt_fork(child, 1) {
> +			igt_drop_root();
> +			test_unauth_vs_render(master);
> +		}
> +		igt_waitchildren();
> +	}
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 0f12df26..e5200b36 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -1,5 +1,6 @@
>  test_progs = [
>  	'core_auth',
> +	'core_unauth_vs_render',
>  	'core_getclient',
>  	'core_getstats',
>  	'core_getversion',
> -- 
> 2.20.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

On Thu, 7 Feb 2019 at 14:17, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Feb 06, 2019 at 01:18:28PM +0000, Emil Velikov wrote:

> > +static bool check_auth(int fd)
> > +{
> > +     pid_t client_pid;
> > +     int i, auth, pid, uid;
> > +     unsigned long magic, iocs;
> > +     bool is_authenticated = false;
> > +
> > +     client_pid = getpid();
> > +     for (i = 0; !is_authenticated; i++) {
> > +             if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
> > +                     break;
> > +             is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
> > +     }
> > +     return is_authenticated;
> > +}
>
> btw the core_auth merger landed, so you can stuff your new subtest in
> there now.
>
Sure will give it a try. If any unrelated issues come up, I'm inclined
to keep it separate though.


[snip]

> > +static void test_unauth_vs_render(int master)
> > +{
> > +     int slave;
> > +     int prime_fd = -1;
> > +     uint32_t handle;
> > +
> > +     /*
> > +      * The second open() happens without CAP_SYS_ADMIN, thus it will NOT
> > +      * be authenticated.
> > +      */
> > +     igt_info("Openning card node from a non-priv. user.\n");
> > +     igt_info("On failure, double-check the node permissions\n");
> > +     /* FIXME: relate to the master given and fix all of IGT */
> > +     slave = drm_open_driver(DRIVER_ANY);
> > +
> > +     igt_require(slave >= 0);
>
> igt_require/skip need to be outside of igt_fork. But this one here should
> be an igt_assert I think, and the testcase needs to somehow make sure that
> it will succeed. I think the namespace trick is probably the best option,
> but that means open-coding the clone stuff, or rewriting igt_fork. I think
> I need to look into that a bit ...
>

Let me try and provide a complete picture, instead of my earlier
fragmented rumbling.

We drop root permissions before drm_open_driver(), as otherwise the
client will be authenticated.
To achieve that, while still preserving the atexit machinery we igt_fork().

If the dev node is missing permissions, say the user is not in the
video group, the open() call will fail.
Thus the igt_skip_on_f() in drm_open_driver() will trigger, which will
interact badly with igt_fork() as documented.

I think that fixing the igt_fork() and igt_skip() interaction is a
good idea, although beyond my current IGT knowledge.
IMHO the current igt_info() should be indicative enough, until
something better is available.

Additionally I wonder if we cannot:
- drop the igt_skip() from drm_open_driver()
- update the existing tests to consistently use igt_require() just after
   - some tests use igt_assert(), others igt_require() and some omit
any fd checking

It will not fix the underlying issue, yet it will make the tests more
consistent and easier to read.
What do you think? I'm ok with providing patches for that.

> > +     igt_assert(check_auth(slave) == false);
> > +
> > +     /* Issuing the following ioctl will fail, no doubt about it. */
> > +     igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
>
> Hm, I'd run this first on master and make sure we get -EBADF as errno.
> Just to make sure the ioctl call we're doing does get through the
> drm_ioctl() layers.
>
> > +
> > +     /*
> > +      * Updated kernels allow render capable, unauthenticated master to
> > +      * issue DRM_AUTH ioctls (like the above), as long as they are
> > +      * annotated as DRM_RENDER_ALLOW - just like FD2HANDLE above.
> > +      *
> > +      * Otherwise, errno is set to -EACCES
> > +      *
> > +      * Note: We are _not_ interested in the FD2HANDLE specific errno. Those
> > +      * should be checked other standalone tests.
> > +      */
> > +     bool imp = has_prime_import(slave);
>
> Hm I think has_prime_import should be an igt_require (and outside of the
> igt_fork).
>
Ack. That's a nice way to handle the igt_skip/igt_fork interaction.

> > +     bool rend = has_render_node(slave);
> > +     igt_info("import %d rend %d\n", imp, rend);
> > +     if (has_prime_import(slave) && has_render_node(slave))
> > +             igt_assert(errno != EACCES);
>
> Still think we should check for the errno we expect here (i.e. EBADF, if
> we filter out !has_prime_import earlier).

As alluded in the comment, the aim of this test is to check the
drm_permit() kernel function.
Since we don't have a specific IOCTL dedicated for it, we use
FD2HANDLE as an example.

The function drm_permit() returns EACCES so the test checks for that.


I'm more than happy to reword the comment to make this clear and
obvious. Suggestions are greatly appreciated.
Alternatively I could use EBADF although clearly comment that we're
happy with anything != EACCES.

Would you prefer that?


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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Thu, Feb 07, 2019 at 05:08:23PM +0000, Emil Velikov wrote:
> On Thu, 7 Feb 2019 at 14:17, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Feb 06, 2019 at 01:18:28PM +0000, Emil Velikov wrote:
> 
> > > +static bool check_auth(int fd)
> > > +{
> > > +     pid_t client_pid;
> > > +     int i, auth, pid, uid;
> > > +     unsigned long magic, iocs;
> > > +     bool is_authenticated = false;
> > > +
> > > +     client_pid = getpid();
> > > +     for (i = 0; !is_authenticated; i++) {
> > > +             if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
> > > +                     break;
> > > +             is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
> > > +     }
> > > +     return is_authenticated;
> > > +}
> >
> > btw the core_auth merger landed, so you can stuff your new subtest in
> > there now.
> >
> Sure will give it a try. If any unrelated issues come up, I'm inclined
> to keep it separate though.
> 
> 
> [snip]
> 
> > > +static void test_unauth_vs_render(int master)
> > > +{
> > > +     int slave;
> > > +     int prime_fd = -1;
> > > +     uint32_t handle;
> > > +
> > > +     /*
> > > +      * The second open() happens without CAP_SYS_ADMIN, thus it will NOT
> > > +      * be authenticated.
> > > +      */
> > > +     igt_info("Openning card node from a non-priv. user.\n");
> > > +     igt_info("On failure, double-check the node permissions\n");
> > > +     /* FIXME: relate to the master given and fix all of IGT */
> > > +     slave = drm_open_driver(DRIVER_ANY);
> > > +
> > > +     igt_require(slave >= 0);
> >
> > igt_require/skip need to be outside of igt_fork. But this one here should
> > be an igt_assert I think, and the testcase needs to somehow make sure that
> > it will succeed. I think the namespace trick is probably the best option,
> > but that means open-coding the clone stuff, or rewriting igt_fork. I think
> > I need to look into that a bit ...
> >
> 
> Let me try and provide a complete picture, instead of my earlier
> fragmented rumbling.
> 
> We drop root permissions before drm_open_driver(), as otherwise the
> client will be authenticated.
> To achieve that, while still preserving the atexit machinery we igt_fork().
> 
> If the dev node is missing permissions, say the user is not in the
> video group, the open() call will fail.
> Thus the igt_skip_on_f() in drm_open_driver() will trigger, which will
> interact badly with igt_fork() as documented.
> 
> I think that fixing the igt_fork() and igt_skip() interaction is a
> good idea, although beyond my current IGT knowledge.
> IMHO the current igt_info() should be indicative enough, until
> something better is available.

Yeah I guess until we have an igt_nonroot_fork block which does some magic
to gauarantee you can still open the device there's not much we can do
here. Huge FIXME comment and call it done, I'd even skip the igt_info.

> Additionally I wonder if we cannot:
> - drop the igt_skip() from drm_open_driver()
> - update the existing tests to consistently use igt_require() just after
>    - some tests use igt_assert(), others igt_require() and some omit
> any fd checking
> 
> It will not fix the underlying issue, yet it will make the tests more
> consistent and easier to read.
> What do you think? I'm ok with providing patches for that.

Yeah there's probably a pile of cargo-culting going on. So the rule of
thumb is that the normal igt functions always work, by bailing out in any
failure case using igt_skip. If you don't have the DRM device that we
need, then we should skip. So all these igt_assert/skips in tests
themselves should probably be thrown out.

For the case where you know what you're doing, there should be __foo
functions, which never bail out automatically. If these don't exist yet,
then we'll need to add it. You're lucky, __drm_open_driver already exists.

> 
> > > +     igt_assert(check_auth(slave) == false);
> > > +
> > > +     /* Issuing the following ioctl will fail, no doubt about it. */
> > > +     igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> >
> > Hm, I'd run this first on master and make sure we get -EBADF as errno.
> > Just to make sure the ioctl call we're doing does get through the
> > drm_ioctl() layers.
> >
> > > +
> > > +     /*
> > > +      * Updated kernels allow render capable, unauthenticated master to
> > > +      * issue DRM_AUTH ioctls (like the above), as long as they are
> > > +      * annotated as DRM_RENDER_ALLOW - just like FD2HANDLE above.
> > > +      *
> > > +      * Otherwise, errno is set to -EACCES
> > > +      *
> > > +      * Note: We are _not_ interested in the FD2HANDLE specific errno. Those
> > > +      * should be checked other standalone tests.
> > > +      */
> > > +     bool imp = has_prime_import(slave);
> >
> > Hm I think has_prime_import should be an igt_require (and outside of the
> > igt_fork).
> >
> Ack. That's a nice way to handle the igt_skip/igt_fork interaction.
> 
> > > +     bool rend = has_render_node(slave);
> > > +     igt_info("import %d rend %d\n", imp, rend);
> > > +     if (has_prime_import(slave) && has_render_node(slave))
> > > +             igt_assert(errno != EACCES);
> >
> > Still think we should check for the errno we expect here (i.e. EBADF, if
> > we filter out !has_prime_import earlier).
> 
> As alluded in the comment, the aim of this test is to check the
> drm_permit() kernel function.
> Since we don't have a specific IOCTL dedicated for it, we use
> FD2HANDLE as an example.
> 
> The function drm_permit() returns EACCES so the test checks for that.
> 
> 
> I'm more than happy to reword the comment to make this clear and
> obvious. Suggestions are greatly appreciated.
> Alternatively I could use EBADF although clearly comment that we're
> happy with anything != EACCES.
> 
> Would you prefer that?

Checking for EBADF plus explicit comment that we aim for anything !=
ENOACCESS sounds good. Plus also running the same ioctl on the first fd,
opened as root, and making sure we get an EBADF there too. That way if
there's ever a kernel change that moves some of these checks around, which
could break our testcase here, then we'll notice and can adjust the
testcase. Sometimes I even first do the ioctl in a way that should work,
and then change only 1 thing to get the case I want to exercise. That
makes sure we're always testing the right check, no matter in which order
the kernel checks stuff, since that check is the only thing that can
change the ioctl outcome from the previous run. Unfortunately that's not
possible here (importing dma-buf isn't simple), so specific bad outcome is
the next best thing we can do.

tldr; errno == EBADF + comment.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

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

As the inline comment says, this test checks that the kernel allows
unauthenticated master with render capable, RENDER_ALLOW ioctls.

The kernel commit has extra details why.

v2:
 - drop RUN_AS_ROOT guard
 - call check_auth() on the {,un}authenticated device
 - check the device is PRIME (import) capable
 - check the device has render node
 - tweak expectations based on above three
 - elaborate why we care only about -EACCES

v3:
 - fold into existing core_auth.c
 - move igt_assert within the subtest
 - make has_prime_import() an igt_require()
 - check for BADF before and after, as requested. Not strictly needed.
 - swap igt_info+drm_open_driver with comment + __drm_open_driver
Former calls igt_skip() which is problematic with igt_fork().

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Hope that covers everything Dan. Alternatively I would prefer to keep
polishing this, while the kernel fix is merged. This way existing users
would work :-)
---
 tests/core_auth.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/tests/core_auth.c b/tests/core_auth.c
index 0c016a37..0b9073cb 100644
--- a/tests/core_auth.c
+++ b/tests/core_auth.c
@@ -1,5 +1,6 @@
 /*
  * Copyright 2015 David Herrmann <dh.herrmann@gmail.com>
+ * Copyright 2018 Collabora, Ltd
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -40,6 +41,7 @@
 #include <sys/time.h>
 #include <sys/poll.h>
 #include <sys/resource.h>
+#include <sys/sysmacros.h>
 #include "drm.h"
 
 #ifdef __linux__
@@ -190,6 +192,98 @@ static void test_basic_auth(int master)
 	close(slave);
 }
 
+static bool has_prime_import(int fd)
+{
+	uint64_t value;
+
+	if (drmGetCap(fd, DRM_CAP_PRIME, &value))
+		return false;
+
+	return value & DRM_PRIME_CAP_IMPORT;
+}
+
+static void check_auth_sanity(int master)
+{
+	uint32_t handle;
+
+	igt_assert(check_auth(master) == true);
+	igt_require(has_prime_import(master));
+
+	igt_assert(drmPrimeFDToHandle(master, -1, &handle) < 0);
+
+	/* IOCTL requires authenticated master as done in drm_permit.
+	 * As we get past that, we'll fail due to the invalid FD.
+	 *
+	 * Note: strictly speaking this is unrelated to the goal of
+	 * the test, although danvet requested it.
+	 */
+	igt_assert(errno == EBADF);
+}
+
+static bool has_render_node(int fd)
+{
+	char node_name[80];
+	struct stat sbuf;
+
+	if (fstat(fd, &sbuf))
+		return false;
+
+	sprintf(node_name, "/dev/dri/renderD%d", minor(sbuf.st_rdev) | 0x80);
+	if (stat(node_name, &sbuf))
+		return false;
+
+	return true;
+}
+
+/*
+ * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
+ * DRM_RENDER_ALLOW ioctls.
+ */
+static void test_unauth_vs_render(int master)
+{
+	int slave;
+	uint32_t handle;
+
+	/*
+	 * FIXME: when drm_open_driver() fails to open() a node (insufficient
+	 * permissions or otherwise, it will igt_skip.
+	 * As of today, igt_skip and igt_fork do not work together.
+	 */
+	slave = __drm_open_driver(DRIVER_ANY);
+	/*
+	 * FIXME: relate to the master fd passed with the above open and fix
+	 * all of IGT.
+	 */
+
+	igt_assert(slave >= 0);
+
+	/*
+	 * The second open() happens without CAP_SYS_ADMIN, thus it will NOT
+	 * be authenticated.
+	 */
+	igt_assert(check_auth(slave) == false);
+
+	/* Issuing the following ioctl will fail, no doubt about it. */
+	igt_assert(drmPrimeFDToHandle(slave, -1, &handle) < 0);
+
+	/*
+	 * Updated kernels allow render capable, unauthenticated master to
+	 * issue DRM_AUTH ioctls (like FD2HANDLE above), as long as they are
+	 * annotated as DRM_RENDER_ALLOW.
+	 *
+	 * Otherwise, errno is set to -EACCES
+	 *
+	 * Note: We are _not_ interested in the FD2HANDLE specific errno,
+	 * yet the EBADF check is added on the explicit request by danvet.
+	 */
+	if (has_render_node(slave))
+		igt_assert(errno == EBADF);
+	else
+		igt_assert(errno == EACCES);
+
+	close(slave);
+}
+
 igt_main
 {
 	int master;
@@ -228,4 +322,19 @@ igt_main
 		igt_subtest("many-magics")
 			test_many_magics(master);
 	}
+
+	igt_subtest_group {
+		igt_fixture
+			master = drm_open_driver(DRIVER_ANY);
+
+		igt_subtest("unauth-vs-render") {
+			check_auth_sanity(master);
+
+			igt_fork(child, 1) {
+				igt_drop_root();
+				test_unauth_vs_render(master);
+			}
+			igt_waitchildren();
+		}
+	}
 }
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

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

As the inline comment says, this test checks that the kernel allows
unauthenticated master with render capable, RENDER_ALLOW ioctls.

The kernel commit has extra details why.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 tests/core_unauth_vs_render.c | 108 ++++++++++++++++++++++++++++++++++
 tests/meson.build             |   1 +
 2 files changed, 109 insertions(+)
 create mode 100644 tests/core_unauth_vs_render.c

diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
new file mode 100644
index 00000000..a7d70d77
--- /dev/null
+++ b/tests/core_unauth_vs_render.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2018 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.velikov@collabora.com>
+ */
+
+/*
+ * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
+ * DRM_RENDER_ALLOW ioctls.
+ */
+
+#include "igt.h"
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <signal.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include <sys/poll.h>
+#include <sys/resource.h>
+#include "drm.h"
+
+IGT_TEST_DESCRIPTION("Call XXX from unauthenticated master doesn't return -EACCES.");
+
+static void test_unauth_vs_render(int master)
+{
+	int slave;
+	int prime_fd;
+	uint32_t handle;
+
+	/*
+	 * The second open() happens without CAP_SYS_ADMIN, thus it
+	 * will not be authenticated.
+	 */
+	slave = drm_open_driver(DRIVER_ANY); // XXX: relate to the master given?
+	igt_require(slave >= 0);
+
+	/* Issuing the following ioctl will fail, no doubt about it. */
+	igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
+
+	/*
+	 * Updated kernels allow render capable, unauthenticated
+	 * master to issue DRM_AUTH ioctls (like the above), as long as
+	 * they are annotated as DRM_RENDER_ALLOW.
+	 *
+	 * Older ones throw -EACCES.
+	 */
+	igt_assert(errno != EACCES);
+
+	close(slave);
+}
+
+/*
+ * By default IGT is executed as root.
+ * Thus we need to drop the priviladges so that the second open() results in a
+ * client which is not unauthenticated. Running as normal user cercumtains that.
+ *
+ * In both cases, we need to ensure the file permissions of the node are
+ * sufficient.
+ */
+#define RUN_AS_ROOT 1
+
+igt_main
+{
+	int master;
+
+	igt_fixture
+		master = drm_open_driver(DRIVER_ANY);
+
+	//igt_debugfs_dump(master, "clients");
+	igt_subtest("unauth-vs-render") {
+#if RUN_AS_ROOT
+		igt_fork(child, 1) {
+			igt_drop_root();
+#endif
+			test_unauth_vs_render(master);
+#if RUN_AS_ROOT
+		}
+		igt_waitchildren();
+#endif
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index b8a6e61b..9bfd706b 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -1,5 +1,6 @@
 test_progs = [
 	'core_auth',
+	'core_unauth_vs_render',
 	'core_get_client_auth',
 	'core_getclient',
 	'core_getstats',
-- 
2.20.1

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Petri Latvala]

Just drive-by spellchecking:


On Mon, Jan 14, 2019 at 08:39:37AM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> As the inline comment says, this test checks that the kernel allows
> unauthenticated master with render capable, RENDER_ALLOW ioctls.
> 
> The kernel commit has extra details why.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  tests/core_unauth_vs_render.c | 108 ++++++++++++++++++++++++++++++++++
>  tests/meson.build             |   1 +
>  2 files changed, 109 insertions(+)
>  create mode 100644 tests/core_unauth_vs_render.c
> 
> diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> new file mode 100644
> index 00000000..a7d70d77
> --- /dev/null
> +++ b/tests/core_unauth_vs_render.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright 2018 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.velikov@collabora.com>
> + */
> +
> +/*
> + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> + * DRM_RENDER_ALLOW ioctls.
> + */
> +
> +#include "igt.h"
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include <sys/poll.h>
> +#include <sys/resource.h>
> +#include "drm.h"
> +
> +IGT_TEST_DESCRIPTION("Call XXX from unauthenticated master doesn't return -EACCES.");
> +
> +static void test_unauth_vs_render(int master)
> +{
> +	int slave;
> +	int prime_fd;
> +	uint32_t handle;
> +
> +	/*
> +	 * The second open() happens without CAP_SYS_ADMIN, thus it
> +	 * will not be authenticated.
> +	 */
> +	slave = drm_open_driver(DRIVER_ANY); // XXX: relate to the master given?
> +	igt_require(slave >= 0);
> +
> +	/* Issuing the following ioctl will fail, no doubt about it. */
> +	igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> +
> +	/*
> +	 * Updated kernels allow render capable, unauthenticated
> +	 * master to issue DRM_AUTH ioctls (like the above), as long as
> +	 * they are annotated as DRM_RENDER_ALLOW.
> +	 *
> +	 * Older ones throw -EACCES.
> +	 */
> +	igt_assert(errno != EACCES);
> +
> +	close(slave);
> +}
> +
> +/*
> + * By default IGT is executed as root.
> + * Thus we need to drop the priviladges so that the second open() results in a

privileges

> + * client which is not unauthenticated. Running as normal user cercumtains that.

circumvents?




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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

On Mon, 14 Jan 2019 at 10:58, Petri Latvala <petri.latvala@intel.com> wrote:
>
> Just drive-by spellchecking:
>
>
> On Mon, Jan 14, 2019 at 08:39:37AM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > As the inline comment says, this test checks that the kernel allows
> > unauthenticated master with render capable, RENDER_ALLOW ioctls.
> >
> > The kernel commit has extra details why.
> >
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> >  tests/core_unauth_vs_render.c | 108 ++++++++++++++++++++++++++++++++++
> >  tests/meson.build             |   1 +
> >  2 files changed, 109 insertions(+)
> >  create mode 100644 tests/core_unauth_vs_render.c
> >
> > diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> > new file mode 100644
> > index 00000000..a7d70d77
> > --- /dev/null
> > +++ b/tests/core_unauth_vs_render.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + * Copyright 2018 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.velikov@collabora.com>
> > + */
> > +
> > +/*
> > + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> > + * DRM_RENDER_ALLOW ioctls.
> > + */
> > +
> > +#include "igt.h"
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <signal.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/time.h>
> > +#include <sys/poll.h>
> > +#include <sys/resource.h>
> > +#include "drm.h"
> > +
> > +IGT_TEST_DESCRIPTION("Call XXX from unauthenticated master doesn't return -EACCES.");
> > +
Oops this should read "Call drmPrimeFDToHandle() ..."

> > +/*
> > + * By default IGT is executed as root.
> > + * Thus we need to drop the priviladges so that the second open() results in a
>
> privileges
>
> > + * client which is not unauthenticated. Running as normal user cercumtains that.
>
> circumvents?
>
Thanks fixed locally.

Fwiw the RUN_AS_ROOT approach feels a bit dirty. Yet alternatives,
such as make IGT run w/o root, require massive undertaking for little
benefit.

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Mon, Jan 14, 2019 at 08:39:37AM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> As the inline comment says, this test checks that the kernel allows
> unauthenticated master with render capable, RENDER_ALLOW ioctls.
> 
> The kernel commit has extra details why.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Sorry for the late reply, got distracted and all that :-/

> ---
>  tests/core_unauth_vs_render.c | 108 ++++++++++++++++++++++++++++++++++
>  tests/meson.build             |   1 +
>  2 files changed, 109 insertions(+)
>  create mode 100644 tests/core_unauth_vs_render.c
> 
> diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> new file mode 100644
> index 00000000..a7d70d77
> --- /dev/null
> +++ b/tests/core_unauth_vs_render.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright 2018 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.velikov@collabora.com>
> + */
> +
> +/*
> + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> + * DRM_RENDER_ALLOW ioctls.
> + */
> +
> +#include "igt.h"
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include <sys/poll.h>
> +#include <sys/resource.h>
> +#include "drm.h"
> +
> +IGT_TEST_DESCRIPTION("Call XXX from unauthenticated master doesn't return -EACCES.");
> +
> +static void test_unauth_vs_render(int master)
> +{
> +	int slave;
> +	int prime_fd;
> +	uint32_t handle;
> +
> +	/*
> +	 * The second open() happens without CAP_SYS_ADMIN, thus it
> +	 * will not be authenticated.
> +	 */
> +	slave = drm_open_driver(DRIVER_ANY); // XXX: relate to the master given?
> +	igt_require(slave >= 0);
> +
> +	/* Issuing the following ioctl will fail, no doubt about it. */
> +	igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> +
> +	/*
> +	 * Updated kernels allow render capable, unauthenticated
> +	 * master to issue DRM_AUTH ioctls (like the above), as long as
> +	 * they are annotated as DRM_RENDER_ALLOW.
> +	 *
> +	 * Older ones throw -EACCES.
> +	 */
> +	igt_assert(errno != EACCES);
> +
> +	close(slave);

Hm I think we need a more strict testcase here. What I like doing is
doing the exact same ioctl twice, once where it should fail, and once
where it doesn't fail. We also need to check for render_allow here
somehow, or this will fail on some drivers. Except for this case here we
want it to succeed both times, but once authenticated and once where we've
made sure we're not authenticated.

I think the following should give us a solid testcase:

1. igt_require(getcap(DRM_CAP_PRIME))

2. Open the primary node, make sure we're authenticated (reusing
check_auth from core_get_client_auth should help). Make sure a render node
for this primary node exists, igt_skip if that's not the case. Might need
a new library function. We need this to handle render-less drivers, which
is the standard for kms-only drm drivers.

3. FD2HANDLE with -1 as handle should always fail with EBADF, so we can
check for that exact error. If we just check for anything going wrong,
we'll catch much fewer bugs.

4. Open the device as non-root, make sure we are _not_ authenticated
(using check_auth).

5. FD2HANDLE like in 3, fail if we get anything else than EBADF.


> +}
> +
> +/*
> + * By default IGT is executed as root.

It's not just the default, it's a hard requirement. The runner has checks
for other drm clients and whether you're root and bails out if that's not
the case. Lots&lots of igts break if you run them as non-root or with
other drm clients running. Only thing that's allowed is enumerating
subtests (because we need that on our build machines to generate the
testlists).

tldr; Please fold in.

> + * Thus we need to drop the priviladges so that the second open() results in a
> + * client which is not unauthenticated. Running as normal user cercumtains that.
> + *
> + * In both cases, we need to ensure the file permissions of the node are
> + * sufficient.
> + */
> +#define RUN_AS_ROOT 1
> +
> +igt_main
> +{
> +	int master;
> +
> +	igt_fixture
> +		master = drm_open_driver(DRIVER_ANY);
> +
> +	//igt_debugfs_dump(master, "clients");
> +	igt_subtest("unauth-vs-render") {
> +#if RUN_AS_ROOT
> +		igt_fork(child, 1) {
> +			igt_drop_root();
> +#endif
> +			test_unauth_vs_render(master);
> +#if RUN_AS_ROOT
> +		}
> +		igt_waitchildren();
> +#endif
> +	}
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index b8a6e61b..9bfd706b 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -1,5 +1,6 @@
>  test_progs = [
>  	'core_auth',
> +	'core_unauth_vs_render',
>  	'core_get_client_auth',

I think it would make sense to put all the auth tests into core_auth,
together with core_get_client_auth. That way we can also reuse the
check_auth function without copypasting it.

Cheers, Daniel

>  	'core_getclient',
>  	'core_getstats',
> -- 
> 2.20.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

On Fri, 18 Jan 2019 at 15:58, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jan 14, 2019 at 08:39:37AM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > As the inline comment says, this test checks that the kernel allows
> > unauthenticated master with render capable, RENDER_ALLOW ioctls.
> >
> > The kernel commit has extra details why.
> >
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>
> Sorry for the late reply, got distracted and all that :-/
>
Looking at the reply I understand why :-P

> > ---
> >  tests/core_unauth_vs_render.c | 108 ++++++++++++++++++++++++++++++++++
> >  tests/meson.build             |   1 +
> >  2 files changed, 109 insertions(+)
> >  create mode 100644 tests/core_unauth_vs_render.c
> >
> > diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> > new file mode 100644
> > index 00000000..a7d70d77
> > --- /dev/null
> > +++ b/tests/core_unauth_vs_render.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + * Copyright 2018 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.velikov@collabora.com>
> > + */
> > +
> > +/*
> > + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> > + * DRM_RENDER_ALLOW ioctls.
> > + */
> > +
> > +#include "igt.h"
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <signal.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/time.h>
> > +#include <sys/poll.h>
> > +#include <sys/resource.h>
> > +#include "drm.h"
> > +
> > +IGT_TEST_DESCRIPTION("Call XXX from unauthenticated master doesn't return -EACCES.");
> > +
> > +static void test_unauth_vs_render(int master)
> > +{
> > +     int slave;
> > +     int prime_fd;
> > +     uint32_t handle;
> > +
> > +     /*
> > +      * The second open() happens without CAP_SYS_ADMIN, thus it
> > +      * will not be authenticated.
> > +      */
> > +     slave = drm_open_driver(DRIVER_ANY); // XXX: relate to the master given?
> > +     igt_require(slave >= 0);
> > +
> > +     /* Issuing the following ioctl will fail, no doubt about it. */
> > +     igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> > +
> > +     /*
> > +      * Updated kernels allow render capable, unauthenticated
> > +      * master to issue DRM_AUTH ioctls (like the above), as long as
> > +      * they are annotated as DRM_RENDER_ALLOW.
> > +      *
> > +      * Older ones throw -EACCES.
> > +      */
> > +     igt_assert(errno != EACCES);
> > +
> > +     close(slave);
>
> Hm I think we need a more strict testcase here. What I like doing is
> doing the exact same ioctl twice, once where it should fail, and once
> where it doesn't fail. We also need to check for render_allow here
> somehow, or this will fail on some drivers. Except for this case here we
> want it to succeed both times, but once authenticated and once where we've
> made sure we're not authenticated.
>
> I think the following should give us a solid testcase:
>
> 1. igt_require(getcap(DRM_CAP_PRIME))
>
> 2. Open the primary node, make sure we're authenticated (reusing
> check_auth from core_get_client_auth should help). Make sure a render node
> for this primary node exists, igt_skip if that's not the case. Might need
> a new library function. We need this to handle render-less drivers, which
> is the standard for kms-only drm drivers.
>
How do you envision the card* to renderD* helper? Should I go with the
minor(rdev) & 0x80 hack, use the libdrm helpers or something else?

A simple drm_open_driver() opens /dev/dri/card0 twice, when using the
drm_open_driver_master we end up with an extra open().
The kernel drm_getclient() was hollowed-out severely (commit
719524df4a2e48fa7ca3ad1697fd9a7f85ec8ad3), so I'm not sure if/how
reliably we can use that.

That's why I used igt_debugfs_dump() to print the state ;-)

> 3. FD2HANDLE with -1 as handle should always fail with EBADF, so we can
> check for that exact error. If we just check for anything going wrong,
> we'll catch much fewer bugs.
>
Ouch, could swear I used -1 at some point.

Why check for EBADF instead of EACCES? After all we're interested if
the permission is being handled correctly and everything else is
extra.

> 4. Open the device as non-root, make sure we are _not_ authenticated
> (using check_auth).
>
> 5. FD2HANDLE like in 3, fail if we get anything else than EBADF.
>
Or perhaps keep the EBADF for 3 and use EACCES for here?

>
> > +}
> > +
> > +/*
> > + * By default IGT is executed as root.
>
> It's not just the default, it's a hard requirement. The runner has checks
> for other drm clients and whether you're root and bails out if that's not
> the case. Lots&lots of igts break if you run them as non-root or with
> other drm clients running. Only thing that's allowed is enumerating
> subtests (because we need that on our build machines to generate the
> testlists).
>
> tldr; Please fold in.
>
Sure will fold.

Modulo an exception or two, IGT tests only require debugfs (write perm
for i915) and user in the video group (to open the node).
That's based on a quick look/test.

Since few distributions don't run the display server and/or compositor
as root, ideally IGT will get root-free at some point.
Ake IGT testing will be closer to real world use-cases.

> > @@ -1,5 +1,6 @@
> >  test_progs = [
> >       'core_auth',
> > +     'core_unauth_vs_render',
> >       'core_get_client_auth',
>
> I think it would make sense to put all the auth tests into core_auth,
> together with core_get_client_auth. That way we can also reuse the
> check_auth function without copypasting it.
>
Considering my luck, I'm weary that combining things will open another
can of worms.
Do you mind if I keep the test separate, so that the kernel fix isn't blocked?

For example, using drm_open_driver_master (as seen in core_auth) for
the unauth_vs_render test did not go well.
Other interesting issues include, igt_fork and igt_skip* (implicit
hidden in the drm_open_driver maze) conflicts.

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Tue, Jan 22, 2019 at 05:44:02PM +0000, Emil Velikov wrote:
> On Fri, 18 Jan 2019 at 15:58, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jan 14, 2019 at 08:39:37AM +0000, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > >
> > > As the inline comment says, this test checks that the kernel allows
> > > unauthenticated master with render capable, RENDER_ALLOW ioctls.
> > >
> > > The kernel commit has extra details why.
> > >
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >
> > Sorry for the late reply, got distracted and all that :-/
> >
> Looking at the reply I understand why :-P
> 
> > > ---
> > >  tests/core_unauth_vs_render.c | 108 ++++++++++++++++++++++++++++++++++
> > >  tests/meson.build             |   1 +
> > >  2 files changed, 109 insertions(+)
> > >  create mode 100644 tests/core_unauth_vs_render.c
> > >
> > > diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> > > new file mode 100644
> > > index 00000000..a7d70d77
> > > --- /dev/null
> > > +++ b/tests/core_unauth_vs_render.c
> > > @@ -0,0 +1,108 @@
> > > +/*
> > > + * Copyright 2018 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.velikov@collabora.com>
> > > + */
> > > +
> > > +/*
> > > + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> > > + * DRM_RENDER_ALLOW ioctls.
> > > + */
> > > +
> > > +#include "igt.h"
> > > +#include <unistd.h>
> > > +#include <stdlib.h>
> > > +#include <stdint.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +#include <signal.h>
> > > +#include <fcntl.h>
> > > +#include <inttypes.h>
> > > +#include <errno.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/ioctl.h>
> > > +#include <sys/time.h>
> > > +#include <sys/poll.h>
> > > +#include <sys/resource.h>
> > > +#include "drm.h"
> > > +
> > > +IGT_TEST_DESCRIPTION("Call XXX from unauthenticated master doesn't return -EACCES.");
> > > +
> > > +static void test_unauth_vs_render(int master)
> > > +{
> > > +     int slave;
> > > +     int prime_fd;
> > > +     uint32_t handle;
> > > +
> > > +     /*
> > > +      * The second open() happens without CAP_SYS_ADMIN, thus it
> > > +      * will not be authenticated.
> > > +      */
> > > +     slave = drm_open_driver(DRIVER_ANY); // XXX: relate to the master given?
> > > +     igt_require(slave >= 0);
> > > +
> > > +     /* Issuing the following ioctl will fail, no doubt about it. */
> > > +     igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> > > +
> > > +     /*
> > > +      * Updated kernels allow render capable, unauthenticated
> > > +      * master to issue DRM_AUTH ioctls (like the above), as long as
> > > +      * they are annotated as DRM_RENDER_ALLOW.
> > > +      *
> > > +      * Older ones throw -EACCES.
> > > +      */
> > > +     igt_assert(errno != EACCES);
> > > +
> > > +     close(slave);
> >
> > Hm I think we need a more strict testcase here. What I like doing is
> > doing the exact same ioctl twice, once where it should fail, and once
> > where it doesn't fail. We also need to check for render_allow here
> > somehow, or this will fail on some drivers. Except for this case here we
> > want it to succeed both times, but once authenticated and once where we've
> > made sure we're not authenticated.
> >
> > I think the following should give us a solid testcase:
> >
> > 1. igt_require(getcap(DRM_CAP_PRIME))
> >
> > 2. Open the primary node, make sure we're authenticated (reusing
> > check_auth from core_get_client_auth should help). Make sure a render node
> > for this primary node exists, igt_skip if that's not the case. Might need
> > a new library function. We need this to handle render-less drivers, which
> > is the standard for kms-only drm drivers.
> >
> How do you envision the card* to renderD* helper? Should I go with the
> minor(rdev) & 0x80 hack, use the libdrm helpers or something else?

I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
:-/

> A simple drm_open_driver() opens /dev/dri/card0 twice, when using the
> drm_open_driver_master we end up with an extra open().
> The kernel drm_getclient() was hollowed-out severely (commit
> 719524df4a2e48fa7ca3ad1697fd9a7f85ec8ad3), so I'm not sure if/how
> reliably we can use that.

I wrote that igt to validate the hollowed-out getclient, and it's used by
libva (somehow all these hacks are used by libva) to check whether it's
authenticated. So should be accurate I hope.

> That's why I used igt_debugfs_dump() to print the state ;-)

Yeah, but for igt I want tests that check themselves, not
humans-in-the-loop stuff. The interactive/debug stuff is useful for humans
to debug when things fail.

> > 3. FD2HANDLE with -1 as handle should always fail with EBADF, so we can
> > check for that exact error. If we just check for anything going wrong,
> > we'll catch much fewer bugs.
> >
> Ouch, could swear I used -1 at some point.
> 
> Why check for EBADF instead of EACCES? After all we're interested if
> the permission is being handled correctly and everything else is
> extra.
> 
> > 4. Open the device as non-root, make sure we are _not_ authenticated
> > (using check_auth).
> >
> > 5. FD2HANDLE like in 3, fail if we get anything else than EBADF.
> >
> Or perhaps keep the EBADF for 3 and use EACCES for here?

The idea behind EBADF is that you can only get that from the ioctl
implementation, which means the core drm auth checks have been passed
already. So it's positive confirmation that the checks work as intended.
Now ideally we'd test one case where we get success against one case where
we get a specific failure (so that even when someone completely reorders
the ioctl implementation we're still hitting the right case and the test
still tests what it should test). But I couldn't come up with something
like that:

- Creating a dma-buf that's guaranteed to work for importing out of thin
  air is hard.

- With your kernel change _everything_ works, at least if we have a render
  node capable driver. So there's not really any known-to-fail case left
  anymore.
 
But yeah it's not perfect.

I guess if you want to check for -EACCESS make a 2nd test (or merge them)
which makes sure that if we have a non-render node, then we get an
EACCESS. Would mean a quick testrun on a display-only drm driver (those
are all non-rendernode) to make sure it works. And would be an even better
test I think.

> 
> >
> > > +}
> > > +
> > > +/*
> > > + * By default IGT is executed as root.
> >
> > It's not just the default, it's a hard requirement. The runner has checks
> > for other drm clients and whether you're root and bails out if that's not
> > the case. Lots&lots of igts break if you run them as non-root or with
> > other drm clients running. Only thing that's allowed is enumerating
> > subtests (because we need that on our build machines to generate the
> > testlists).
> >
> > tldr; Please fold in.
> >
> Sure will fold.
> 
> Modulo an exception or two, IGT tests only require debugfs (write perm
> for i915) and user in the video group (to open the node).
> That's based on a quick look/test.
> 
> Since few distributions don't run the display server and/or compositor
> as root, ideally IGT will get root-free at some point.
> Ake IGT testing will be closer to real world use-cases.

IGT isnt like piglit, most of the tests check for corner cases where you
really can't have anything else touching the display (e.g. anything kms).
Being root is the easiest way to check for that (in debugfs, if there's
any other drm client the igt runner complains&quits). I guess there's some
igts which can be run like piglits, but those are the rare exceptions.

Now the entire "run compositor as non-root" business is a good point. We
do have some igts that drop CAP_SYS_ADMIN (see igt_drop_root()), but none
that make sure this holds for kms/core functionality. It's just a few
i915-gem tests. Would be a good gap to fill though. I think for the
non-rendernode EACCESS test we'll need that (since root can do whatever).

> > > @@ -1,5 +1,6 @@
> > >  test_progs = [
> > >       'core_auth',
> > > +     'core_unauth_vs_render',
> > >       'core_get_client_auth',
> >
> > I think it would make sense to put all the auth tests into core_auth,
> > together with core_get_client_auth. That way we can also reuse the
> > check_auth function without copypasting it.
> >
> Considering my luck, I'm weary that combining things will open another
> can of worms.
> Do you mind if I keep the test separate, so that the kernel fix isn't blocked?
> 
> For example, using drm_open_driver_master (as seen in core_auth) for
> the unauth_vs_render test did not go well.
> Other interesting issues include, igt_fork and igt_skip* (implicit
> hidden in the drm_open_driver maze) conflicts.

Hm, should work. I just wanted to avoid copypasteing the check_auth
function. You need to be somewhat careful with merging them though, needs
an igt_subtest_group. I can type the prep patch. Or follow up&rebase once
this has landed, if you prefer.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Wed, Jan 23, 2019 at 12:18:14PM +0100, Daniel Vetter wrote:
> On Tue, Jan 22, 2019 at 05:44:02PM +0000, Emil Velikov wrote:
> > On Fri, 18 Jan 2019 at 15:58, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Jan 14, 2019 at 08:39:37AM +0000, Emil Velikov wrote:
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > >
> > > > As the inline comment says, this test checks that the kernel allows
> > > > unauthenticated master with render capable, RENDER_ALLOW ioctls.
> > > >
> > > > The kernel commit has extra details why.
> > > >
> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > >
> > > Sorry for the late reply, got distracted and all that :-/
> > >
> > Looking at the reply I understand why :-P
> > 
> > > > ---
> > > >  tests/core_unauth_vs_render.c | 108 ++++++++++++++++++++++++++++++++++
> > > >  tests/meson.build             |   1 +
> > > >  2 files changed, 109 insertions(+)
> > > >  create mode 100644 tests/core_unauth_vs_render.c
> > > >
> > > > diff --git a/tests/core_unauth_vs_render.c b/tests/core_unauth_vs_render.c
> > > > new file mode 100644
> > > > index 00000000..a7d70d77
> > > > --- /dev/null
> > > > +++ b/tests/core_unauth_vs_render.c
> > > > @@ -0,0 +1,108 @@
> > > > +/*
> > > > + * Copyright 2018 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.velikov@collabora.com>
> > > > + */
> > > > +
> > > > +/*
> > > > + * Testcase: Render capable, unauthenticated master doesn't throw -EACCES for
> > > > + * DRM_RENDER_ALLOW ioctls.
> > > > + */
> > > > +
> > > > +#include "igt.h"
> > > > +#include <unistd.h>
> > > > +#include <stdlib.h>
> > > > +#include <stdint.h>
> > > > +#include <stdio.h>
> > > > +#include <string.h>
> > > > +#include <signal.h>
> > > > +#include <fcntl.h>
> > > > +#include <inttypes.h>
> > > > +#include <errno.h>
> > > > +#include <sys/stat.h>
> > > > +#include <sys/ioctl.h>
> > > > +#include <sys/time.h>
> > > > +#include <sys/poll.h>
> > > > +#include <sys/resource.h>
> > > > +#include "drm.h"
> > > > +
> > > > +IGT_TEST_DESCRIPTION("Call XXX from unauthenticated master doesn't return -EACCES.");
> > > > +
> > > > +static void test_unauth_vs_render(int master)
> > > > +{
> > > > +     int slave;
> > > > +     int prime_fd;
> > > > +     uint32_t handle;
> > > > +
> > > > +     /*
> > > > +      * The second open() happens without CAP_SYS_ADMIN, thus it
> > > > +      * will not be authenticated.
> > > > +      */
> > > > +     slave = drm_open_driver(DRIVER_ANY); // XXX: relate to the master given?
> > > > +     igt_require(slave >= 0);
> > > > +
> > > > +     /* Issuing the following ioctl will fail, no doubt about it. */
> > > > +     igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> > > > +
> > > > +     /*
> > > > +      * Updated kernels allow render capable, unauthenticated
> > > > +      * master to issue DRM_AUTH ioctls (like the above), as long as
> > > > +      * they are annotated as DRM_RENDER_ALLOW.
> > > > +      *
> > > > +      * Older ones throw -EACCES.
> > > > +      */
> > > > +     igt_assert(errno != EACCES);
> > > > +
> > > > +     close(slave);
> > >
> > > Hm I think we need a more strict testcase here. What I like doing is
> > > doing the exact same ioctl twice, once where it should fail, and once
> > > where it doesn't fail. We also need to check for render_allow here
> > > somehow, or this will fail on some drivers. Except for this case here we
> > > want it to succeed both times, but once authenticated and once where we've
> > > made sure we're not authenticated.
> > >
> > > I think the following should give us a solid testcase:
> > >
> > > 1. igt_require(getcap(DRM_CAP_PRIME))
> > >
> > > 2. Open the primary node, make sure we're authenticated (reusing
> > > check_auth from core_get_client_auth should help). Make sure a render node
> > > for this primary node exists, igt_skip if that's not the case. Might need
> > > a new library function. We need this to handle render-less drivers, which
> > > is the standard for kms-only drm drivers.
> > >
> > How do you envision the card* to renderD* helper? Should I go with the
> > minor(rdev) & 0x80 hack, use the libdrm helpers or something else?
> 
> I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
> :-/
> 
> > A simple drm_open_driver() opens /dev/dri/card0 twice, when using the
> > drm_open_driver_master we end up with an extra open().
> > The kernel drm_getclient() was hollowed-out severely (commit
> > 719524df4a2e48fa7ca3ad1697fd9a7f85ec8ad3), so I'm not sure if/how
> > reliably we can use that.
> 
> I wrote that igt to validate the hollowed-out getclient, and it's used by
> libva (somehow all these hacks are used by libva) to check whether it's
> authenticated. So should be accurate I hope.
> 
> > That's why I used igt_debugfs_dump() to print the state ;-)
> 
> Yeah, but for igt I want tests that check themselves, not
> humans-in-the-loop stuff. The interactive/debug stuff is useful for humans
> to debug when things fail.
> 
> > > 3. FD2HANDLE with -1 as handle should always fail with EBADF, so we can
> > > check for that exact error. If we just check for anything going wrong,
> > > we'll catch much fewer bugs.
> > >
> > Ouch, could swear I used -1 at some point.
> > 
> > Why check for EBADF instead of EACCES? After all we're interested if
> > the permission is being handled correctly and everything else is
> > extra.
> > 
> > > 4. Open the device as non-root, make sure we are _not_ authenticated
> > > (using check_auth).
> > >
> > > 5. FD2HANDLE like in 3, fail if we get anything else than EBADF.
> > >
> > Or perhaps keep the EBADF for 3 and use EACCES for here?
> 
> The idea behind EBADF is that you can only get that from the ioctl
> implementation, which means the core drm auth checks have been passed
> already. So it's positive confirmation that the checks work as intended.
> Now ideally we'd test one case where we get success against one case where
> we get a specific failure (so that even when someone completely reorders
> the ioctl implementation we're still hitting the right case and the test
> still tests what it should test). But I couldn't come up with something
> like that:
> 
> - Creating a dma-buf that's guaranteed to work for importing out of thin
>   air is hard.
> 
> - With your kernel change _everything_ works, at least if we have a render
>   node capable driver. So there's not really any known-to-fail case left
>   anymore.
>  
> But yeah it's not perfect.
> 
> I guess if you want to check for -EACCESS make a 2nd test (or merge them)
> which makes sure that if we have a non-render node, then we get an
> EACCESS. Would mean a quick testrun on a display-only drm driver (those
> are all non-rendernode) to make sure it works. And would be an even better
> test I think.

I just realized that as soon as you open a file as root you're
authenticated, so this tests nothing. I think we need a new
drm_open_any_nonroot(), which drops CAP_SYS_ADMIN (while otherwise 
staying as root, can't open the mkdev otherwise I think) and then restores
it. Or something like this. Tbh not quite sure how to make this happen
exactly.

Noticed while I was trying to create a new check_auth() subtest for
non-root not being authenticated from the start, and it didn't fail like I
thought it should :-)

So a simple

	drm_open_any();
	igt_drop_root();

Doesn't cut it unfortunately :-/
-Daniel

> 
> > 
> > >
> > > > +}
> > > > +
> > > > +/*
> > > > + * By default IGT is executed as root.
> > >
> > > It's not just the default, it's a hard requirement. The runner has checks
> > > for other drm clients and whether you're root and bails out if that's not
> > > the case. Lots&lots of igts break if you run them as non-root or with
> > > other drm clients running. Only thing that's allowed is enumerating
> > > subtests (because we need that on our build machines to generate the
> > > testlists).
> > >
> > > tldr; Please fold in.
> > >
> > Sure will fold.
> > 
> > Modulo an exception or two, IGT tests only require debugfs (write perm
> > for i915) and user in the video group (to open the node).
> > That's based on a quick look/test.
> > 
> > Since few distributions don't run the display server and/or compositor
> > as root, ideally IGT will get root-free at some point.
> > Ake IGT testing will be closer to real world use-cases.
> 
> IGT isnt like piglit, most of the tests check for corner cases where you
> really can't have anything else touching the display (e.g. anything kms).
> Being root is the easiest way to check for that (in debugfs, if there's
> any other drm client the igt runner complains&quits). I guess there's some
> igts which can be run like piglits, but those are the rare exceptions.
> 
> Now the entire "run compositor as non-root" business is a good point. We
> do have some igts that drop CAP_SYS_ADMIN (see igt_drop_root()), but none
> that make sure this holds for kms/core functionality. It's just a few
> i915-gem tests. Would be a good gap to fill though. I think for the
> non-rendernode EACCESS test we'll need that (since root can do whatever).
> 
> > > > @@ -1,5 +1,6 @@
> > > >  test_progs = [
> > > >       'core_auth',
> > > > +     'core_unauth_vs_render',
> > > >       'core_get_client_auth',
> > >
> > > I think it would make sense to put all the auth tests into core_auth,
> > > together with core_get_client_auth. That way we can also reuse the
> > > check_auth function without copypasting it.
> > >
> > Considering my luck, I'm weary that combining things will open another
> > can of worms.
> > Do you mind if I keep the test separate, so that the kernel fix isn't blocked?
> > 
> > For example, using drm_open_driver_master (as seen in core_auth) for
> > the unauth_vs_render test did not go well.
> > Other interesting issues include, igt_fork and igt_skip* (implicit
> > hidden in the drm_open_driver maze) conflicts.
> 
> Hm, should work. I just wanted to avoid copypasteing the check_auth
> function. You need to be somewhat careful with merging them though, needs
> an igt_subtest_group. I can type the prep patch. Or follow up&rebase once
> this has landed, if you prefer.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

On Wed, 23 Jan 2019 at 11:33, Daniel Vetter <daniel@ffwll.ch> wrote:

> > > How do you envision the card* to renderD* helper? Should I go with the
> > > minor(rdev) & 0x80 hack, use the libdrm helpers or something else?
> >
> > I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
> > :-/
> >
Fwiw: The libdrm helpers use something else have been around for ages.
Mesa uses them not sure for others :-(

That said, I could remove all the crazy libdrm code and use
minor(rdev) + 0x80. Are you OK with that?

> > > A simple drm_open_driver() opens /dev/dri/card0 twice, when using the
> > > drm_open_driver_master we end up with an extra open().
> > > The kernel drm_getclient() was hollowed-out severely (commit
> > > 719524df4a2e48fa7ca3ad1697fd9a7f85ec8ad3), so I'm not sure if/how
> > > reliably we can use that.
> >
> > I wrote that igt to validate the hollowed-out getclient, and it's used by
> > libva (somehow all these hacks are used by libva) to check whether it's
> > authenticated. So should be accurate I hope.
> >
Code works fine, but have butchered my earlier testing. Sorry about the noise.

> > I guess if you want to check for -EACCESS make a 2nd test (or merge them)
> > which makes sure that if we have a non-render node, then we get an
> > EACCESS. Would mean a quick testrun on a display-only drm driver (those
> > are all non-rendernode) to make sure it works. And would be an even better
> > test I think.
This 2nd test is exactly what I wrote with this patch, is it not?
Admittedly it's missing the !DRM_CAP_PRIME case, which I will fix for
v2.

If we want to check that FD2HANDLE returns EBADF for invalid fd (-1 or
otherwise) - sure, but that's unrelated to the goal here.

>
> I just realized that as soon as you open a file as root you're
> authenticated, so this tests nothing. I think we need a new
> drm_open_any_nonroot(), which drops CAP_SYS_ADMIN (while otherwise
> staying as root, can't open the mkdev otherwise I think) and then restores
> it. Or something like this. Tbh not quite sure how to make this happen
> exactly.
>
> Noticed while I was trying to create a new check_auth() subtest for
> non-root not being authenticated from the start, and it didn't fail like I
> thought it should :-)
>
> So a simple
>
>         drm_open_any();
>         igt_drop_root();
>
> Doesn't cut it unfortunately :-/

Is that an assumption or you've tested the patch?

Just ran the patch over a dozen times [today alone] checking via
check_auth() and igt_debugfs_dump().
Admittedly a $chmod -R +rx /sys/kernel/debug was needed.

Did you miss the drm_open_any() call _after_ igt_drop_root() or I'm
really lucky somehow?

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Wed, Jan 23, 2019 at 03:55:39PM +0000, Emil Velikov wrote:
> On Wed, 23 Jan 2019 at 11:33, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > > > How do you envision the card* to renderD* helper? Should I go with the
> > > > minor(rdev) & 0x80 hack, use the libdrm helpers or something else?
> > >
> > > I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
> > > :-/
> > >
> Fwiw: The libdrm helpers use something else have been around for ages.
> Mesa uses them not sure for others :-(
> 
> That said, I could remove all the crazy libdrm code and use
> minor(rdev) + 0x80. Are you OK with that?

I think cleaning up the igt drm_open hilarity is a different project ...
minor+0x80 has my ack.
> 
> > > > A simple drm_open_driver() opens /dev/dri/card0 twice, when using the
> > > > drm_open_driver_master we end up with an extra open().
> > > > The kernel drm_getclient() was hollowed-out severely (commit
> > > > 719524df4a2e48fa7ca3ad1697fd9a7f85ec8ad3), so I'm not sure if/how
> > > > reliably we can use that.
> > >
> > > I wrote that igt to validate the hollowed-out getclient, and it's used by
> > > libva (somehow all these hacks are used by libva) to check whether it's
> > > authenticated. So should be accurate I hope.
> > >
> Code works fine, but have butchered my earlier testing. Sorry about the noise.
> 
> > > I guess if you want to check for -EACCESS make a 2nd test (or merge them)
> > > which makes sure that if we have a non-render node, then we get an
> > > EACCESS. Would mean a quick testrun on a display-only drm driver (those
> > > are all non-rendernode) to make sure it works. And would be an even better
> > > test I think.
> This 2nd test is exactly what I wrote with this patch, is it not?
> Admittedly it's missing the !DRM_CAP_PRIME case, which I will fix for
> v2.

You check for != EACCESS, this idea is to 1) make sure we have a
!DRIVER_RENDER driver and 2) check that we still get an EACCESS (like on
old kernels). So different test. I think ...


> If we want to check that FD2HANDLE returns EBADF for invalid fd (-1 or
> otherwise) - sure, but that's unrelated to the goal here.

The idea is to check for something that happens only once the drm_ioctl()
core function actually calls ioctl->func. Just to make the test more
robust against changes in drm_ioctl() - I always aim for a know outcome
instead of checking for errno != ESOMETHING. Checking for a specific to
happen is a stronger test than checking for a specific thing to not happen
(since that leaves the door open for unrelated breakage with the same
result).

> > I just realized that as soon as you open a file as root you're
> > authenticated, so this tests nothing. I think we need a new
> > drm_open_any_nonroot(), which drops CAP_SYS_ADMIN (while otherwise
> > staying as root, can't open the mkdev otherwise I think) and then restores
> > it. Or something like this. Tbh not quite sure how to make this happen
> > exactly.
> >
> > Noticed while I was trying to create a new check_auth() subtest for
> > non-root not being authenticated from the start, and it didn't fail like I
> > thought it should :-)
> >
> > So a simple
> >
> >         drm_open_any();
> >         igt_drop_root();
> >
> > Doesn't cut it unfortunately :-/
> 
> Is that an assumption or you've tested the patch?
> 
> Just ran the patch over a dozen times [today alone] checking via
> check_auth() and igt_debugfs_dump().
> Admittedly a $chmod -R +rx /sys/kernel/debug was needed.
> 
> Did you miss the drm_open_any() call _after_ igt_drop_root() or I'm
> really lucky somehow?

If you chmod and do igt_drop_root(); drm_open_any(); it'll work. The
problem is that the kernel sets drm_file->authenticated at open() time, so
dropping priviledges later on changes nothing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

On Wed, 23 Jan 2019 at 16:44, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Jan 23, 2019 at 03:55:39PM +0000, Emil Velikov wrote:
> > On Wed, 23 Jan 2019 at 11:33, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > > > How do you envision the card* to renderD* helper? Should I go with the
> > > > > minor(rdev) & 0x80 hack, use the libdrm helpers or something else?
> > > >
> > > > I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
> > > > :-/
> > > >
> > Fwiw: The libdrm helpers use something else have been around for ages.
> > Mesa uses them not sure for others :-(
> >
> > That said, I could remove all the crazy libdrm code and use
> > minor(rdev) + 0x80. Are you OK with that?
>
> I think cleaning up the igt drm_open hilarity is a different project ...
> minor+0x80 has my ack.

I mentioned libdrm your reply says igt drm_open. Slightly confused,
but it's not relevant so meh.

> > > > I guess if you want to check for -EACCESS make a 2nd test (or merge them)
> > > > which makes sure that if we have a non-render node, then we get an
> > > > EACCESS. Would mean a quick testrun on a display-only drm driver (those
> > > > are all non-rendernode) to make sure it works. And would be an even better
> > > > test I think.
> > This 2nd test is exactly what I wrote with this patch, is it not?
> > Admittedly it's missing the !DRM_CAP_PRIME case, which I will fix for
> > v2.
>
> You check for != EACCESS, this idea is to 1) make sure we have a
> !DRIVER_RENDER driver and 2) check that we still get an EACCESS (like on
> old kernels). So different test. I think ...
>
I fear we might be talking past each other. What I'm proposing is:

/*
 * Updated kernels allow render capable, unauthenticated
 * master to issue DRM_AUTH ioctls (like the above), as long as
 * they are annotated as DRM_RENDER_ALLOW.
 *
 * Otherwise we return EACCES.
 *
 * Here we are not interested in the specific validation done by
 * FD2HANDLE ioctl itself, but only about EACCES being thrown by
 * core DRM
 */
if (getcap(DRM_CAP_PRIME)) // has PRIME
       igt_assert(errno != EACCES);
else
       igt_assert(errno == EACCES);

This way we check exactly for what's expected.

>
> > If we want to check that FD2HANDLE returns EBADF for invalid fd (-1 or
> > otherwise) - sure, but that's unrelated to the goal here.
>
> The idea is to check for something that happens only once the drm_ioctl()
> core function actually calls ioctl->func. Just to make the test more
> robust against changes in drm_ioctl() - I always aim for a know outcome
> instead of checking for errno != ESOMETHING. Checking for a specific to
> happen is a stronger test than checking for a specific thing to not happen
> (since that leaves the door open for unrelated breakage with the same
> result).
>
Cannot agree more about robust ioctls checking. If we want to validate
that FD2HANDLE of -1 returns EBADF that should be in some prime_*
test, right?


> If you chmod and do igt_drop_root(); drm_open_any(); it'll work.

Right, the test does igt_drop_root(); drm_open_any();

It's upto the machine maintainer to chmod or equivalent. Although if
you prefer I could add the chmod to the test itself?
It feels very nasty and I'm not sure if it will be possible to undo.

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Petri Latvala]

On Wed, Jan 23, 2019 at 08:01:45PM +0000, Emil Velikov wrote:
> It's upto the machine maintainer to chmod or equivalent. Although if
> you prefer I could add the chmod to the test itself?
> It feels very nasty and I'm not sure if it will be possible to undo.

To feel less nasty, consider creating a mount namespace in the test
and mounting debugfs with mode=whatyouwant, before dropping
root.


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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Thu, Jan 24, 2019 at 10:41:10AM +0200, Petri Latvala wrote:
> On Wed, Jan 23, 2019 at 08:01:45PM +0000, Emil Velikov wrote:
> > It's upto the machine maintainer to chmod or equivalent. Although if
> > you prefer I could add the chmod to the test itself?
> > It feels very nasty and I'm not sure if it will be possible to undo.
> 
> To feel less nasty, consider creating a mount namespace in the test
> and mounting debugfs with mode=whatyouwant, before dropping
> root.

Yeah that sounds really clean, and since we're root we can do it :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Wed, Jan 23, 2019 at 08:01:45PM +0000, Emil Velikov wrote:
> On Wed, 23 Jan 2019 at 16:44, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Jan 23, 2019 at 03:55:39PM +0000, Emil Velikov wrote:
> > > On Wed, 23 Jan 2019 at 11:33, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > > > How do you envision the card* to renderD* helper? Should I go with the
> > > > > > minor(rdev) & 0x80 hack, use the libdrm helpers or something else?
> > > > >
> > > > > I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
> > > > > :-/
> > > > >
> > > Fwiw: The libdrm helpers use something else have been around for ages.
> > > Mesa uses them not sure for others :-(
> > >
> > > That said, I could remove all the crazy libdrm code and use
> > > minor(rdev) + 0x80. Are you OK with that?
> >
> > I think cleaning up the igt drm_open hilarity is a different project ...
> > minor+0x80 has my ack.
> 
> I mentioned libdrm your reply says igt drm_open. Slightly confused,
> but it's not relevant so meh.
> 
> > > > > I guess if you want to check for -EACCESS make a 2nd test (or merge them)
> > > > > which makes sure that if we have a non-render node, then we get an
> > > > > EACCESS. Would mean a quick testrun on a display-only drm driver (those
> > > > > are all non-rendernode) to make sure it works. And would be an even better
> > > > > test I think.
> > > This 2nd test is exactly what I wrote with this patch, is it not?
> > > Admittedly it's missing the !DRM_CAP_PRIME case, which I will fix for
> > > v2.
> >
> > You check for != EACCESS, this idea is to 1) make sure we have a
> > !DRIVER_RENDER driver and 2) check that we still get an EACCESS (like on
> > old kernels). So different test. I think ...
> >
> I fear we might be talking past each other. What I'm proposing is:
> 
> /*
>  * Updated kernels allow render capable, unauthenticated
>  * master to issue DRM_AUTH ioctls (like the above), as long as
>  * they are annotated as DRM_RENDER_ALLOW.
>  *
>  * Otherwise we return EACCES.
>  *
>  * Here we are not interested in the specific validation done by
>  * FD2HANDLE ioctl itself, but only about EACCES being thrown by
>  * core DRM
>  */
> if (getcap(DRM_CAP_PRIME)) // has PRIME
>        igt_assert(errno != EACCES);
> else
>        igt_assert(errno == EACCES);
> 
> This way we check exactly for what's expected.

The problem why I don't like errno != EACCESS checks is aliasing of
failures. Here's the general recipe:

1. run ioctl in a very specific way.

2. make sure the outcome is _exaclty_ what you suspect. Best case this
means success, slightly worse is a specific errno. Really bad is EINVAL,
because we have way too much code in the kernel that throws EINVAL.

3. Change one thing and one thing only in your ioctl command (could be an
argument, could be environment like CAP_SYS_ADMIN), re-run it.

4. Make sure it fails in a very specific fashion.

If you instead have an unspecific testcase that just checks for errno !=
ESOMETHING, then if you're unlucky, and there's some unrelated breakage,
then your test still passes, but it stops checking what you think it
shouold check.

E.g. in this case here if you break both the access check _and_ something
else in fd2handle, then the result might still be errno != ESOMETHING, and
so you won't spot the issue.

Of course having perfect test coverage for everything else would prevent
that, but engineering reality is that you'll never have perfect enough
test coverage, and even after 5+ years of working hard, igt definitely
doesn't have perfect test coverage. Hence imo better to write more robust
and more specific tests.

Being strict in what you emit and forgiving in what you accept is not a
great principle for testcases (or kernel uapi design in general, same
reasons why I don't like the new drmIsMaster idea).
-Daniel

> 
> >
> > > If we want to check that FD2HANDLE returns EBADF for invalid fd (-1 or
> > > otherwise) - sure, but that's unrelated to the goal here.
> >
> > The idea is to check for something that happens only once the drm_ioctl()
> > core function actually calls ioctl->func. Just to make the test more
> > robust against changes in drm_ioctl() - I always aim for a know outcome
> > instead of checking for errno != ESOMETHING. Checking for a specific to
> > happen is a stronger test than checking for a specific thing to not happen
> > (since that leaves the door open for unrelated breakage with the same
> > result).
> >
> Cannot agree more about robust ioctls checking. If we want to validate
> that FD2HANDLE of -1 returns EBADF that should be in some prime_*
> test, right?
> 
> 
> > If you chmod and do igt_drop_root(); drm_open_any(); it'll work.
> 
> Right, the test does igt_drop_root(); drm_open_any();
> 
> It's upto the machine maintainer to chmod or equivalent. Although if
> you prefer I could add the chmod to the test itself?
> It feels very nasty and I'm not sure if it will be possible to undo.
> 
> Thanks
> Emil

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Emil Velikov]

On Thu, 24 Jan 2019 at 11:04, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Jan 23, 2019 at 08:01:45PM +0000, Emil Velikov wrote:
> > On Wed, 23 Jan 2019 at 16:44, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Wed, Jan 23, 2019 at 03:55:39PM +0000, Emil Velikov wrote:
> > > > On Wed, 23 Jan 2019 at 11:33, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > > > > How do you envision the card* to renderD* helper? Should I go with the
> > > > > > > minor(rdev) & 0x80 hack, use the libdrm helpers or something else?
> > > > > >
> > > > > > I think it's minor(rdev) + 0x80, but yeah I think that's official uapi now
> > > > > > :-/
> > > > > >
> > > > Fwiw: The libdrm helpers use something else have been around for ages.
> > > > Mesa uses them not sure for others :-(
> > > >
> > > > That said, I could remove all the crazy libdrm code and use
> > > > minor(rdev) + 0x80. Are you OK with that?
> > >
> > > I think cleaning up the igt drm_open hilarity is a different project ...
> > > minor+0x80 has my ack.
> >
> > I mentioned libdrm your reply says igt drm_open. Slightly confused,
> > but it's not relevant so meh.
> >
> > > > > > I guess if you want to check for -EACCESS make a 2nd test (or merge them)
> > > > > > which makes sure that if we have a non-render node, then we get an
> > > > > > EACCESS. Would mean a quick testrun on a display-only drm driver (those
> > > > > > are all non-rendernode) to make sure it works. And would be an even better
> > > > > > test I think.
> > > > This 2nd test is exactly what I wrote with this patch, is it not?
> > > > Admittedly it's missing the !DRM_CAP_PRIME case, which I will fix for
> > > > v2.
> > >
> > > You check for != EACCESS, this idea is to 1) make sure we have a
> > > !DRIVER_RENDER driver and 2) check that we still get an EACCESS (like on
> > > old kernels). So different test. I think ...
> > >
> > I fear we might be talking past each other. What I'm proposing is:
> >
> > /*
> >  * Updated kernels allow render capable, unauthenticated
> >  * master to issue DRM_AUTH ioctls (like the above), as long as
> >  * they are annotated as DRM_RENDER_ALLOW.
> >  *
> >  * Otherwise we return EACCES.
> >  *
> >  * Here we are not interested in the specific validation done by
> >  * FD2HANDLE ioctl itself, but only about EACCES being thrown by
> >  * core DRM
> >  */
> > if (getcap(DRM_CAP_PRIME)) // has PRIME
> >        igt_assert(errno != EACCES);
> > else
> >        igt_assert(errno == EACCES);
> >
> > This way we check exactly for what's expected.
>
> The problem why I don't like errno != EACCESS checks is aliasing of
> failures.

Fully agree in the general case, although here:
 - we want to validate the drm_permit() handling
 - there is no specific ioctl that only exercises the above function,
so we select a random one (FD2HANDLE)
 - the function itself returns EACCES and we don't care about the
FD2HANDLE specifics

True, some FD2HANDLE failures may be aliased by the != EACCES check.
Yet those should really be in a prime test.

How about i write a simple EBADF test under tests/prime*.c and keep
the EACCES in tests/core_auth.c (a)?
Alternatively, I could keep the EBADF in tests/core_auth.c although
I'll add a chunky comment that, in practise, we only care about EACCES
(b).

Which one do you prefer?

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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Petri Latvala]

On Wed, Jan 23, 2019 at 12:18:14PM +0100, Daniel Vetter wrote:
> Being root is the easiest way to check for that (in debugfs, if there's
> any other drm client the igt runner complains&quits).


For the record, that was the case when piglit was our test
runner. This is not (yet) implemented with igt_runner.



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

Re: [igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Daniel Vetter]

On Wed, Jan 23, 2019 at 01:42:47PM +0200, Petri Latvala wrote:
> On Wed, Jan 23, 2019 at 12:18:14PM +0100, Daniel Vetter wrote:
> > Being root is the easiest way to check for that (in debugfs, if there's
> > any other drm client the igt runner complains&quits).
> 
> 
> For the record, that was the case when piglit was our test
> runner. This is not (yet) implemented with igt_runner.

Would be good to have those sanity checks again, since lots of newcomers
to igt get this wrong and assume you can run it like piglit. Which isn't
the case at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev