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

[Petri Latvala <petri.latvala@intel.com>]

On Wed, Feb 19, 2020 at 02:24:45PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> This test adds three distinct subtests:
>  - drop/set master as root
>  - drop/set master as non-root
>  - drop/set master for a shared fd
> 
> Currently the second subtest will fail, with kernel patch to address
> that has been submitted.
> 
> v2: Add to the autotools build
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  tests/Makefile.sources |   1 +
>  tests/core_setmaster.c | 182 +++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build      |   1 +
>  3 files changed, 184 insertions(+)
>  create mode 100644 tests/core_setmaster.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index b87d6333..5da36a91 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -18,6 +18,7 @@ TESTS_progs = \
>  	core_getclient \
>  	core_getstats \
>  	core_getversion \
> +	core_setmaster \
>  	core_setmaster_vs_auth \
>  	debugfs_test \
>  	dmabuf \
> diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c
> new file mode 100644
> index 00000000..2447c077
> --- /dev/null
> +++ b/tests/core_setmaster.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright © 2020 Collabora, Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Emil Velikov <emil.l.velikov@gmail.com>
> + *
> + */
> +
> +/*
> + * Testcase: Check that drop/setMaster behaves correctly wrt root/user access
> + *
> + * Test checks if the ioctls succeed or fail, depending if the applications was
> + * run with root, user privileges or if we have separate privileged arbitrator.
> + */
> +
> +#include "igt.h"
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +
> +IGT_TEST_DESCRIPTION("Check that drop/setMaster behaves correctly wrt root/user"
> +		     " access");
> +
> +static bool is_master(int fd)
> +{
> +	/* FIXME: replace with drmIsMaster once we bumped libdrm version */
> +	return drmAuthMagic(fd, 0) != -EACCES;
> +}
> +
> +static void check_drop_set(void)
> +{
> +	int master;
> +
> +	master = __drm_open_driver(DRIVER_ANY);
> +
> +	/* Double-check if open has failed */
> +	igt_assert_neq(master, -1);

Just use drm_open_driver(). For sure you don't want to produce a
'FAIL' when running on a system without GPU drivers. 'SKIP' is correct
for that case.

Same goes for the other __drm_open_driver() call.

> +
> +	/* At this point we're master capable due to:
> +	 * - being root - always
> +	 * - normal user - as the only drm only drm client (on this VT)
> +	 */
> +	igt_assert_eq(is_master(master), true);
> +
> +	/* If we have SYS_CAP_ADMIN we're in the textbook best-case scenario.
> +	 *
> +	 * Otherwise newer kernels allow the application to drop/revoke its
> +	 * master capability and request it again later.
> +	 *
> +	 * In this case, we address two types of issues:
> +	 * - the application no longer need suid-root (or equivalent) which
> +	 * was otherwise required _solely_ for these two ioctls
> +	 * - plenty of applications ignore (or discard) the result of the
> +	 * calls all together.
> +	 */
> +	igt_assert_eq(drmDropMaster(master), 0);
> +	igt_assert_eq(drmSetMaster(master), 0);
> +
> +	close(master);
> +}
> +
> +static unsigned tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
> +{
> +	char path[256];
> +	struct stat st;
> +	unsigned i;
> +
> +	for (i = 0; i < max_perm; i++) {
> +		snprintf(path, sizeof(path), "/dev/dri/card%u", i);
> +
> +		/* Existing userspace assumes there's no gaps, do the same. */
> +		if (stat(path, &st) != 0)
> +			break;
> +
> +		if (save) {
> +			/* Save and toggle */
> +			saved_perm[i] = st.st_mode & (S_IROTH | S_IWOTH);
> +			st.st_mode |= S_IROTH | S_IWOTH;
> +		} else {
> +			/* Clear and restore */
> +			st.st_mode &= ~(S_IROTH | S_IWOTH);
> +			st.st_mode |= saved_perm[i];
> +		}
> +
> +		/* In the extremely unlikely case of this failing, there't not
> +		 * much we can do.

We can igt_require that it works though. chmod failing means we get a
false 'FAIL' from the test.

Restoring failure leaves the system to a state that is going to scare
people with the extra perms...


> +		 */
> +		chmod(path, st.st_mode);
> +	}
> +	return i;
> +}
> +
> +
> +igt_main
> +{
> +	igt_subtest("master-drop-set-root") {
> +		check_drop_set();
> +	}
> +
> +
> +	igt_subtest("master-drop-set-user") {
> +		uint8_t saved_perm[255];
> +		unsigned num;
> +
> +		/* Upon dropping root we end up as random user, which
> +		 * a) is not in the video group, and
> +		 * b) lacks logind ACL, thus
> +		 * any open() fill fail.
> +		 *
> +		 * As such, save the state of original other rw permissions
> +		 * and toggle them on.
> +		 */
> +		num = tweak_perm(saved_perm, ARRAY_SIZE(saved_perm), true);
> +
> +		igt_fork(child, 1) {
> +			igt_drop_root();
> +			check_drop_set();
> +		}
> +		igt_waitchildren();
> +
> +		/* Restore the orignal permissions */
> +		tweak_perm(saved_perm, num, false);

If the test fails we never restore the permissions with this flow. I
suppose this would be the best (but still ugly) way to make that
happen:

igt_fixture {
   tweak_perm(...)
}

igt_subtest("master-drop-set-user") {
...
}

igt_fixture {
  tweak_perm(saved);
}

Wrapping all that in an igt_subtest_group not required but could be
used to communicate to human readers that this is the subtest these
two fixtures are for.



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