[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.

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

[igt-dev] ✓ Fi.CI.BAT: success for tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5413 -> IGTPW_2230
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55149/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2230 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          FAIL [fdo#103167] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (38 -> 41)
------------------------------

  Additional (6): fi-hsw-peppy fi-skl-6260u fi-ilk-650 fi-snb-2520m fi-skl-iommu fi-skl-6700k2 
  Missing    (3): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


Build changes
-------------

    * IGT: IGT_4765 -> IGTPW_2230

  CI_DRM_5413: 9751ba94138d1dfbf427f84c05984d9657ccc356 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2230: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2230/
  IGT_4765: fde4dce431bf324939a982017169214e0fa00d4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@core_unauth_vs_render@unauth-vs-render

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2230/
_______________________________________________
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

[igt-dev] ✓ Fi.CI.IGT: success for tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5413_full -> IGTPW_2230_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55149/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2230_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@core_unauth_vs_render@unauth-vs-render}:
    - shard-hsw:          NOTRUN -> FAIL
    - shard-glk:          NOTRUN -> FAIL
    - shard-apl:          NOTRUN -> FAIL
    - shard-snb:          NOTRUN -> FAIL
    - shard-kbl:          NOTRUN -> FAIL

  
Known issues
------------

  Here are the changes found in IGTPW_2230_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-snb:          NOTRUN -> DMESG-WARN [fdo#107956] +1

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-apl:          PASS -> FAIL [fdo#106510] / [fdo#108145] +1
    - shard-glk:          PASS -> FAIL [fdo#108145] +1
    - shard-kbl:          PASS -> FAIL [fdo#107725] / [fdo#108145]

  * igt@kms_color@pipe-b-ctm-max:
    - shard-apl:          PASS -> FAIL [fdo#108147]

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]
    - shard-kbl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +3

  * igt@kms_cursor_crc@cursor-64x64-dpms:
    - shard-kbl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-glk:          PASS -> FAIL [fdo#103232] +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-apl:          PASS -> FAIL [fdo#103167] +5

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
    - shard-kbl:          PASS -> FAIL [fdo#103167] +4

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          PASS -> FAIL [fdo#103167] +6

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-glk:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          PASS -> FAIL [fdo#103166] +6

  * igt@kms_plane_alpha_blend@pipe-a-alpha-transparant-fb:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_universal_plane@universal-plane-pipe-c-functional:
    - shard-glk:          PASS -> FAIL [fdo#103166] +5

  
#### Possible fixes ####

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-glk:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-256x256-sliding:
    - shard-apl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-random:
    - shard-kbl:          FAIL [fdo#103232] -> PASS

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-kbl:          DMESG-WARN [fdo#103313] -> PASS

  * igt@kms_flip@wf_vblank-ts-check:
    - shard-apl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS +5

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-cpu:
    - shard-glk:          FAIL [fdo#103167] -> PASS +3

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-kbl:          FAIL [fdo#103167] -> PASS

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-glk:          FAIL [fdo#108948] -> PASS
    - shard-apl:          FAIL [fdo#108948] -> PASS

  * igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
    - shard-glk:          FAIL [fdo#108145] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
    - shard-kbl:          FAIL [fdo#108145] -> PASS
    - shard-apl:          FAIL [fdo#108145] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-glk:          FAIL [fdo#103166] -> PASS +1
    - shard-kbl:          FAIL [fdo#103166] -> PASS +1

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-apl:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-kbl:          {SKIP} [fdo#109271] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103313]: https://bugs.freedesktop.org/show_bug.cgi?id=103313
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

    * IGT: IGT_4765 -> IGTPW_2230
    * Piglit: piglit_4509 -> None

  CI_DRM_5413: 9751ba94138d1dfbf427f84c05984d9657ccc356 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2230: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2230/
  IGT_4765: fde4dce431bf324939a982017169214e0fa00d4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2230/
_______________________________________________
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