[PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Mon, May 27, 2019 at 3:26 PM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Mon, May 27, 2019 at 01:52:05PM +0100, Emil Velikov wrote:
> > On 2019/05/27, Koenig, Christian wrote:
> > > Am 27.05.19 um 14:05 schrieb Emil Velikov:
> > > > On 2019/05/27, Koenig, Christian wrote:
> > > >> Am 27.05.19 um 10:17 schrieb Emil Velikov:
> > > >>> From: Emil Velikov <emil.velikov at collabora.com>
> > > >>>
> > > >>> Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> > > >>> render node. A seemingly deliberate design decision.
> > > >>>
> > > >>> Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> > > >>> yet not all userspace checks if it's authenticated, but instead uses
> > > >>> uncommon assumptions.
> > > >>>
> > > >>> After days of digging through git log and testing, only a eingle (ab)use
> > > >>> was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> > > >>> assuming that failure implies lack of authentication.
> > > >>>
> > > >>> Affected versions are:
> > > >>>    - the whole 18.2.x series, which is EOL
> > > >>>    - the whole 18.3.x series, which is EOL
> > > >>>    - the 19.0.x series, prior to 19.0.4
> > > >> Well you could have saved your time, cause this is still a NAK.
> > > >>
> > > > Did I mention that I've spent quite a bit of time in AMDVLK? Even fixed
> > > > a bug while I was there :-)
> > >
> > > Yeah, thanks for doing this.
> > >
> > > But we have done so much stuff with customers which can't be audited
> > > this way, that I still have a really bad feeling about this :/
> > >
> > Fair point, I've already thought about those.
> >
> > Example A:  customer X, using closed source/private software Y.
> > Ioctls A, B and C require the old behaviour - simply add FORCE_AUTH to
> > the A B C and carry on happily.
>
> Hm, if the entire concern here is all the bazillion different versions of
> blobs shipped in the past few years, why can't we just have a revert of
> this in the amdgpu DKMS? Not like one more patch among the hundres will
> matter. I'd suspect that the overlap of people wanting to run the blobs
> and people who don't run the DKMS or similar is roughly 0. Always been the
> case here at Intel at least.
>
> If there's other stuff that we need to audit (like rocm or whatever), then
> we should look into those ofc.

Also note that Emil's patch actually doesn't break anything, since
default y. So you don't even need the revert (except maybe in 10 years
or so when we throw that option out).
-Daniel


> > Example B: the team, as a whole thinks that this will be problematic for
> > customer X - add FORCE_AUTH to all ioctls and carry on.
> >
> > I do see and understand why anyone can be hesitant about the series.
> >
> > IMHO the above examples, illustrate quite reasonable compromise between
> > open-source and closed-source/private solutions.
> >
> > Don't you agree?
> >
> > > >> For the record: I strongly think that we don't want to expose any render
> > > >> functionality on the primary node.
> > > >>
> > > >> To even go a step further I would say that at least on AMD hardware we
> > > >> should completely disable DRI2 for one of the next generations.
> > > >>
> > > > It's doable and overall pretty neat idea.
> > > >
> > > > There is a consern that this approach may cause far more regressions
> > > > that this series. We are talking about years worth of Mesa drivers (et
> > > > al) that depend on render functionality exposed via the primary node.
> > >
> > > Yeah, that's I'm perfectly aware of. It's the reason why I said we
> > > should only do it for new hardware generations.
> > >
> > > But at some point I think we should do this and get rid of
> > > authentication/flink/DRI2/....
> > >
> > Fwiw I share a similar sentiment. If you've looked through the series,
> > this is effectively step 1, towards nuking DRM_AUTH :-)
> >
> > > > I'm OK with writing the patches, but it'll be up-to the AMDGPU team to
> > > > follow-up with any regressions. Are you ok with that?
> > >
> > > As long as we have a check like adev->family_id >
> > > WHATEVER_IS_THE_CURRENT_LATEST_UPSTREAM_HW, I think we are fine with that.
> > >
> > > If I understood Michel correctly xf86-video-amdgpu should work, but
> > > modeset might break and needs a patch.
> > >
> > Unless I have concrete WHATEVER_IS_THE... I cannot do much here :-(
> >
> >
> > Thanks
> > Emil
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch