[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 at ffwll.ch> wrote:
>
> 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 :-/
>
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 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.
>
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