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

Submitted by Emil Velikov on Feb. 6, 2019, 1:18 p.m.

Details

Message ID 20190206131828.17018-1-emil.l.velikov@gmail.com
State New
Series "tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling"
Headers show

Commit Message

Emil Velikov Feb. 6, 2019, 1:18 p.m.
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

Patch hide | download patch | download mbox

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',

Comments

Petri Latvala Feb. 7, 2019, 8:59 a.m.
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*.
Emil Velikov Feb. 7, 2019, noon
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
Petri Latvala Feb. 7, 2019, 12:08 p.m.
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.
Daniel Vetter Feb. 7, 2019, 2:17 p.m.
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
Emil Velikov Feb. 7, 2019, 5:08 p.m.
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
Daniel Vetter Feb. 7, 2019, 5:47 p.m.
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