[PATCH] drm: Kernel Crash in drm_unlock

[PATCH] drm: Kernel Crash in drm_unlock

[Peter Antoine]

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.

Issue: GMINL-7446
Change-Id: I901ff713be53c5ec1c9eaf7ee0ff4314a659af05
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_lock.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index f645268..80253a7 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		return -EINVAL;
 	}
 
+	if (!master->lock.hw_lock) {
+		DRM_ERROR(
+			"Device has been unregistered. Hard exit. Process %d\n",
+			task_pid_nr(current));
+		send_sig(SIGTERM, current, 0);
+		return -EINTR;
+	}
+
 	if (drm_lock_free(&master->lock, lock->context)) {
 		/* FIXME: Should really bail out here. */
 	}
-- 
1.9.1

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

[PATCH] drm: Possible lock priority escalation.

[Peter Antoine]

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: GMINL-7408
Change-Id: I681d6b8d4e5de156b53ca80e0a6f9e72d77b6c06
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_lock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index 80253a7..776f2db 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -60,7 +60,7 @@ int drm_lock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 
 	++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;
@@ -150,7 +150,7 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 	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;
-- 
1.9.1

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH] drm: Possible lock priority escalation.

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6099
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                                  304/304              304/304
IVB                                  338/338              338/338
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  309/309              309/309
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Fixes unsafe deference in locks.

[Peter Antoine]

This patch fixes an unsafe deference in the DRM_IOCTL_NEW_CTX. If the
ioctl is called before the lock is created or after it has been destroyed.
The code will deference a NULL pointer. This ioctl is a root ioctl so
exploitation is limited.

Issue: GMINL-7409
Change-Id: Icabf814abe8225d616fdf4f981cd36d2b27f7ad5
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_context.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
index a4b017b..4754e79 100644
--- a/drivers/gpu/drm/drm_context.c
+++ b/drivers/gpu/drm/drm_context.c
@@ -252,7 +252,13 @@ static int drm_context_switch_complete(struct drm_device *dev,
 {
 	dev->last_context = new;	/* PRE/POST: This is the _only_ writer. */
 
-	if (!_DRM_LOCK_IS_HELD(file_priv->master->lock.hw_lock->lock)) {
+	if (file_priv->master->lock.hw_lock == NULL) {
+		DRM_ERROR(
+			"Device has been unregistered. Hard exit. Process %d\n",
+			task_pid_nr(current));
+		send_sig(SIGTERM, current, 0);
+		return -EINTR;
+	} else if (!_DRM_LOCK_IS_HELD(file_priv->master->lock.hw_lock->lock)) {
 		DRM_ERROR("Lock isn't held after context switch\n");
 	}
 
-- 
1.9.1

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH] drm: Fixes unsafe deference in locks.

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6100
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              304/304              303/304
IVB                                  338/338              338/338
BYT                                  287/287              287/287
HSW                 -1              361/361              360/361
BDW                                  309/309              309/309
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@kms_rotation_crc@sprite-rotation      PASS(2)      FAIL(1)PASS(1)
*HSW  igt@pm_rpm@drm-resources-equal      PASS(4)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Kernel Crash in drm_unlock

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6098
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                                  304/304              304/304
IVB                                  338/338              338/338
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  309/309              309/309
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Daniel Vetter]

On Tue, Mar 31, 2015 at 09:09:33AM +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.
> 
> Issue: GMINL-7446
> Change-Id: I901ff713be53c5ec1c9eaf7ee0ff4314a659af05
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>

Can you really blow this up at runtime with modern modeset drivers like
i915? Counts for all three patches ...

> ---
>  drivers/gpu/drm/drm_lock.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index f645268..80253a7 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)

Also please rebase to latest upstream when submitting patches to the
public (the function is now called drm_legacy_unlock).

>  		return -EINVAL;
>  	}
>  
> +	if (!master->lock.hw_lock) {
> +		DRM_ERROR(
> +			"Device has been unregistered. Hard exit. Process %d\n",
> +			task_pid_nr(current));
> +		send_sig(SIGTERM, current, 0);
> +		return -EINTR;
> +	}
> +
>  	if (drm_lock_free(&master->lock, lock->context)) {
>  		/* FIXME: Should really bail out here. */
>  	}
> -- 
> 1.9.1
> 
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

And please remove this disclaimer.

Thanks, Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Damien Lespiau]

On Tue, Mar 31, 2015 at 03:25:32PM +0200, Daniel Vetter wrote:
> > ---
> >  drivers/gpu/drm/drm_lock.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > index f645268..80253a7 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
> 
> Also please rebase to latest upstream when submitting patches to the
> public (the function is now called drm_legacy_unlock).

and include dri-devel@lists.freedesktop.org for core drm patches.

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

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

This was found by the security guys using an ioctl fuzzer.
12 lines of code from a new unprivileged user and the kernel goes bang.
  
The other crash was just found using code inspection, but it is the same basic issue.
Either the hw_lock was not created or the was deleted and the pointer is dereferenced.

For the escalation, there is not proof of concept, but it is a bad comparison as the bits are stripped off for other checks.

I'll be re-spinning the patches when I get notified that I am on the no footer list.

Peter.
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, March 31, 2015 2:26 PM
To: Antoine, Peter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock

On Tue, Mar 31, 2015 at 09:09:33AM +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.
> 
> Issue: GMINL-7446
> Change-Id: I901ff713be53c5ec1c9eaf7ee0ff4314a659af05
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>

Can you really blow this up at runtime with modern modeset drivers like i915? Counts for all three patches ...

> ---
>  drivers/gpu/drm/drm_lock.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> index f645268..80253a7 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void 
> *data, struct drm_file *file_priv)

Also please rebase to latest upstream when submitting patches to the public (the function is now called drm_legacy_unlock).

>  		return -EINVAL;
>  	}
>  
> +	if (!master->lock.hw_lock) {
> +		DRM_ERROR(
> +			"Device has been unregistered. Hard exit. Process %d\n",
> +			task_pid_nr(current));
> +		send_sig(SIGTERM, current, 0);
> +		return -EINTR;
> +	}
> +
>  	if (drm_lock_free(&master->lock, lock->context)) {
>  		/* FIXME: Should really bail out here. */
>  	}
> --
> 1.9.1
> 
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for 
> the sole use of the intended recipient(s). Any review or distribution 
> by others is strictly prohibited. If you are not the intended 
> recipient, please contact the sender and delete all copies.

And please remove this disclaimer.

Thanks, Daniel

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

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Daniel Vetter]

On Tue, Mar 31, 2015 at 01:34:25PM +0000, Antoine, Peter wrote:
> This was found by the security guys using an ioctl fuzzer.
> 12 lines of code from a new unprivileged user and the kernel goes bang.
>   
> The other crash was just found using code inspection, but it is the same basic issue.
> Either the hw_lock was not created or the was deleted and the pointer is dereferenced.
> 
> For the escalation, there is not proof of concept, but it is a bad
> comparison as the bits are stripped off for other checks.
> 
> I'll be re-spinning the patches when I get notified that I am on the no
> footer list.

In that case I think an igt testcase to make this go boom would be great.
Testbinary prefix for drm core is drm_ (there's some already).

Meanwhile I did dig out the history for this and it's not pretty. See

commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Sep 20 08:32:59 2013 +1000

    Revert "drm: mark context support as a legacy subsystem"

Imo the correct way to fix this isn't to try to fix the code (it's
hopeless, making it go boom with fuzzing is just the tip of the iceberg),
but instead to disable it. But we may not break nouvea, so needs a bit
more elaborate:
1. Add DRIVER_KMS_LEGACY_CONTEXT driver flag and add it to nouveau.
2. Modify all the DRIVER_MODESET checks from my patch
(7c510133d93dd6f15ca040733ba7b2891ed61fd1) to still let the ioctls through
when DRIVER_KMS_LEGACY_CONTEXT is set.

Can you please sign up for this plus the minimal igt?

Thanks, Daniel
> 
> Peter.
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 31, 2015 2:26 PM
> To: Antoine, Peter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock
> 
> On Tue, Mar 31, 2015 at 09:09:33AM +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.
> > 
> > Issue: GMINL-7446
> > Change-Id: I901ff713be53c5ec1c9eaf7ee0ff4314a659af05
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> 
> Can you really blow this up at runtime with modern modeset drivers like i915? Counts for all three patches ...
> 
> > ---
> >  drivers/gpu/drm/drm_lock.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> > index f645268..80253a7 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void 
> > *data, struct drm_file *file_priv)
> 
> Also please rebase to latest upstream when submitting patches to the public (the function is now called drm_legacy_unlock).
> 
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (!master->lock.hw_lock) {
> > +		DRM_ERROR(
> > +			"Device has been unregistered. Hard exit. Process %d\n",
> > +			task_pid_nr(current));
> > +		send_sig(SIGTERM, current, 0);
> > +		return -EINTR;
> > +	}
> > +
> >  	if (drm_lock_free(&master->lock, lock->context)) {
> >  		/* FIXME: Should really bail out here. */
> >  	}
> > --
> > 1.9.1
> > 
> > ---------------------------------------------------------------------
> > Intel Corporation (UK) Limited
> > Registered No. 1134945 (England)
> > Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> > 
> > This e-mail and any attachments may contain confidential material for 
> > the sole use of the intended recipient(s). Any review or distribution 
> > by others is strictly prohibited. If you are not the intended 
> > recipient, please contact the sender and delete all copies.
> 
> And please remove this disclaimer.
> 
> Thanks, Daniel
> 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

Probably. I'll need to check this end.
I'll have a look.

Peter.

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, March 31, 2015 3:01 PM
To: Antoine, Peter
Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock

On Tue, Mar 31, 2015 at 01:34:25PM +0000, Antoine, Peter wrote:
> This was found by the security guys using an ioctl fuzzer.
> 12 lines of code from a new unprivileged user and the kernel goes bang.
>   
> The other crash was just found using code inspection, but it is the same basic issue.
> Either the hw_lock was not created or the was deleted and the pointer is dereferenced.
> 
> For the escalation, there is not proof of concept, but it is a bad 
> comparison as the bits are stripped off for other checks.
> 
> I'll be re-spinning the patches when I get notified that I am on the 
> no footer list.

In that case I think an igt testcase to make this go boom would be great.
Testbinary prefix for drm core is drm_ (there's some already).

Meanwhile I did dig out the history for this and it's not pretty. See

commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Sep 20 08:32:59 2013 +1000

    Revert "drm: mark context support as a legacy subsystem"

Imo the correct way to fix this isn't to try to fix the code (it's hopeless, making it go boom with fuzzing is just the tip of the iceberg), but instead to disable it. But we may not break nouvea, so needs a bit more elaborate:
1. Add DRIVER_KMS_LEGACY_CONTEXT driver flag and add it to nouveau.
2. Modify all the DRIVER_MODESET checks from my patch
(7c510133d93dd6f15ca040733ba7b2891ed61fd1) to still let the ioctls through when DRIVER_KMS_LEGACY_CONTEXT is set.

Can you please sign up for this plus the minimal igt?

Thanks, Daniel
> 
> Peter.
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> Daniel Vetter
> Sent: Tuesday, March 31, 2015 2:26 PM
> To: Antoine, Peter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock
> 
> On Tue, Mar 31, 2015 at 09:09:33AM +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.
> > 
> > Issue: GMINL-7446
> > Change-Id: I901ff713be53c5ec1c9eaf7ee0ff4314a659af05
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> 
> Can you really blow this up at runtime with modern modeset drivers like i915? Counts for all three patches ...
> 
> > ---
> >  drivers/gpu/drm/drm_lock.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> > index f645268..80253a7 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void 
> > *data, struct drm_file *file_priv)
> 
> Also please rebase to latest upstream when submitting patches to the public (the function is now called drm_legacy_unlock).
> 
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (!master->lock.hw_lock) {
> > +		DRM_ERROR(
> > +			"Device has been unregistered. Hard exit. Process %d\n",
> > +			task_pid_nr(current));
> > +		send_sig(SIGTERM, current, 0);
> > +		return -EINTR;
> > +	}
> > +
> >  	if (drm_lock_free(&master->lock, lock->context)) {
> >  		/* FIXME: Should really bail out here. */
> >  	}
> > --
> > 1.9.1
> > 
> > --------------------------------------------------------------------
> > -
> > Intel Corporation (UK) Limited
> > Registered No. 1134945 (England)
> > Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> > 
> > This e-mail and any attachments may contain confidential material 
> > for the sole use of the intended recipient(s). Any review or 
> > distribution by others is strictly prohibited. If you are not the 
> > intended recipient, please contact the sender and delete all copies.
> 
> And please remove this disclaimer.
> 
> Thanks, Daniel
> 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for 
> the sole use of the intended recipient(s). Any review or distribution 
> by others is strictly prohibited. If you are not the intended 
> recipient, please contact the sender and delete all copies.
> 

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

Hi Daniel,

I am having a look at this now, as have some time.

So, to sum up what I think you want.
1. Re-base and apply the patches (so that the known holes are closed in
the Nouveau driver).
2. Add DRIVER_KMS_LEGACY_CONTEXT to include/drm/drmP.h
3. Add DRIVER_KMS_LEGACY_CONTEXT to .driver_features in file
drivers/gpu/drm/nouveau/nouveau_drm.h.
4. Change all the hw_lock IOCTL functions to have:
   +       if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
   +               return -EINVAL;
   +
5. Add an igt test, that would induce the crash on platforms that are
not patched and have DRIVER_KMS_LEGACY_CONTEXT enabled?

Is this about right?

Thanks,
Peter.


On Tue, 2015-03-31 at 16:00 +0200, Daniel Vetter wrote:
> On Tue, Mar 31, 2015 at 01:34:25PM +0000, Antoine, Peter wrote:
> > This was found by the security guys using an ioctl fuzzer.
> > 12 lines of code from a new unprivileged user and the kernel goes bang.
> >   
> > The other crash was just found using code inspection, but it is the same basic issue.
> > Either the hw_lock was not created or the was deleted and the pointer is dereferenced.
> > 
> > For the escalation, there is not proof of concept, but it is a bad
> > comparison as the bits are stripped off for other checks.
> > 
> > I'll be re-spinning the patches when I get notified that I am on the no
> > footer list.
> 
> In that case I think an igt testcase to make this go boom would be great.
> Testbinary prefix for drm core is drm_ (there's some already).
> 
> Meanwhile I did dig out the history for this and it's not pretty. See
> 
> commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
> Author: Dave Airlie <airlied@redhat.com>
> Date:   Fri Sep 20 08:32:59 2013 +1000
> 
>     Revert "drm: mark context support as a legacy subsystem"
> 
> Imo the correct way to fix this isn't to try to fix the code (it's
> hopeless, making it go boom with fuzzing is just the tip of the iceberg),
> but instead to disable it. But we may not break nouvea, so needs a bit
> more elaborate:
> 1. Add DRIVER_KMS_LEGACY_CONTEXT driver flag and add it to nouveau.
> 2. Modify all the DRIVER_MODESET checks from my patch
> (7c510133d93dd6f15ca040733ba7b2891ed61fd1) to still let the ioctls through
> when DRIVER_KMS_LEGACY_CONTEXT is set.
> 
> Can you please sign up for this plus the minimal igt?
> 
> Thanks, Daniel
> > 
> > Peter.
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, March 31, 2015 2:26 PM
> > To: Antoine, Peter
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock
> > 
> > On Tue, Mar 31, 2015 at 09:09:33AM +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.
> > > 
> > > Issue: GMINL-7446
> > > Change-Id: I901ff713be53c5ec1c9eaf7ee0ff4314a659af05
> > > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > 
> > Can you really blow this up at runtime with modern modeset drivers like i915? Counts for all three patches ...
> > 
> > > ---
> > >  drivers/gpu/drm/drm_lock.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> > > index f645268..80253a7 100644
> > > --- a/drivers/gpu/drm/drm_lock.c
> > > +++ b/drivers/gpu/drm/drm_lock.c
> > > @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void 
> > > *data, struct drm_file *file_priv)
> > 
> > Also please rebase to latest upstream when submitting patches to the public (the function is now called drm_legacy_unlock).
> > 
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	if (!master->lock.hw_lock) {
> > > +		DRM_ERROR(
> > > +			"Device has been unregistered. Hard exit. Process %d\n",
> > > +			task_pid_nr(current));
> > > +		send_sig(SIGTERM, current, 0);
> > > +		return -EINTR;
> > > +	}
> > > +
> > >  	if (drm_lock_free(&master->lock, lock->context)) {
> > >  		/* FIXME: Should really bail out here. */
> > >  	}
> > > --
> > > 1.9.1
> > > 
> > > ---------------------------------------------------------------------
> > > Intel Corporation (UK) Limited
> > > Registered No. 1134945 (England)
> > > Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> > > 
> > > This e-mail and any attachments may contain confidential material for 
> > > the sole use of the intended recipient(s). Any review or distribution 
> > > by others is strictly prohibited. If you are not the intended 
> > > recipient, please contact the sender and delete all copies.
> > 
> > And please remove this disclaimer.
> > 
> > Thanks, Daniel
> > 
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > ---------------------------------------------------------------------
> > Intel Corporation (UK) Limited
> > Registered No. 1134945 (England)
> > Registered Office: Pipers Way, Swindon SN3 1RJ
> > VAT No: 860 2173 47
> > 
> > This e-mail and any attachments may contain confidential material for
> > the sole use of the intended recipient(s). Any review or distribution
> > by others is strictly prohibited. If you are not the intended
> > recipient, please contact the sender and delete all copies.
> > 
> 

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

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Daniel Vetter]

On Wed, Apr 15, 2015 at 02:22:54PM +0000, Antoine, Peter wrote:
> Hi Daniel,
> 
> I am having a look at this now, as have some time.
> 
> So, to sum up what I think you want.
> 1. Re-base and apply the patches (so that the known holes are closed in
> the Nouveau driver).
> 2. Add DRIVER_KMS_LEGACY_CONTEXT to include/drm/drmP.h
> 3. Add DRIVER_KMS_LEGACY_CONTEXT to .driver_features in file
> drivers/gpu/drm/nouveau/nouveau_drm.h.
> 4. Change all the hw_lock IOCTL functions to have:
>    +       if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
>    +               return -EINVAL;
>    +
> 5. Add an igt test, that would induce the crash on platforms that are
> not patched and have DRIVER_KMS_LEGACY_CONTEXT enabled?
> 
> Is this about right?

Sounds like a solid plan. When you send out the patch please also cc
dri-devel since this will all touch code outside of i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Damien Lespiau]

On Tue, Mar 31, 2015 at 03:25:32PM +0200, Daniel Vetter wrote:
> > ---
> >  drivers/gpu/drm/drm_lock.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > index f645268..80253a7 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
> 
> Also please rebase to latest upstream when submitting patches to the
> public (the function is now called drm_legacy_unlock).

While we're at it, how did you send those emails to not have the patch
ordering in the tags? we should have had 1/3, 2/3 and 3/3 tags there.

The usual way is to give the revision of the start of the series (the
patch just before what you want to send, ie.
  
  git send-email --to=intel-gfx HEAD~3

  or

  git send-email --to=intel-gfx drm-intel/drm-intel-nightly

Remember to use --dry-run the first few times if you're not sure.

HTH,

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

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

Patch ordering, is deliberate. They are not dependent on each other.
I'll rebase and add the new  dri-devel@lists.freedesktop.org when is resubmit the patches.

Peter.

-----Original Message-----
From: Lespiau, Damien 
Sent: Tuesday, March 31, 2015 2:35 PM
To: Daniel Vetter
Cc: Antoine, Peter; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock

On Tue, Mar 31, 2015 at 03:25:32PM +0200, Daniel Vetter wrote:
> > ---
> >  drivers/gpu/drm/drm_lock.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> > index f645268..80253a7 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -156,6 +156,14 @@ int drm_unlock(struct drm_device *dev, void 
> > *data, struct drm_file *file_priv)
> 
> Also please rebase to latest upstream when submitting patches to the 
> public (the function is now called drm_legacy_unlock).

While we're at it, how did you send those emails to not have the patch ordering in the tags? we should have had 1/3, 2/3 and 3/3 tags there.

The usual way is to give the revision of the start of the series (the patch just before what you want to send, ie.
  
  git send-email --to=intel-gfx HEAD~3

  or

  git send-email --to=intel-gfx drm-intel/drm-intel-nightly

Remember to use --dry-run the first few times if you're not sure.

HTH,

--
Damien
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Damien Lespiau]

On Tue, Mar 31, 2015 at 02:38:15PM +0100, Antoine, Peter wrote:
> Patch ordering, is deliberate. They are not dependent on each other.
> I'll rebase and add the new  dri-devel@lists.freedesktop.org when is
> resubmit the patches.

Ah, well, huummm. That is something new and innovative for sure. I
haven't seen any precedent for this one. I'd rather we always do the
same thing to makes tools easier to write on top of the upstream
mailing-list centered process, otherwise it'll be painful. For instance
is PRTS going to cope? patchwork now sees all the patches as 1/3:

  http://patchwork.lespiau.name/series/1290/

We could make the tool understand that, but I believe it'll be much
easier if we stick to the somewhat established conventions.

HTH,

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

Re: [PATCH] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

No problem.
Will sequence them when I re-submit.

-----Original Message-----
From: Lespiau, Damien 
Sent: Tuesday, March 31, 2015 2:44 PM
To: Antoine, Peter
Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock

On Tue, Mar 31, 2015 at 02:38:15PM +0100, Antoine, Peter wrote:
> Patch ordering, is deliberate. They are not dependent on each other.
> I'll rebase and add the new  dri-devel@lists.freedesktop.org when is 
> resubmit the patches.

Ah, well, huummm. That is something new and innovative for sure. I haven't seen any precedent for this one. I'd rather we always do the same thing to makes tools easier to write on top of the upstream mailing-list centered process, otherwise it'll be painful. For instance is PRTS going to cope? patchwork now sees all the patches as 1/3:

  http://patchwork.lespiau.name/series/1290/

We could make the tool understand that, but I believe it'll be much easier if we stick to the somewhat established conventions.

HTH,

--
Damien
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH] drm: Kernel Crash in drm_unlock

[He, Shuang]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Damien Lespiau
> Sent: Tuesday, March 31, 2015 9:44 PM
> To: Antoine, Peter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm: Kernel Crash in drm_unlock
> 
> On Tue, Mar 31, 2015 at 02:38:15PM +0100, Antoine, Peter wrote:
> > Patch ordering, is deliberate. They are not dependent on each other.
> > I'll rebase and add the new  dri-devel@lists.freedesktop.org when is
> > resubmit the patches.
> 
> Ah, well, huummm. That is something new and innovative for sure. I
> haven't seen any precedent for this one. I'd rather we always do the
> same thing to makes tools easier to write on top of the upstream
> mailing-list centered process, otherwise it'll be painful. For instance
> is PRTS going to cope? patchwork now sees all the patches as 1/3:
> 
>   http://patchwork.lespiau.name/series/1290/
[He, Shuang] PRTS is treating each one as a single patch

Thanks
	--Shuang
> 
> We could make the tool understand that, but I believe it'll be much
> easier if we stick to the somewhat established conventions.
> 
> HTH,
> 
> --
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx