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

[Daniel Vetter]

On Wed, Jan 23, 2019 at 03:55:39PM +0000, Emil Velikov wrote:
> On Wed, 23 Jan 2019 at 11:33, Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > > > 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
> > > :-/
> > >
> Fwiw: The libdrm helpers use something else have been around for ages.
> Mesa uses them not sure for others :-(
> 
> That said, I could remove all the crazy libdrm code and use
> minor(rdev) + 0x80. Are you OK with that?

I think cleaning up the igt drm_open hilarity is a different project ...
minor+0x80 has my ack.
> 
> > > > 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.
> > >
> Code works fine, but have butchered my earlier testing. Sorry about the noise.
> 
> > > 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.
> This 2nd test is exactly what I wrote with this patch, is it not?
> Admittedly it's missing the !DRM_CAP_PRIME case, which I will fix for
> v2.

You check for != EACCESS, this idea is to 1) make sure we have a
!DRIVER_RENDER driver and 2) check that we still get an EACCESS (like on
old kernels). So different test. I think ...


> If we want to check that FD2HANDLE returns EBADF for invalid fd (-1 or
> otherwise) - sure, but that's unrelated to the goal here.

The idea is to check for something that happens only once the drm_ioctl()
core function actually calls ioctl->func. Just to make the test more
robust against changes in drm_ioctl() - I always aim for a know outcome
instead of checking for errno != ESOMETHING. Checking for a specific to
happen is a stronger test than checking for a specific thing to not happen
(since that leaves the door open for unrelated breakage with the same
result).

> > 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 :-/
> 
> Is that an assumption or you've tested the patch?
> 
> Just ran the patch over a dozen times [today alone] checking via
> check_auth() and igt_debugfs_dump().
> Admittedly a $chmod -R +rx /sys/kernel/debug was needed.
> 
> Did you miss the drm_open_any() call _after_ igt_drop_root() or I'm
> really lucky somehow?

If you chmod and do igt_drop_root(); drm_open_any(); it'll work. The
problem is that the kernel sets drm_file->authenticated at open() time, so
dropping priviledges later on changes nothing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch