Re: [Intel-gfx] [PATCH 3/5] drm: Possible lock priority escalation.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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