[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:18:14 UTC 2019
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.
>
> >
> > > +}
> > > +
> > > +/*
> > > + * 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
More information about the igt-dev
mailing list