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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
