diff mbox

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

Message ID 1429798078-18987-4-git-send-email-peter.antoine@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Antoine April 23, 2015, 2:07 p.m. UTC 
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(-)

Comments

Ville Syrjala April 27, 2015, 4:52 p.m. UTC | #1 
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
Daniel Vetter May 4, 2015, 1:56 p.m. UTC | #2 
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

> > ---
> >  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
Peter Antoine May 5, 2015, 6:45 a.m. UTC | #3 
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

>
Daniel Vetter May 5, 2015, 7:23 a.m. UTC | #4 
On Tue, May 05, 2015 at 06:45:30AM +0000, Antoine, Peter wrote:
> 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.

Because we don't want to fix the legacy context crap but instead outright
disable it for everyone (well except nouveau) running in kms code. drm
legacy code really is broken by design, there's no way to fix it.

And worst case we'll end up breaking some old machines in some and then
have to deal with the regression. Which means I won't apply these patches
if we can somehow just disable all that code for i915.
-Daniel
diff mbox

Patch

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);
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;
 
-	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;