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 peter.antoine@intel.com
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, &dev->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 kernel context %d\n", task_pid_nr(current), lock->context); return -EINVAL;
These two changes look OK to me.
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC