[PATCH 3/3] drm: Reject DRI1 hw lock ioctl functions for kms drivers
On Tue, 2015-06-23 at 11:37 +0200, Daniel Vetter wrote:
> I've done some extensive history digging across libdrm, mesa and
> xf86-video-{intel,nouveau,ati}. The only potential user of this with
> kms drivers I could find was ttmtest, which once used drmGetLock
> still. But that mistake was quickly fixed up. Even the intel xvmc
> library (which otherwise was really good with using dri1 stuff in kms
> mode) managed to never take the hw lock for dri2 (and hence kms).
>
> Hence it should be save to unconditionally disallow this.
>
> Cc: Peter Antoine
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_lock.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index f861361a635e..4924d381b664 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> struct drm_master *master = file_priv->master;
> int ret = 0;
>
> + if (drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> ++file_priv->lock_count;
>
> if (lock->context == DRM_KERNEL_CONTEXT) {
> @@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data,
> struct drm_file *file_
> struct drm_lock *lock = data;
> struct drm_master *master = file_priv->master;
>
> + if (drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> if (lock->context == DRM_KERNEL_CONTEXT) {
> DRM_ERROR("Process %d using kernel context %d\n",
> task_pid_nr(current), lock->context);
Reviewed-by: Peter Antoine
[PATCH 2/3] drm: Convert drm_legacy_ctxbitmap_init to void return type
On Tue, 2015-06-23 at 11:37 +0200, Daniel Vetter wrote:
> It can't fail really.
>
> Also remove the redundant kms check Peter added.
>
> Cc: Peter Antoine
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_context.c | 5 ++---
> drivers/gpu/drm/drm_drv.c | 10 +-
> drivers/gpu/drm/drm_legacy.h | 2 +-
> 3 files changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> index 32958dabd7b0..192a5f9eeb74 100644
> --- a/drivers/gpu/drm/drm_context.c
> +++ b/drivers/gpu/drm/drm_context.c
> @@ -89,14 +89,13 @@ static int drm_legacy_ctxbitmap_next(struct drm_device *
> dev)
> *
> * Initialise the drm_device::ctx_idr
> */
> -int drm_legacy_ctxbitmap_init(struct drm_device * dev)
> +void drm_legacy_ctxbitmap_init(struct drm_device * dev)
> {
> if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> drm_core_check_feature(dev, DRIVER_MODESET))
> - return -EINVAL;
> + return;
>
> idr_init(>ctx_idr);
> - return 0;
> }
>
> /**
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e5717116346d..ddc4943404c6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -584,14 +584,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver
> *driver,
> if (drm_ht_create(>map_hash, 12))
> goto err_minors;
>
> - if (drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) ||
> - !drm_core_check_feature(dev, DRIVER_MODESET))
> - ret = drm_legacy_ctxbitmap_init(dev);
> - if (ret) {
> - DRM_ERROR(
> - "Cannot allocate memory for context bitmap.\n");
> - goto err_ht;
> - }
> + drm_legacy_ctxbitmap_init(dev);
>
> if (drm_core_check_feature(dev, DRIVER_GEM)) {
> ret = drm_gem_init(dev);
> @@ -605,7 +598,6 @@ struct drm_device *drm_dev_alloc(struct drm_driver
> *driver,
>
> err_ctxbitmap:
> drm_legacy_ctxbitmap_cleanup(dev);
> -err_ht:
> drm_ht_remove(>map_hash);
> err_minors:
> drm_minor_free(dev, DRM_MINOR_LEGACY);
> diff --git a/drivers/gpu/drm/drm_legacy.h b/drivers/gpu/drm/drm_legacy.h
> index c1dc61473db5..9b731786e4db 100644
> --- a/drivers/gpu/drm/drm_legacy.h
> +++ b/drivers/gpu/drm/drm_legacy.h
> @@ -42,7 +42,7 @@ struct drm_file;
> #define DRM_KERNEL_CONTEXT 0
> #define DRM_RESERVED_CONTEXTS1
>
> -int drm_legacy_ctxbitmap_init(struct drm_device *dev);
> +void drm_legacy_ctxbitmap_init(struct drm_device *dev);
> void drm_legacy_ctxbitmap_cleanup(struct drm_device *dev);
> void drm_legacy_ctxbitmap_free(struct drm_device *dev, int ctx_handle);
> void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file
> *file);
Reviewed-by: Peter Antoine
[PATCH 1/3] drm: Turn off Legacy Context Functions
On Tue, 2015-06-23 at 11:37 +0200, Daniel Vetter wrote:
> From: Peter Antoine
>
> The context functions are not used by the i915 driver and should not
> be used by modeset drivers. These driver functions contain several bugs
> and security holes. This change makes these functions optional can be
> turned on by a setting, they are turned off by default for modeset
> driver with the exception of the nouvea driver that may require them with
> an old version of libdrm.
>
> The previous attempt was
>
> commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> Author: Daniel Vetter
> Date: Thu Aug 8 15:41:21 2013 +0200
>
> drm: mark context support as a legacy subsystem
>
> but this had to be reverted
>
> commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
> Author: Dave Airlie
> Date: Fri Sep 20 08:32:59 2013 +1000
>
> Revert "drm: mark context support as a legacy subsystem"
>
> v2: remove returns from void function, and formatting (Daniel Vetter)
>
> v3:
> - s/Nova/nouveau/ in the commit message, and add references to the
> previous attempts
> - drop the part touching the drm hw lock, that should be a separate
> patch.
>
> Signed-off-by: Peter Antoine (v2)
> Cc: Peter Antoine (v2)
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_context.c | 48
> +++
> drivers/gpu/drm/drm_drv.c | 13 ++
> drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++-
> include/drm/drmP.h| 23 +
> 4 files changed, 70 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> index 9b23525c0ed0..32958dabd7b0 100644
> --- a/drivers/gpu/drm/drm_context.c
> +++ b/drivers/gpu/drm/drm_context.c
> @@ -53,6 +53,10 @@ struct drm_ctx_list {
> */
> void drm_legacy_ctxbitmap_free(struct drm_device * dev, int ctx_handle)
> {
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return;
> +
> mutex_lock(>struct_mutex);
> idr_remove(>ctx_idr, ctx_handle);
> mutex_unlock(>struct_mutex);
> @@ -87,6 +91,10 @@ static int drm_legacy_ctxbitmap_next(struct drm_device *
> dev)
> */
> int drm_legacy_ctxbitmap_init(struct drm_device * dev)
> {
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> idr_init(>ctx_idr);
> return 0;
> }
> @@ -101,6 +109,10 @@ int drm_legacy_ctxbitmap_init(struct drm_device * dev)
> */
> void drm_legacy_ctxbitmap_cleanup(struct drm_device * dev)
> {
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return;
> +
> mutex_lock(>struct_mutex);
> idr_destroy(>ctx_idr);
> mutex_unlock(>struct_mutex);
> @@ -119,6 +131,10 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev,
> struct drm_file *file)
> {
> struct drm_ctx_list *pos, *tmp;
>
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return;
> +
> mutex_lock(>ctxlist_mutex);
>
> list_for_each_entry_safe(pos, tmp, >ctxlist, head) {
> @@ -161,6 +177,10 @@ int drm_legacy_getsareactx(struct drm_device *dev, void
> *data,
> struct drm_local_map *map;
> struct drm_map_list *_entry;
>
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> mutex_lock(>struct_mutex);
>
> map = idr_find(>ctx_idr, request->ctx_id);
> @@ -205,6 +225,10 @@ int drm_legacy_setsareactx(struct drm_device *dev, void
> *data,
> struct drm_local_map *map = NULL;
> struct drm_map_list *r_list = NULL;
>
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> mutex_lock(>struct_mutex);
> list_for_each_entry(r_list, >maplist, head) {
> if (r_list->map
> @@ -305,6 +329,10 @@ int drm_legacy_resctx(struct drm_device *dev, void *data,
> struct drm_ctx ctx;
> int i;
>
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> if (res->count >= DRM_RESERVED_CONTEXTS) {
> memset(, 0, sizeof(ctx));
> for (i = 0; i < DRM_RESERVED_CONTEXTS; i++) {
> @@ -335,6 +363,10 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
> struct drm_ctx_list *ctx_entry;
> struct drm_ctx *ctx = data;
>
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> + drm_core_check_feature(dev, DRIVER_MODESET))
> + return
[PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock fixes.
Please ignore this test as fixes are being implemented differently.
On Thu, 2015-04-23 at 15:07 +0100, Peter Antoine 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
> ---
> 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
> +bool drm_has_legacy_context(int fd)
> +{
> + int tmp = 0;
> + drm_i915_getparam_t gp;
> +
> + memset(, 0, sizeof(gp));
> + gp.value =
> + 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, ) != 0)
> + return true;
> +
> + return tmp == 1;
> +}
> /**
> * gem_available_aperture_size:
> * @fd: open i915 drm file descriptor
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index ced7ef3..3adc700 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -120,6 +120,7 @@ bool gem_has_bsd(int fd);
> bool gem_has_blt(int fd);
> bool gem_has_vebox(int fd);
> bool gem_has_bsd2(int fd);
> +bool drm_has_legacy_context(int fd);
> bool gem_uses_aliasing_ppgtt(int fd);
> int gem_available_fences(int fd);
> uint64_t gem_available_aperture_size(int fd);
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 71de6de..2f69afc 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -84,6 +84,7 @@ TESTS_progs_M = \
> pm_sseu \
> prime_self_import \
> template \
> + drm_hw_lock \
> $(NULL)
>
> TESTS_progs = \
> diff --git a/tests/drm_hw_lock.c b/tests/drm_hw_lock.c
> new file mode 100644
> index 000..aad50ba
> --- /dev/null
> +++ b/tests/drm_hw_lock.c
> @@ -0,0 +1,207 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *Peter Antoine
> + */
> +
> +/*
> + * Testcase: Test the HW_LOCKs for correct support and non-crashing.
> + *
> + * This test will check that he hw_locks are only g_supported for drivers
> that
> + * require that support. If it is not g_supported then the functions all
> return
> + * the correct error code.
> + *
> + * If g_supported it will check that the functions do not crash when the
> crash
> + * tests are used, also that one of the tests is a security level escalation
> + * that should be rejected.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include "drmtest.h"
> +#include "igt_core.h"
> +#include "ioctl_wrappers.h"
> +
> +#ifndef DRM_KERNEL_CONTEXT
> +#define DRM_KERNEL_CONTEXT (0)
> +#endif
> +
> +#ifndef _DRM_LOCK_HELD
> +#define _DRM_LOCK_HELD 0x8000U /**< Hardware lock is held */
> +#endif
> +
> +#ifndef _DRM_LOCK_CONT
> +#define _DRM_LOCK_CONT 0x4000U /**< Hardware lock is contended */
> +#endif
> +
> +static bool g_sig_fired;
> +static bool g_supported;
> +static struct sigaction
[Intel-gfx] [PATCH 3/5] drm: Possible lock priority escalation.
On Mon, 2015-05-04 at 15:56 +0200, Daniel Vetter wrote:
> On Mon, Apr 27, 2015 at 07:52:46PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 23, 2015 at 03:07:56PM +0100, Peter Antoine wrote:
> > > If an application that has a driver lock created, wants the lock the
> > > kernel context, it is not allowed to. If the call to drm_lock has a
> > > context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
> > > then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
> > > But as the DRM_LOCK_CONT bits are not part of the context id this allows
> > > operations on the DRM_KERNEL_CONTEXT.
> > >
> > > Issue: VIZ-5485
> > > Signed-off-by: Peter Antoine
>
> If you're touching code with drm_legacy_ prefix of in such a file you've
> ended up in the horrible corners of the dri1 dungeons and should head back
> out pronto ;-)
>
> If we can actually run into this code on production i915 then we need to
> improve the locks at the door of these dungeons for kms drivers, not try
> to fix up the mess behind them. That's just plain impossible.
>
> If you want to make really sure we get this right some simple drm igt
> tests to make sure these codepaths are really dead for kms driver might be
> good. But otherwise we really can only annotate this as wontfix in
> code security issue scanners.
> -Daniel
>
There is a test that covers this fix. This is a simple three line fix
that stops a userspace driver locking the kernel context. Yes they are
other problems with this code, but why are they stopping this patch that
does a simple fix from going in?
I'll happily drop this patch if it causes more problems that it fixes.
Peter.
> > > ---
> > > drivers/gpu/drm/drm_context.c | 6 +++---
> > > drivers/gpu/drm/drm_lock.c| 4 ++--
> > > 2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> > > index 96350d1..1febcd3 100644
> > > --- a/drivers/gpu/drm/drm_context.c
> > > +++ b/drivers/gpu/drm/drm_context.c
> > > @@ -123,7 +123,7 @@ void drm_legacy_ctxbitmap_flush(struct drm_device
> > > *dev, struct drm_file *file)
> > >
> > > list_for_each_entry_safe(pos, tmp, >ctxlist, head) {
> > > if (pos->tag == file &&
> > > - pos->handle != DRM_KERNEL_CONTEXT) {
> > > + _DRM_LOCKING_CONTEXT(pos->handle) != DRM_KERNEL_CONTEXT) {
> > > if (dev->driver->context_dtor)
> > > dev->driver->context_dtor(dev, pos->handle);
> > >
> > > @@ -342,7 +342,7 @@ int drm_legacy_addctx(struct drm_device *dev, void
> > > *data,
> > > struct drm_ctx *ctx = data;
> > >
> > > ctx->handle = drm_legacy_ctxbitmap_next(dev);
> > > - if (ctx->handle == DRM_KERNEL_CONTEXT) {
> > > + if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
> > > /* Skip kernel's context and get a new one. */
> > > ctx->handle = drm_legacy_ctxbitmap_next(dev);
> > > }
> > > @@ -449,7 +449,7 @@ int drm_legacy_rmctx(struct drm_device *dev, void
> > > *data,
> > > struct drm_ctx *ctx = data;
> > >
> > > DRM_DEBUG("%d\n", ctx->handle);
> > > - if (ctx->handle != DRM_KERNEL_CONTEXT) {
> > > + if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
> > > if (dev->driver->context_dtor)
> > > dev->driver->context_dtor(dev, ctx->handle);
> > > drm_legacy_ctxbitmap_free(dev, ctx->handle);
> >
> > How about just fixing the end parameter passed to idr_alloc()? AFAICS
> > that would take care of the context code.
> >
> > Well, there are a few more issues with the code:
> > - not properly checking for negative return value from idr_alloc()
> > - leaking the ctx id on kmalloc() error
> > - pointless check for idr_alloc() returning 0 even though the min is 1
> >
> > > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > > index 070dd5d..94500930 100644
> > > --- a/drivers/gpu/drm/drm_lock.c
> > > +++ b/drivers/gpu/drm/drm_lock.c
> > > @@ -63,7 +63,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> > >
> > > ++file_priv->lock_count;
> >
> > While you're poking around this dungeopn, maybe you can kill lock_count?
> > We never seem to decrement it, and it's only checked in
> > drm_legacy_i_have_hw_lock().
> >
> > >
> > > - if (lock->context == DRM_KERNEL_CONTEXT) {
> > > + if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > > DRM_ERROR("Process %d using kernel context %d\n",
> > > task_pid_nr(current), lock->context);
> > > return -EINVAL;
> > > @@ -153,7 +153,7 @@ int drm_legacy_unlock(struct drm_device *dev, void
> > > *data, struct drm_file *file_
> > > struct drm_lock *lock = data;
> > > struct drm_master *master = file_priv->master;
> > >
> > > - if (lock->context == DRM_KERNEL_CONTEXT) {
> > > + if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > > DRM_ERROR("Process %d using
[Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock
On Mon, 2015-05-04 at 15:52 +0200, Daniel Vetter wrote: > On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris at chris-wilson.co.uk wrote: > > On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote: > > > On 24/04/15 06:52, Antoine, Peter wrote: > > > > I picked up this work due to the following Jira ticket created by the > > > > security team (on Android) and was asked to give it a second look and > > > > found a few more issues with the hw lock code. > > > > > > > > https://jira01.devtools.intel.com/browse/GMINL-5388 > > > > I/O control on /dev/dri/card0 crashes the kernel (0x4008642b) > > > > > > > > It also stops Linux as it kills the driver, I guess it might be possible > > > > to reload the gfx driver. On a unpatched system the test that is > > > > included in the issue or the igt test that has been posted for the issue > > > > will show the problem. > > > > > > > > I ran the test on an unpatched system here and the gui stopped and the > > > > keyboard stopped responding, so I rebooted. With the patched system I > > > > did not need to reboot. > > > > > > > > Should I change the SIGTERM to SIGSEGV, not quite the same thing but > > > > tooling is better at handling a segfault than a SIGTERM and the > > > > application that calls this IOCTL is using an uninitialised hw lock so > > > > it is kind of the same as differencing an uninitialised pointer (kind > > > > of). Or, I could just remove it, but the bug has been in the code for at > > > > least two years (and known about), and I would guess that any code that > > > > is calling this is fuzzing the IOCTLs (as this is how the security team > > > > found it) and we should reward them with a application exit. > > > > > > > > Peter. > > > > > > SIGSEGV would be a better choice. > > > > > > SIGTERM is normally sent by a user -- it's the default signal sent by > > > kill(1). It's also commonly used to tell a long-running daemon process > > > to tidy up and exit cleanly. > > > > > > SIGSEGV commonly means "you accessed something that doesn't exist/isn't > > > mapped/you don't have permissions for". There are specific subcases that > > > can be indicated via the siginfo data; this is from the sigaction(1) > > > manpage: > > > > > > The following values can be placed in si_code for a SIGSEGV signal: > > > > > > SEGV_MAPERRaddress not mapped to object > > > > > > SEGV_ACCERRinvalid permissions for mapped object > > > > > > SIGBUS would also be a possibility but that's generally taken to mean > > > that an access got all the way to some physical bus and then faulted, > > > whereas SIGSEGV suggests the access was rejected during the > > > virtual-to-physical mapping process. > > > > None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate. > > Seconded, we really don't want to be in the business of fixing up the drm > design mistakes of the past 15 years. As long as we can fully lock out > this particular dragon when running i915 we're imo good enough. The dri1 > design of a kernel shim driver cooperating with the ums driver for hw > ownership is fundamentally unfixable. > > Also we can't change any of it for drivers actually using it since it'll > break them, which is a big no-go. > -Daniel I will remove it. But, If you are using this code path the driver/kernel will have crashed. It covers a NULL pointer deference, so we are not changing the API that anyone is actually using. Peter.
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
> > > > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index
> > > > > 62c40777..367e42f 100644
> > > > > --- a/include/drm/drmP.h
> > > > > +++ b/include/drm/drmP.h
> > > > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
> > > > >
> > > > > /* driver capabilities and requirements mask */
> > > > > -#define DRIVER_USE_AGP 0x1
> > > > > -#define DRIVER_PCI_DMA 0x8
> > > > > -#define DRIVER_SG 0x10
> > > > > -#define DRIVER_HAVE_DMA0x20
> > > > > -#define DRIVER_HAVE_IRQ0x40
> > > > > -#define DRIVER_IRQ_SHARED 0x80
> > > > > -#define DRIVER_GEM 0x1000
> > > > > -#define DRIVER_MODESET 0x2000
> > > > > -#define DRIVER_PRIME 0x4000
> > > > > -#define DRIVER_RENDER 0x8000
> > > > > -#define DRIVER_ATOMIC 0x1
> > > > > +#define DRIVER_USE_AGP 0x1
> > > > > +#define DRIVER_PCI_DMA 0x8
> > > > > +#define DRIVER_SG0x10
> > > > > +#define DRIVER_HAVE_DMA 0x20
> > > > > +#define DRIVER_HAVE_IRQ 0x40
> > > > > +#define DRIVER_IRQ_SHARED0x80
> > > > > +#define DRIVER_GEM 0x1000
> > > > > +#define DRIVER_MODESET 0x2000
> > > > > +#define DRIVER_PRIME 0x4000
> > > > > +#define DRIVER_RENDER0x8000
> > > > > +#define DRIVER_ATOMIC0x1
> > > > > +#define DRIVER_KMS_LEGACY_CONTEXT0x2
> > > >
> > > > Why is there KMS in the name?
> > > >
> > > > By suggestion of Daniel.
> > > >
> > > > I was thinking just checking for GEM, but I think there was some
> > > > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > > > now dead as far as i915 is concerned, so in theory it should be fine.
> > > > But I'm not sure if some other driver might have the same baggage.
> > > >
> > > > Other drivers have the same baggage.
> > > >
> > > > I suppose one option would be to check for MODESET instead. kms+dri1
> > > > doesn't sound like an entirely sane combination to me.
> > > >
> > > > Can't use the MODESET as this was how it was turned off in the previous
> > > > incarnation and was reverted by Dave Airle.
> > >
> > > Reference?
> >
> > From the next commit [5/5] as it is the one that actually turns off the
> > functions that were turned off before.
> >
> > These changes are based on the two patches:
> > commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
> > Author: Dave Airlie
> >
> > And the commit that the above patch reverts:
> > commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> > Author: Daniel Vetter
>
> Looking at ancient libdrm sources makes me think nouveau just used to
> create and destroy the context, but not actually use it for anything.
> So nopping out the ioctls should be good enough AFAICS. Or am I missing
> something?
>
An old version of libdrm that still requires support needs them, it's
the reason that David Airlie reverted the patch that Daniel did to
remove the functions. Do they still need support, I don't know? David
Airlie is on the cc list.
A discussion was had and this was the way that it was suggested it be
done. This seems a good half-way house, the actual security holes that
have been found have been fixed and the functions (that seem to have a
lot more security issues in them) are turned off for the drivers that
don't use them, and if a driver does require them, it will be a one line
change to reintroduce them. Are we carrying code we don't need to
support, probably.
Peter.
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
reply at end.
On Tue, 2015-04-28 at 13:40 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 05:52:20AM +0000, Antoine, Peter wrote:
> > Hi,
> >
> > (replies inline)
> >
> > -Original Message-
> > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > Sent: Monday, April 27, 2015 6:04 PM
> > To: Antoine, Peter
> > Cc: intel-gfx at lists.freedesktop.org; airlied at redhat.com; dri-devel at
> > lists.freedesktop.org; daniel.vetter at ffwll.ch
> > Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions
> > optional.
> >
> > On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> > > As these functions are only used by one driver and there are security
> > > holes in these functions. Make the functions optional.
> > >
> > > Issue: VIZ-5485
> > > Signed-off-by: Peter Antoine
> > > ---
> > > drivers/gpu/drm/drm_lock.c| 6 ++
> > > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > > drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++-
> > > include/drm/drmP.h| 23 ---
> > > include/uapi/drm/i915_drm.h | 1 +
> > > 5 files changed, 24 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > > index 94500930..b61d4c7 100644
> > > --- a/drivers/gpu/drm/drm_lock.c
> > > +++ b/drivers/gpu/drm/drm_lock.c
> > > @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> > > struct drm_master *master = file_priv->master;
> > > int ret = 0;
> > >
> > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > + return -EINVAL;
> > > +
> > > ++file_priv->lock_count;
> > >
> > > if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@
> > > -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void
> > > *data, struct drm_file *file_
> > > struct drm_lock *lock = data;
> > > struct drm_master *master = file_priv->master;
> > >
> > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > + return -EINVAL;
> > > +
> > > if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > > DRM_ERROR("Process %d using kernel context %d\n",
> > > task_pid_nr(current), lock->context); diff --git
> > > a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index e44116f..c771ef0 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void
> > > *data,
> > > if (!value)
> > > return -ENODEV;
> > > break;
> > > + case I915_PARAM_HAS_LEGACY_CONTEXT:
> > > + value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> > > + break;
> >
> > Seems pointless to add a parameter that'll always be false.
> >
> > There is some history to these changes, the HW_LOCK functions were removed
> > previously but causes an issue with the Nouveau drivers. So that the
> > functions where put back in. So the parameter has been added to allow for
> > that driver to turn the legacy context on as it is needed.
> >
> > > default:
> > > DRM_DEBUG("Unknown parameter %d\n", param->param);
> > > return -EINVAL;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > index 8763deb..936b423 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = {
> > > .driver_features =
> > > DRIVER_USE_AGP |
> > > - DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> > > + DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> > > + DRIVER_KMS_LEGACY_CONTEXT,
> >
> > Why is this here? AFAICS only the via driver cares about legacy contexts,
> > and only dri1 drivers care about the hw lock.
> >
> > See above.
> > >
> > > .load = nouveau_drm_load,
> > > .unload = nouveau_drm_unload,
> > > diff --git a/i
[Intel-gfx] [PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock fixes.
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 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
> > > ---
> > > 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(, 0, sizeof(gp));
> > > + gp.value =
> > > + 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, ) != 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.
Peter.
[Intel-gfx] [PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock fixes.
Thanks for the review, new patch inbound.
-Original Message-
From: Thomas Wood [mailto:thomas.w...@intel.com]
Sent: Monday, April 27, 2015 4:25 PM
To: Antoine, Peter
Cc: Intel Graphics Development; airlied at redhat.com; dri-devel at
lists.freedesktop.org; Daniel Vetter
Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock
fixes.
On 23 April 2015 at 15:07, Peter Antoine 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
> ---
> 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(, 0, sizeof(gp));
> + gp.value =
> + 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, ) != 0)
> + return true;
> +
> + return tmp == 1;
> +}
> /**
> * gem_available_aperture_size:
> * @fd: open i915 drm file descriptor diff --git
> a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h index ced7ef3..3adc700
> 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -120,6 +120,7 @@ bool gem_has_bsd(int fd); bool gem_has_blt(int
> fd); bool gem_has_vebox(int fd); bool gem_has_bsd2(int fd);
> +bool drm_has_legacy_context(int fd);
> bool gem_uses_aliasing_ppgtt(int fd); int gem_available_fences(int
> fd); uint64_t gem_available_aperture_size(int fd); diff --git
> a/tests/Makefile.sources b/tests/Makefile.sources index
> 71de6de..2f69afc 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -84,6 +84,7 @@ TESTS_progs_M = \
> pm_sseu \
> prime_self_import \
> template \
> + drm_hw_lock \
Please also add the binary name to .gitignore.
> $(NULL)
>
> TESTS_progs = \
> diff --git a/tests/drm_hw_lock.c b/tests/drm_hw_lock.c new file mode
> 100644 index 000..aad50ba
> --- /dev/null
> +++ b/tests/drm_hw_lock.c
> @@ -0,0 +1,207 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> +"Software"),
> + * to deal in the Software without restriction, including without
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including
> +the next
> + * paragraph) shall be included in all copies or substantial portions
> +of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> +SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> +OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> +OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *Peter Antoine
> + */
> +
> +/*
> + * Testcase: Test the HW_LOCKs for correct support and non-crashing.
This would be a good sentence to use in the IGT_TEST_DESCRIPTION macro, to add
a description for the
[Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
Hi,
(replies inline)
-Original Message-
From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
Sent: Monday, April 27, 2015 6:04 PM
To: Antoine, Peter
Cc: intel-gfx at lists.freedesktop.org; airlied at redhat.com; dri-devel at
lists.freedesktop.org; daniel.vetter at ffwll.ch
Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions
optional.
On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> As these functions are only used by one driver and there are security
> holes in these functions. Make the functions optional.
>
> Issue: VIZ-5485
> Signed-off-by: Peter Antoine
> ---
> drivers/gpu/drm/drm_lock.c| 6 ++
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++-
> include/drm/drmP.h| 23 ---
> include/uapi/drm/i915_drm.h | 1 +
> 5 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index 94500930..b61d4c7 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> struct drm_master *master = file_priv->master;
> int ret = 0;
>
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> + return -EINVAL;
> +
> ++file_priv->lock_count;
>
> if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@
> -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data,
> struct drm_file *file_
> struct drm_lock *lock = data;
> struct drm_master *master = file_priv->master;
>
> + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> + return -EINVAL;
> +
> if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> DRM_ERROR("Process %d using kernel context %d\n",
> task_pid_nr(current), lock->context); diff --git
> a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e44116f..c771ef0 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void
> *data,
> if (!value)
> return -ENODEV;
> break;
> + case I915_PARAM_HAS_LEGACY_CONTEXT:
> + value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> + break;
Seems pointless to add a parameter that'll always be false.
There is some history to these changes, the HW_LOCK functions were removed
previously but causes an issue with the Nouveau drivers. So that the functions
where put back in. So the parameter has been added to allow for that driver to
turn the legacy context on as it is needed.
> default:
> DRM_DEBUG("Unknown parameter %d\n", param->param);
> return -EINVAL;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 8763deb..936b423 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = {
> .driver_features =
> DRIVER_USE_AGP |
> - DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> + DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> + DRIVER_KMS_LEGACY_CONTEXT,
Why is this here? AFAICS only the via driver cares about legacy contexts, and
only dri1 drivers care about the hw lock.
See above.
>
> .load = nouveau_drm_load,
> .unload = nouveau_drm_unload,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h index
> 62c40777..367e42f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
>
> /* driver capabilities and requirements mask */
> -#define DRIVER_USE_AGP 0x1
> -#define DRIVER_PCI_DMA 0x8
> -#define DRIVER_SG 0x10
> -#define DRIVER_HAVE_DMA0x20
> -#define DRIVER_HAVE_IRQ0x40
> -#define DRIVER_IRQ_SHARED 0x80
> -#define DRIVER_GEM 0x1000
> -#define DRIVER_MODESET 0x2000
> -#define DRIVER_PRIME 0x4000
> -#define DRIVER_RENDER 0x8000
> -#define DRIVER_ATOMIC 0x1
> +#define DRIVER_USE_AGP 0x1
> +#define DRIVER_PCI_DMA 0x8
> +#define DRIVER_SG0x10
> +#define DRIVER_HAVE_DMA 0x20
> +#define DRIVER_HAVE_IRQ 0x40
>
[Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code. https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b) It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem. I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot. Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit. Peter. On Thu, 2015-04-23 at 15:39 +0100, Chris Wilson wrote: > On Thu, Apr 23, 2015 at 02:34:24PM +0000, Antoine, Peter wrote: > > Before the patch the system required rebooting (driver crash and/or kernel > > panic). > > Now the application gets terminated. > > It should have an GPF which should not have required a reboot, but > terminated the application. Unless you set it to automatically reboot. > -Chris >
[Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock
Before the patch the system required rebooting (driver crash and/or kernel panic). Now the application gets terminated. This follows the pattern in the other parts of this code that checks that pointer. Peter. -Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Thursday, April 23, 2015 3:20 PM To: Antoine, Peter Cc: intel-gfx at lists.freedesktop.org; airlied at redhat.com; dri-devel at lists.freedesktop.org; daniel.vetter at ffwll.ch Subject: Re: [Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock On Thu, Apr 23, 2015 at 03:07:54PM +0100, Peter Antoine wrote: > This patch fixes a possible kernel crash when drm_unlock > (DRM_IOCTL_UNLOCK) is called by a application that has not had a lock > created by it. This crash can be caused by any application from all users. Crashing the application is still a crash... -Chris -- Chris Wilson, Intel Open Source Technology Centre
