[igt-dev] [PATCH i-g-t] tests/core_unauth_vs_render: new test for the relaxed DRM_AUTH handling

Thu Feb 7 17:47:22 UTC 2019

On Thu, Feb 07, 2019 at 05:08:23PM +0000, Emil Velikov wrote:
> On Thu, 7 Feb 2019 at 14:17, Daniel Vetter <daniel at ffwll.ch> wrote:
> >
> > On Wed, Feb 06, 2019 at 01:18:28PM +0000, Emil Velikov wrote:
> 
> > > +static bool check_auth(int fd)
> > > +{
> > > +     pid_t client_pid;
> > > +     int i, auth, pid, uid;
> > > +     unsigned long magic, iocs;
> > > +     bool is_authenticated = false;
> > > +
> > > +     client_pid = getpid();
> > > +     for (i = 0; !is_authenticated; i++) {
> > > +             if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
> > > +                     break;
> > > +             is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
> > > +     }
> > > +     return is_authenticated;
> > > +}
> >
> > btw the core_auth merger landed, so you can stuff your new subtest in
> > there now.
> >
> Sure will give it a try. If any unrelated issues come up, I'm inclined
> to keep it separate though.
> 
> 
> [snip]
> 
> > > +static void test_unauth_vs_render(int master)
> > > +{
> > > +     int slave;
> > > +     int prime_fd = -1;
> > > +     uint32_t handle;
> > > +
> > > +     /*
> > > +      * The second open() happens without CAP_SYS_ADMIN, thus it will NOT
> > > +      * be authenticated.
> > > +      */
> > > +     igt_info("Openning card node from a non-priv. user.\n");
> > > +     igt_info("On failure, double-check the node permissions\n");
> > > +     /* FIXME: relate to the master given and fix all of IGT */
> > > +     slave = drm_open_driver(DRIVER_ANY);
> > > +
> > > +     igt_require(slave >= 0);
> >
> > igt_require/skip need to be outside of igt_fork. But this one here should
> > be an igt_assert I think, and the testcase needs to somehow make sure that
> > it will succeed. I think the namespace trick is probably the best option,
> > but that means open-coding the clone stuff, or rewriting igt_fork. I think
> > I need to look into that a bit ...
> >
> 
> Let me try and provide a complete picture, instead of my earlier
> fragmented rumbling.
> 
> We drop root permissions before drm_open_driver(), as otherwise the
> client will be authenticated.
> To achieve that, while still preserving the atexit machinery we igt_fork().
> 
> If the dev node is missing permissions, say the user is not in the
> video group, the open() call will fail.
> Thus the igt_skip_on_f() in drm_open_driver() will trigger, which will
> interact badly with igt_fork() as documented.
> 
> I think that fixing the igt_fork() and igt_skip() interaction is a
> good idea, although beyond my current IGT knowledge.
> IMHO the current igt_info() should be indicative enough, until
> something better is available.

Yeah I guess until we have an igt_nonroot_fork block which does some magic
to gauarantee you can still open the device there's not much we can do
here. Huge FIXME comment and call it done, I'd even skip the igt_info.

> Additionally I wonder if we cannot:
> - drop the igt_skip() from drm_open_driver()
> - update the existing tests to consistently use igt_require() just after
>    - some tests use igt_assert(), others igt_require() and some omit
> any fd checking
> 
> It will not fix the underlying issue, yet it will make the tests more
> consistent and easier to read.
> What do you think? I'm ok with providing patches for that.

Yeah there's probably a pile of cargo-culting going on. So the rule of
thumb is that the normal igt functions always work, by bailing out in any
failure case using igt_skip. If you don't have the DRM device that we
need, then we should skip. So all these igt_assert/skips in tests
themselves should probably be thrown out.

For the case where you know what you're doing, there should be __foo
functions, which never bail out automatically. If these don't exist yet,
then we'll need to add it. You're lucky, __drm_open_driver already exists.

> 
> > > +     igt_assert(check_auth(slave) == false);
> > > +
> > > +     /* Issuing the following ioctl will fail, no doubt about it. */
> > > +     igt_assert(drmPrimeFDToHandle(slave, prime_fd, &handle) < 0);
> >
> > Hm, I'd run this first on master and make sure we get -EBADF as errno.
> > Just to make sure the ioctl call we're doing does get through the
> > drm_ioctl() layers.
> >
> > > +
> > > +     /*
> > > +      * 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 - just like FD2HANDLE above.
> > > +      *
> > > +      * Otherwise, errno is set to -EACCES
> > > +      *
> > > +      * Note: We are _not_ interested in the FD2HANDLE specific errno. Those
> > > +      * should be checked other standalone tests.
> > > +      */
> > > +     bool imp = has_prime_import(slave);
> >
> > Hm I think has_prime_import should be an igt_require (and outside of the
> > igt_fork).
> >
> Ack. That's a nice way to handle the igt_skip/igt_fork interaction.
> 
> > > +     bool rend = has_render_node(slave);
> > > +     igt_info("import %d rend %d\n", imp, rend);
> > > +     if (has_prime_import(slave) && has_render_node(slave))
> > > +             igt_assert(errno != EACCES);
> >
> > Still think we should check for the errno we expect here (i.e. EBADF, if
> > we filter out !has_prime_import earlier).
> 
> As alluded in the comment, the aim of this test is to check the
> drm_permit() kernel function.
> Since we don't have a specific IOCTL dedicated for it, we use
> FD2HANDLE as an example.
> 
> The function drm_permit() returns EACCES so the test checks for that.
> 
> 
> I'm more than happy to reword the comment to make this clear and
> obvious. Suggestions are greatly appreciated.
> Alternatively I could use EBADF although clearly comment that we're
> happy with anything != EACCES.
> 
> Would you prefer that?

Checking for EBADF plus explicit comment that we aim for anything !=
ENOACCESS sounds good. Plus also running the same ioctl on the first fd,
opened as root, and making sure we get an EBADF there too. That way if
there's ever a kernel change that moves some of these checks around, which
could break our testcase here, then we'll notice and can adjust the
testcase. Sometimes I even first do the ioctl in a way that should work,
and then change only 1 thing to get the case I want to exercise. That
makes sure we're always testing the right check, no matter in which order
the kernel checks stuff, since that check is the only thing that can
change the ioctl outcome from the previous run. Unfortunately that's not
possible here (importing dma-buf isn't simple), so specific bad outcome is
the next best thing we can do.

tldr; errno == EBADF + comment.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch