[igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling
Daniel Vetter
daniel at ffwll.ch
Wed Jan 23 11:33:29 UTC 2019
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 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?
>
> 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
More information about the igt-dev
mailing list