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

["Antoine, Peter" <peter.antoine@intel.com>]

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
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx