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

[Emil Velikov]

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.

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.

> > +     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?


Thanks
Emil