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

Fri Jan 18 15:58:44 UTC 2019

On Mon, Jan 14, 2019 at 08:39:37AM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at 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 at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch