Re: [PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock fixes.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 28, 2015 at 09:28:51AM +0000, Antoine, Peter wrote:
> On Mon, 2015-04-27 at 16:33 +0100, Chris Wilson wrote:
> > On Mon, Apr 27, 2015 at 04:24:37PM +0100, Thomas Wood wrote:
> > > On 23 April 2015 at 15:07, Peter Antoine <peter.antoine@intel.com> wrote:
> > > > There are several issues with the hardware locks functions that stretch
> > > > from kernel crashes to priority escalations. This new test will test the
> > > > the fixes for these features.
> > > >
> > > > This test will cause a driver/kernel crash on un-patched kernels, the
> > > > following patches should be applied to stop the crashes:
> > > >
> > > >   drm: Kernel Crash in drm_unlock
> > > >   drm: Fixes unsafe deference in locks.
> > > >
> > > > Issue: VIZ-5485
> > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > > > ---
> > > >  lib/ioctl_wrappers.c   |  19 +++++
> > > >  lib/ioctl_wrappers.h   |   1 +
> > > >  tests/Makefile.sources |   1 +
> > > >  tests/drm_hw_lock.c    | 207 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 228 insertions(+)
> > > >  create mode 100644 tests/drm_hw_lock.c
> > > >
> > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > > index 000d394..ad8b3d3 100644
> > > > --- a/lib/ioctl_wrappers.c
> > > > +++ b/lib/ioctl_wrappers.c
> > > > @@ -964,6 +964,25 @@ bool gem_has_bsd2(int fd)
> > > >  {
> > > >         return gem_has_enable_ring(fd,LOCAL_I915_PARAM_HAS_BSD2);
> > > >  }
> > > > +#define I915_PARAM_HAS_LEGACY_CONTEXT   35
> > > 
> > > 
> > > Please add some API documentation for this new function here.
> > > 
> > > > +bool drm_has_legacy_context(int fd)
> > > > +{
> > > > +       int tmp = 0;
> > > > +       drm_i915_getparam_t gp;
> > > > +
> > > > +       memset(&gp, 0, sizeof(gp));
> > > > +       gp.value = &tmp;
> > > > +       gp.param = I915_PARAM_HAS_LEGACY_CONTEXT;
> > > > +
> > > > +       /*
> > > > +        * if legacy context param is not supported, then it's old and we
> > > > +        * can assume that the HW_LOCKS are supported.
> > > > +        */
> > > > +       if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0)
> > > > +               return true;
> > 
> > Would not a simpler test be to try and legally acquire a hwlock?
> > If it fails, hwlocks are not supported. No need for a PARAM.
> > -Chris
> > 
> 
> The test relies on the hw_lock not being created (f the HW-LOCKs are
> supported). If I grab, then delete the lock I might find more problems
> with this code. :)
> 
> As you have to be root to create and delete the hwlock the security
> issues are minimal as you have root already you don't really need to use
> these ioctls to harm the system. So as the exposure is minimal, any more
> fixes on code that is legacy and being turned off seems to be a bit of a
> time waste.
> 
> Also, the PARAM should give legitimate code the opportunity to avoid the
> problems but avoiding these features completely.

root != kernel, at least in secure boot enviroments. Which means not even
root is allowed to crash/exploit the kernel ... Yes there's a pile of
.config options and stuff you need to set correctly to fully lock out root
from the kernel, but in general drivers really shouldn't have their own
root2kernel backdoors.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx