The following series of patches fix a number of security holes in the i915 driver (actually in drm but...). The first three patches remove the actual security issues that have been found. The last two patches make the functions optional for all drivers. The only driver that has this feature turned on is the Nouveau driver, thus hopefully not breaking that driver.
The patch set has been tested on the Intel platforms but has not been tested on the Nouveau driver. Hopefully someone with a working card and the right combination of drmlib can verify that the patches do what they say.
There is a i-g-t test that goes with this patchset, but that test SHOULD NOT be run before the kernel is patches as the test will crash the driver and/or make the kernel panic.
Peter.
Peter Antoine (5): drm: Kernel Crash in drm_unlock drm: Fixes unsafe deference in locks. drm: Possible lock priority escalation. drm: Make HW_LOCK access functions optional. drm: Make Legacy Context access functions optional.
drivers/gpu/drm/drm_context.c | 50 ++++++++++++++++++++++++++++++++--- drivers/gpu/drm/drm_drv.c | 12 +++++---- drivers/gpu/drm/drm_lock.c | 18 +++++++++++-- drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++- include/drm/drmP.h | 23 ++++++++-------- include/uapi/drm/i915_drm.h | 1 + 7 files changed, 87 insertions(+), 23 deletions(-)
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: VIZ-5485 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 f861361..070dd5d 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -159,6 +159,14 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_ 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 -EPERM; + } + if (drm_legacy_lock_free(&master->lock, lock->context)) { /* FIXME: Should really bail out here. */ }
On Thu, Apr 23, 2015 at 03:07:54PM +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.
Crashing the application is still a crash... -Chris
Before the patch the system required rebooting (driver crash and/or kernel panic). Now the application gets terminated.
This follows the pattern in the other parts of this code that checks that pointer.
Peter.
-----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Thursday, April 23, 2015 3:20 PM To: Antoine, Peter Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock
On Thu, Apr 23, 2015 at 03:07:54PM +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.
Crashing the application is still a crash... -Chris
-- Chris Wilson, Intel Open Source Technology Centre
On Thu, Apr 23, 2015 at 02:34:24PM +0000, Antoine, Peter wrote:
Before the patch the system required rebooting (driver crash and/or kernel panic). Now the application gets terminated.
It should have an GPF which should not have required a reboot, but terminated the application. Unless you set it to automatically reboot. -Chris
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code.
https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem.
I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot.
Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit.
Peter.
On Thu, 2015-04-23 at 15:39 +0100, Chris Wilson wrote:
On Thu, Apr 23, 2015 at 02:34:24PM +0000, Antoine, Peter wrote:
Before the patch the system required rebooting (driver crash and/or kernel panic). Now the application gets terminated.
It should have an GPF which should not have required a reboot, but terminated the application. Unless you set it to automatically reboot. -Chris
On 24/04/15 06:52, Antoine, Peter wrote:
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code.
https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem.
I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot.
Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit.
Peter.
SIGSEGV would be a better choice.
SIGTERM is normally sent by a user -- it's the default signal sent by kill(1). It's also commonly used to tell a long-running daemon process to tidy up and exit cleanly.
SIGSEGV commonly means "you accessed something that doesn't exist/isn't mapped/you don't have permissions for". There are specific subcases that can be indicated via the siginfo data; this is from the sigaction(1) manpage:
The following values can be placed in si_code for a SIGSEGV signal:
SEGV_MAPERR address not mapped to object
SEGV_ACCERR invalid permissions for mapped object
SIGBUS would also be a possibility but that's generally taken to mean that an access got all the way to some physical bus and then faulted, whereas SIGSEGV suggests the access was rejected during the virtual-to-physical mapping process.
.Dave.
On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
On 24/04/15 06:52, Antoine, Peter wrote:
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code.
https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem.
I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot.
Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit.
Peter.
SIGSEGV would be a better choice.
SIGTERM is normally sent by a user -- it's the default signal sent by kill(1). It's also commonly used to tell a long-running daemon process to tidy up and exit cleanly.
SIGSEGV commonly means "you accessed something that doesn't exist/isn't mapped/you don't have permissions for". There are specific subcases that can be indicated via the siginfo data; this is from the sigaction(1) manpage:
The following values can be placed in si_code for a SIGSEGV signal: SEGV_MAPERR address not mapped to object SEGV_ACCERR invalid permissions for mapped objectSIGBUS would also be a possibility but that's generally taken to mean that an access got all the way to some physical bus and then faulted, whereas SIGSEGV suggests the access was rejected during the virtual-to-physical mapping process.
None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate. -Chris
On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris@chris-wilson.co.uk wrote:
On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
On 24/04/15 06:52, Antoine, Peter wrote:
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code.
https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem.
I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot.
Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit.
Peter.
SIGSEGV would be a better choice.
SIGTERM is normally sent by a user -- it's the default signal sent by kill(1). It's also commonly used to tell a long-running daemon process to tidy up and exit cleanly.
SIGSEGV commonly means "you accessed something that doesn't exist/isn't mapped/you don't have permissions for". There are specific subcases that can be indicated via the siginfo data; this is from the sigaction(1) manpage:
The following values can be placed in si_code for a SIGSEGV signal: SEGV_MAPERR address not mapped to object SEGV_ACCERR invalid permissions for mapped objectSIGBUS would also be a possibility but that's generally taken to mean that an access got all the way to some physical bus and then faulted, whereas SIGSEGV suggests the access was rejected during the virtual-to-physical mapping process.
None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.
Seconded, we really don't want to be in the business of fixing up the drm design mistakes of the past 15 years. As long as we can fully lock out this particular dragon when running i915 we're imo good enough. The dri1 design of a kernel shim driver cooperating with the ums driver for hw ownership is fundamentally unfixable.
Also we can't change any of it for drivers actually using it since it'll break them, which is a big no-go. -Daniel
On Mon, 2015-05-04 at 15:52 +0200, Daniel Vetter wrote:
On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris@chris-wilson.co.uk wrote:
On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
On 24/04/15 06:52, Antoine, Peter wrote:
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code.
https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem.
I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot.
Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit.
Peter.
SIGSEGV would be a better choice.
SIGTERM is normally sent by a user -- it's the default signal sent by kill(1). It's also commonly used to tell a long-running daemon process to tidy up and exit cleanly.
SIGSEGV commonly means "you accessed something that doesn't exist/isn't mapped/you don't have permissions for". There are specific subcases that can be indicated via the siginfo data; this is from the sigaction(1) manpage:
The following values can be placed in si_code for a SIGSEGV signal: SEGV_MAPERR address not mapped to object SEGV_ACCERR invalid permissions for mapped objectSIGBUS would also be a possibility but that's generally taken to mean that an access got all the way to some physical bus and then faulted, whereas SIGSEGV suggests the access was rejected during the virtual-to-physical mapping process.
None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.
Seconded, we really don't want to be in the business of fixing up the drm design mistakes of the past 15 years. As long as we can fully lock out this particular dragon when running i915 we're imo good enough. The dri1 design of a kernel shim driver cooperating with the ums driver for hw ownership is fundamentally unfixable.
Also we can't change any of it for drivers actually using it since it'll break them, which is a big no-go. -Daniel
I will remove it. But, If you are using this code path the driver/kernel will have crashed. It covers a NULL pointer deference, so we are not changing the API that anyone is actually using.
Peter.
On Tue, May 05, 2015 at 06:37:51AM +0000, Antoine, Peter wrote:
On Mon, 2015-05-04 at 15:52 +0200, Daniel Vetter wrote:
On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris@chris-wilson.co.uk wrote:
On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
On 24/04/15 06:52, Antoine, Peter wrote:
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code.
https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem.
I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot.
Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit.
Peter.
SIGSEGV would be a better choice.
SIGTERM is normally sent by a user -- it's the default signal sent by kill(1). It's also commonly used to tell a long-running daemon process to tidy up and exit cleanly.
SIGSEGV commonly means "you accessed something that doesn't exist/isn't mapped/you don't have permissions for". There are specific subcases that can be indicated via the siginfo data; this is from the sigaction(1) manpage:
The following values can be placed in si_code for a SIGSEGV signal: SEGV_MAPERR address not mapped to object SEGV_ACCERR invalid permissions for mapped objectSIGBUS would also be a possibility but that's generally taken to mean that an access got all the way to some physical bus and then faulted, whereas SIGSEGV suggests the access was rejected during the virtual-to-physical mapping process.
None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.
Seconded, we really don't want to be in the business of fixing up the drm design mistakes of the past 15 years. As long as we can fully lock out this particular dragon when running i915 we're imo good enough. The dri1 design of a kernel shim driver cooperating with the ums driver for hw ownership is fundamentally unfixable.
Also we can't change any of it for drivers actually using it since it'll break them, which is a big no-go. -Daniel
I will remove it. But, If you are using this code path the driver/kernel will have crashed. It covers a NULL pointer deference, so we are not changing the API that anyone is actually using.
With ums/dri1 userspace can crash the kernel. That's pretty much by design, and it's pretty much unfixable. Trying to fix up the obvious ones like here is just cargo-culting. The only real option is to make absolutely sure we can't ever reach these codepaths at all from kms drivers like i915. Which tends to be a lot more work for auditing, but will be much more useful. -Daniel
On 28/04/15 10:21, Dave Gordon wrote:
On 24/04/15 06:52, Antoine, Peter wrote:
I picked up this work due to the following Jira ticket created by the security team (on Android) and was asked to give it a second look and found a few more issues with the hw lock code.
https://jira01.devtools.intel.com/browse/GMINL-5388 I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
It also stops Linux as it kills the driver, I guess it might be possible to reload the gfx driver. On a unpatched system the test that is included in the issue or the igt test that has been posted for the issue will show the problem.
I ran the test on an unpatched system here and the gui stopped and the keyboard stopped responding, so I rebooted. With the patched system I did not need to reboot.
Should I change the SIGTERM to SIGSEGV, not quite the same thing but tooling is better at handling a segfault than a SIGTERM and the application that calls this IOCTL is using an uninitialised hw lock so it is kind of the same as differencing an uninitialised pointer (kind of). Or, I could just remove it, but the bug has been in the code for at least two years (and known about), and I would guess that any code that is calling this is fuzzing the IOCTLs (as this is how the security team found it) and we should reward them with a application exit.
Peter.
SIGSEGV would be a better choice.
[snip]
Nope, I've changed my mind about this. I thought the problematic case was just a process releasing the lock without having acquired it, but on further examination it's really that the DRM master process has gone away or otherwise deleted (or never created?) the lock. And THEN the (non-master?) process tries to release the (now) non-existent lock.
But more importantly, there's existing code in the acquire-lock path that sends SIGTERM for this case, so obviously the release-lock code should be as similar as possible.
.Dave.
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: VIZ-5485 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 9b23525..96350d1 100644 --- a/drivers/gpu/drm/drm_context.c +++ b/drivers/gpu/drm/drm_context.c @@ -277,7 +277,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 -EPERM; + } else if (!_DRM_LOCK_IS_HELD(file_priv->master->lock.hw_lock->lock)) { DRM_ERROR("Lock isn't held after context switch\n"); }
On Thu, Apr 23, 2015 at 03:07:55PM +0100, Peter Antoine wrote:
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.
You've turned an application crash into an application crash... Just with a slightly less verbose kernel log. -Chris
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); 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;
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
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
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
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
As these functions are only used by one driver and there are security holes in these functions. Make the functions optional.
Issue: VIZ-5485 Signed-off-by: Peter Antoine peter.antoine@intel.com --- drivers/gpu/drm/drm_lock.c | 6 ++++++ drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++- include/drm/drmP.h | 23 ++++++++++++----------- include/uapi/drm/i915_drm.h | 1 + 5 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c index 94500930..b61d4c7 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data, struct drm_master *master = file_priv->master; int ret = 0;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + ++file_priv->lock_count;
if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@ -153,6 +156,9 @@ 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 (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { DRM_ERROR("Process %d using kernel context %d\n", task_pid_nr(current), lock->context); diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index e44116f..c771ef0 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data, if (!value) return -ENODEV; break; + case I915_PARAM_HAS_LEGACY_CONTEXT: + value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT); + break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 8763deb..936b423 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = { .driver_features = DRIVER_USE_AGP | - DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER, + DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER | + DRIVER_KMS_LEGACY_CONTEXT,
.load = nouveau_drm_load, .unload = nouveau_drm_unload, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
/***********************************************************************/ /** \name Macros to make printk easier */ diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 4851d66..0ad611a2 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait { #define I915_PARAM_REVISION 32 #define I915_PARAM_SUBSLICE_TOTAL 33 #define I915_PARAM_EU_TOTAL 34 +#define I915_PARAM_HAS_LEGACY_CONTEXT 35
typedef struct drm_i915_getparam { int param;
On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
As these functions are only used by one driver and there are security holes in these functions. Make the functions optional.
Issue: VIZ-5485 Signed-off-by: Peter Antoine peter.antoine@intel.com
drivers/gpu/drm/drm_lock.c | 6 ++++++ drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++- include/drm/drmP.h | 23 ++++++++++++----------- include/uapi/drm/i915_drm.h | 1 + 5 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c index 94500930..b61d4c7 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data, struct drm_master *master = file_priv->master; int ret = 0;
if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;++file_priv->lock_count;
if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
@@ -153,6 +156,9 @@ 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 (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;- if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { DRM_ERROR("Process %d using kernel context %d\n", task_pid_nr(current), lock->context);
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index e44116f..c771ef0 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data, if (!value) return -ENODEV; break;
- case I915_PARAM_HAS_LEGACY_CONTEXT:
value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);break;
Seems pointless to add a parameter that'll always be false.
default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 8763deb..936b423 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = { .driver_features = DRIVER_USE_AGP |
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |DRIVER_KMS_LEGACY_CONTEXT,
Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.
.load = nouveau_drm_load, .unload = nouveau_drm_unload, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
Why is there KMS in the name?
I was thinking just checking for GEM, but I think there was some gem+dri1 userland for i915 at some point in time. ums and dri1 are now dead as far as i915 is concerned, so in theory it should be fine. But I'm not sure if some other driver might have the same baggage.
I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
/***********************************************************************/ /** \name Macros to make printk easier */ diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 4851d66..0ad611a2 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait { #define I915_PARAM_REVISION 32 #define I915_PARAM_SUBSLICE_TOTAL 33 #define I915_PARAM_EU_TOTAL 34 +#define I915_PARAM_HAS_LEGACY_CONTEXT 35
typedef struct drm_i915_getparam { int param; -- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,
(replies inline)
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, April 27, 2015 6:04 PM To: Antoine, Peter Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
As these functions are only used by one driver and there are security holes in these functions. Make the functions optional.
Issue: VIZ-5485 Signed-off-by: Peter Antoine peter.antoine@intel.com
drivers/gpu/drm/drm_lock.c | 6 ++++++ drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++- include/drm/drmP.h | 23 ++++++++++++----------- include/uapi/drm/i915_drm.h | 1 + 5 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c index 94500930..b61d4c7 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data, struct drm_master *master = file_priv->master; int ret = 0;
if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;++file_priv->lock_count;
if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@
-153,6 +156,9 @@ 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 (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;- if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { DRM_ERROR("Process %d using kernel context %d\n", task_pid_nr(current), lock->context); diff --git
a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index e44116f..c771ef0 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data, if (!value) return -ENODEV; break;
- case I915_PARAM_HAS_LEGACY_CONTEXT:
value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);break;
Seems pointless to add a parameter that'll always be false.
There is some history to these changes, the HW_LOCK functions were removed previously but causes an issue with the Nouveau drivers. So that the functions where put back in. So the parameter has been added to allow for that driver to turn the legacy context on as it is needed.
default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 8763deb..936b423 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = { .driver_features = DRIVER_USE_AGP |
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |DRIVER_KMS_LEGACY_CONTEXT,
Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.
See above.
.load = nouveau_drm_load, .unload = nouveau_drm_unload, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
Why is there KMS in the name?
By suggestion of Daniel.
I was thinking just checking for GEM, but I think there was some gem+dri1 userland for i915 at some point in time. ums and dri1 are now dead as far as i915 is concerned, so in theory it should be fine. But I'm not sure if some other driver might have the same baggage.
Other drivers have the same baggage.
I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
Peter.
/********************************************************************* **/ /** \name Macros to make printk easier */ diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 4851d66..0ad611a2 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait { #define I915_PARAM_REVISION 32 #define I915_PARAM_SUBSLICE_TOTAL 33 #define I915_PARAM_EU_TOTAL 34 +#define I915_PARAM_HAS_LEGACY_CONTEXT 35
typedef struct drm_i915_getparam { int param; -- 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
On Tue, Apr 28, 2015 at 05:52:20AM +0000, Antoine, Peter wrote:
Hi,
(replies inline)
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, April 27, 2015 6:04 PM To: Antoine, Peter Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
As these functions are only used by one driver and there are security holes in these functions. Make the functions optional.
Issue: VIZ-5485 Signed-off-by: Peter Antoine peter.antoine@intel.com
drivers/gpu/drm/drm_lock.c | 6 ++++++ drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++- include/drm/drmP.h | 23 ++++++++++++----------- include/uapi/drm/i915_drm.h | 1 + 5 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c index 94500930..b61d4c7 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data, struct drm_master *master = file_priv->master; int ret = 0;
if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;++file_priv->lock_count;
if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@
-153,6 +156,9 @@ 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 (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;- if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { DRM_ERROR("Process %d using kernel context %d\n", task_pid_nr(current), lock->context); diff --git
a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index e44116f..c771ef0 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data, if (!value) return -ENODEV; break;
- case I915_PARAM_HAS_LEGACY_CONTEXT:
value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);break;Seems pointless to add a parameter that'll always be false.
There is some history to these changes, the HW_LOCK functions were removed previously but causes an issue with the Nouveau drivers. So that the functions where put back in. So the parameter has been added to allow for that driver to turn the legacy context on as it is needed.
default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 8763deb..936b423 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = { .driver_features = DRIVER_USE_AGP |
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |DRIVER_KMS_LEGACY_CONTEXT,Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.
See above.
.load = nouveau_drm_load, .unload = nouveau_drm_unload, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
Why is there KMS in the name?
By suggestion of Daniel.
I was thinking just checking for GEM, but I think there was some gem+dri1 userland for i915 at some point in time. ums and dri1 are now dead as far as i915 is concerned, so in theory it should be fine. But I'm not sure if some other driver might have the same baggage.
Other drivers have the same baggage.
I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
Reference?
Peter.
/********************************************************************* **/ /** \name Macros to make printk easier */ diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 4851d66..0ad611a2 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait { #define I915_PARAM_REVISION 32 #define I915_PARAM_SUBSLICE_TOTAL 33 #define I915_PARAM_EU_TOTAL 34 +#define I915_PARAM_HAS_LEGACY_CONTEXT 35
typedef struct drm_i915_getparam { int param; -- 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
reply at end.
On Tue, 2015-04-28 at 13:40 +0300, Ville Syrjälä wrote:
On Tue, Apr 28, 2015 at 05:52:20AM +0000, Antoine, Peter wrote:
Hi,
(replies inline)
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, April 27, 2015 6:04 PM To: Antoine, Peter Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
As these functions are only used by one driver and there are security holes in these functions. Make the functions optional.
Issue: VIZ-5485 Signed-off-by: Peter Antoine peter.antoine@intel.com
drivers/gpu/drm/drm_lock.c | 6 ++++++ drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++- include/drm/drmP.h | 23 ++++++++++++----------- include/uapi/drm/i915_drm.h | 1 + 5 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c index 94500930..b61d4c7 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data, struct drm_master *master = file_priv->master; int ret = 0;
if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;++file_priv->lock_count;
if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@
-153,6 +156,9 @@ 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 (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
return -EINVAL;- if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { DRM_ERROR("Process %d using kernel context %d\n", task_pid_nr(current), lock->context); diff --git
a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index e44116f..c771ef0 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data, if (!value) return -ENODEV; break;
- case I915_PARAM_HAS_LEGACY_CONTEXT:
value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);break;Seems pointless to add a parameter that'll always be false.
There is some history to these changes, the HW_LOCK functions were removed previously but causes an issue with the Nouveau drivers. So that the functions where put back in. So the parameter has been added to allow for that driver to turn the legacy context on as it is needed.
default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 8763deb..936b423 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = { .driver_features = DRIVER_USE_AGP |
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |DRIVER_KMS_LEGACY_CONTEXT,Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.
See above.
.load = nouveau_drm_load, .unload = nouveau_drm_unload, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
Why is there KMS in the name?
By suggestion of Daniel.
I was thinking just checking for GEM, but I think there was some gem+dri1 userland for i915 at some point in time. ums and dri1 are now dead as far as i915 is concerned, so in theory it should be fine. But I'm not sure if some other driver might have the same baggage.
Other drivers have the same baggage.
I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
Reference?
From the next commit [5/5] as it is the one that actually turns off the functions that were turned off before.
These changes are based on the two patches: commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095 Author: Dave Airlie airlied@redhat.com
And the commit that the above patch reverts: commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 Author: Daniel Vetter daniel.vetter@ffwll.ch
Peter.
/********************************************************************* **/ /** \name Macros to make printk easier */ diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 4851d66..0ad611a2 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait { #define I915_PARAM_REVISION 32 #define I915_PARAM_SUBSLICE_TOTAL 33 #define I915_PARAM_EU_TOTAL 34 +#define I915_PARAM_HAS_LEGACY_CONTEXT 35
typedef struct drm_i915_getparam { int param; -- 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
On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
Why is there KMS in the name?
By suggestion of Daniel.
I was thinking just checking for GEM, but I think there was some gem+dri1 userland for i915 at some point in time. ums and dri1 are now dead as far as i915 is concerned, so in theory it should be fine. But I'm not sure if some other driver might have the same baggage.
Other drivers have the same baggage.
I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
Reference?
From the next commit [5/5] as it is the one that actually turns off the functions that were turned off before.
These changes are based on the two patches: commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095 Author: Dave Airlie airlied@redhat.com
And the commit that the above patch reverts: commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 Author: Daniel Vetter daniel.vetter@ffwll.ch
Looking at ancient libdrm sources makes me think nouveau just used to create and destroy the context, but not actually use it for anything. So nopping out the ioctls should be good enough AFAICS. Or am I missing something?
On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote:
On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
Why is there KMS in the name?
By suggestion of Daniel.
I was thinking just checking for GEM, but I think there was some gem+dri1 userland for i915 at some point in time. ums and dri1 are now dead as far as i915 is concerned, so in theory it should be fine. But I'm not sure if some other driver might have the same baggage.
Other drivers have the same baggage.
I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
Reference?
From the next commit [5/5] as it is the one that actually turns off the functions that were turned off before.
These changes are based on the two patches: commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095 Author: Dave Airlie airlied@redhat.com
And the commit that the above patch reverts: commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 Author: Daniel Vetter daniel.vetter@ffwll.ch
Looking at ancient libdrm sources makes me think nouveau just used to create and destroy the context, but not actually use it for anything. So nopping out the ioctls should be good enough AFAICS. Or am I missing something?
An old version of libdrm that still requires support needs them, it's the reason that David Airlie reverted the patch that Daniel did to remove the functions. Do they still need support, I don't know? David Airlie is on the cc list.
A discussion was had and this was the way that it was suggested it be done. This seems a good half-way house, the actual security holes that have been found have been fixed and the functions (that seem to have a lot more security issues in them) are turned off for the drivers that don't use them, and if a driver does require them, it will be a one line change to reintroduce them. Are we carrying code we don't need to support, probably.
Peter.
On Tue, Apr 28, 2015 at 01:29:21PM +0000, Antoine, Peter wrote:
On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote:
On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 62c40777..367e42f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
/* driver capabilities and requirements mask */ -#define DRIVER_USE_AGP 0x1 -#define DRIVER_PCI_DMA 0x8 -#define DRIVER_SG 0x10 -#define DRIVER_HAVE_DMA 0x20 -#define DRIVER_HAVE_IRQ 0x40 -#define DRIVER_IRQ_SHARED 0x80 -#define DRIVER_GEM 0x1000 -#define DRIVER_MODESET 0x2000 -#define DRIVER_PRIME 0x4000 -#define DRIVER_RENDER 0x8000 -#define DRIVER_ATOMIC 0x10000 +#define DRIVER_USE_AGP 0x1 +#define DRIVER_PCI_DMA 0x8 +#define DRIVER_SG 0x10 +#define DRIVER_HAVE_DMA 0x20 +#define DRIVER_HAVE_IRQ 0x40 +#define DRIVER_IRQ_SHARED 0x80 +#define DRIVER_GEM 0x1000 +#define DRIVER_MODESET 0x2000 +#define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000 +#define DRIVER_ATOMIC 0x10000 +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
Why is there KMS in the name?
By suggestion of Daniel.
I was thinking just checking for GEM, but I think there was some gem+dri1 userland for i915 at some point in time. ums and dri1 are now dead as far as i915 is concerned, so in theory it should be fine. But I'm not sure if some other driver might have the same baggage.
Other drivers have the same baggage.
I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
Reference?
From the next commit [5/5] as it is the one that actually turns off the functions that were turned off before.
These changes are based on the two patches: commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095 Author: Dave Airlie airlied@redhat.com
And the commit that the above patch reverts: commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 Author: Daniel Vetter daniel.vetter@ffwll.ch
These two commits definitely should be referenced from the commit message, othwerise no one will understand the history.
Looking at ancient libdrm sources makes me think nouveau just used to create and destroy the context, but not actually use it for anything. So nopping out the ioctls should be good enough AFAICS. Or am I missing something?
An old version of libdrm that still requires support needs them, it's the reason that David Airlie reverted the patch that Daniel did to remove the functions. Do they still need support, I don't know? David Airlie is on the cc list.
A discussion was had and this was the way that it was suggested it be done. This seems a good half-way house, the actual security holes that have been found have been fixed and the functions (that seem to have a lot more security issues in them) are turned off for the drivers that don't use them, and if a driver does require them, it will be a one line change to reintroduce them. Are we carrying code we don't need to support, probably.
Iirc it was in the ddx, and it was actually using the mmap code. Leftovers from ums, but unfortunately X crashes if we take them away. If I recall correctly nouveau was in staging still, but per Linus staging or not doesn't matter when distros are shipping with the code already. I did dig out the details way back out of curiosity, but lost them since then again. -Daniel
Iirc it was in the ddx, and it was actually using the mmap code. Leftovers from ums, but unfortunately X crashes if we take them away. If I recall correctly nouveau was in staging still, but per Linus staging or not doesn't matter when distros are shipping with the code already. I did dig out the details way back out of curiosity, but lost them since then again. -Daniel
There were two different nouveau problems
a) old libdrm used contexts, it called drmCreateContext and drmDestroyContext from what I could see.
b) old DDX used DRI1 functions, DRIOpenDRMMaster in the X server when it wanted to use open, that function added maps with locks and mapped them.
I get the impression this is case (b).
Dave.
As these functions are only used by one driver and there are security holes in these functions. Make the functions optional.
These changes are based on the two patches: commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095 Author: Dave Airlie airlied@redhat.com
And the commit that the above patch reverts: commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1 Author: Daniel Vetter daniel.vetter@ffwll.ch
This should now turn off the context feature.
Issue: VIZ-5485 Signed-off-by: Peter Antoine peter.antoine@intel.com --- drivers/gpu/drm/drm_context.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_drv.c | 12 +++++++----- 2 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c index 1febcd3..09af26c 100644 --- a/drivers/gpu/drm/drm_context.c +++ b/drivers/gpu/drm/drm_context.c @@ -53,6 +53,9 @@ struct drm_ctx_list { */ void drm_legacy_ctxbitmap_free(struct drm_device * dev, int ctx_handle) { + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + mutex_lock(&dev->struct_mutex); idr_remove(&dev->ctx_idr, ctx_handle); mutex_unlock(&dev->struct_mutex); @@ -87,6 +90,9 @@ static int drm_legacy_ctxbitmap_next(struct drm_device * dev) */ int drm_legacy_ctxbitmap_init(struct drm_device * dev) { + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + idr_init(&dev->ctx_idr); return 0; } @@ -101,6 +107,9 @@ int drm_legacy_ctxbitmap_init(struct drm_device * dev) */ void drm_legacy_ctxbitmap_cleanup(struct drm_device * dev) { + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + mutex_lock(&dev->struct_mutex); idr_destroy(&dev->ctx_idr); mutex_unlock(&dev->struct_mutex); @@ -119,6 +128,9 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file) { struct drm_ctx_list *pos, *tmp;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + mutex_lock(&dev->ctxlist_mutex);
list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) { @@ -161,6 +173,9 @@ int drm_legacy_getsareactx(struct drm_device *dev, void *data, struct drm_local_map *map; struct drm_map_list *_entry;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + mutex_lock(&dev->struct_mutex);
map = idr_find(&dev->ctx_idr, request->ctx_id); @@ -205,6 +220,9 @@ int drm_legacy_setsareactx(struct drm_device *dev, void *data, struct drm_local_map *map = NULL; struct drm_map_list *r_list = NULL;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + mutex_lock(&dev->struct_mutex); list_for_each_entry(r_list, &dev->maplist, head) { if (r_list->map @@ -311,6 +329,9 @@ int drm_legacy_resctx(struct drm_device *dev, void *data, struct drm_ctx ctx; int i;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + if (res->count >= DRM_RESERVED_CONTEXTS) { memset(&ctx, 0, sizeof(ctx)); for (i = 0; i < DRM_RESERVED_CONTEXTS; i++) { @@ -341,6 +362,9 @@ int drm_legacy_addctx(struct drm_device *dev, void *data, struct drm_ctx_list *ctx_entry; struct drm_ctx *ctx = data;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + ctx->handle = drm_legacy_ctxbitmap_next(dev); if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) { /* Skip kernel's context and get a new one. */ @@ -384,6 +408,9 @@ int drm_legacy_getctx(struct drm_device *dev, void *data, { struct drm_ctx *ctx = data;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + /* This is 0, because we don't handle any context flags */ ctx->flags = 0;
@@ -406,6 +433,9 @@ int drm_legacy_switchctx(struct drm_device *dev, void *data, { struct drm_ctx *ctx = data;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + DRM_DEBUG("%d\n", ctx->handle); return drm_context_switch(dev, dev->last_context, ctx->handle); } @@ -426,6 +456,9 @@ int drm_legacy_newctx(struct drm_device *dev, void *data, { struct drm_ctx *ctx = data;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + DRM_DEBUG("%d\n", ctx->handle); drm_context_switch_complete(dev, file_priv, ctx->handle);
@@ -448,6 +481,9 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data, { struct drm_ctx *ctx = data;
+ if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + return -EINVAL; + DRM_DEBUG("%d\n", ctx->handle); if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) { if (dev->driver->context_dtor) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 48f7359..dc662e8 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -596,11 +596,13 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, if (drm_ht_create(&dev->map_hash, 12)) goto err_minors;
- ret = drm_legacy_ctxbitmap_init(dev); - if (ret) { - DRM_ERROR("Cannot allocate memory for context bitmap.\n"); - goto err_ht; - } + if (drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT)) + ret = drm_legacy_ctxbitmap_init(dev); + if (ret) { + DRM_ERROR( + "Cannot allocate memory for context bitmap.\n"); + goto err_ht; + }
if (drm_core_check_feature(dev, DRIVER_GEM)) { ret = drm_gem_init(dev);
dri-devel@lists.freedesktop.org
-
Antoine, Peter -
Chris Wilson -
chris＠chris-wilson.co.uk
-
Daniel Vetter -
Dave Airlie -
Dave Gordon -
Peter Antoine
-
Ville Syrjälä