* [PATCH] drm: Fix authentication kernel crash
@ 2012-01-24 9:31 Thomas Hellstrom
2012-01-24 14:47 ` Daniel Vetter
2012-01-24 17:12 ` Greg KH
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Hellstrom @ 2012-01-24 9:31 UTC (permalink / raw
To: airlied; +Cc: airlied, Thomas Hellstrom, stable, dri-devel
If the master tries to authenticate a client using drm_authmagic and
that client has already closed its drm file descriptor,
either wilfully or because it was terminated, the
call to drm_authmagic will dereference a stale pointer into kmalloc'ed memory
and corrupt it.
Typically this results in a hard system hang.
This patch fixes that problem by removing any authentication tokens
(struct drm_magic_entry) open for a file descriptor when that file
descriptor is closed.
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
Please review. This should also go into stable kernels.
drivers/gpu/drm/drm_auth.c | 6 +++++-
drivers/gpu/drm/drm_fops.c | 5 +++++
include/drm/drmP.h | 1 +
3 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 3f46772..ba23790 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -101,7 +101,7 @@ static int drm_add_magic(struct drm_master *master, struct drm_file *priv,
* Searches and unlinks the entry in drm_device::magiclist with the magic
* number hash key, while holding the drm_device::struct_mutex lock.
*/
-static int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
+int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
{
struct drm_magic_entry *pt;
struct drm_hash_item *hash;
@@ -136,6 +136,8 @@ static int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
* If there is a magic number in drm_file::magic then use it, otherwise
* searches an unique non-zero magic number and add it associating it with \p
* file_priv.
+ * This ioctl needs protection by the drm_global_mutex, which protects
+ * struct drm_file::magic and struct drm_magic_entry::priv.
*/
int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
{
@@ -173,6 +175,8 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
* \return zero if authentication successed, or a negative number otherwise.
*
* Checks if \p file_priv is associated with the magic number passed in \arg.
+ * This ioctl needs protection by the drm_global_mutex, which protects
+ * struct drm_file::magic and struct drm_magic_entry::priv.
*/
int drm_authmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c00cf15..6263b01 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -487,6 +487,11 @@ int drm_release(struct inode *inode, struct file *filp)
(long)old_encode_dev(file_priv->minor->device),
dev->open_count);
+ /* Release any auth tokens that might point to this file_priv,
+ (do that under the drm_global_mutex) */
+ if (file_priv->magic)
+ (void) drm_remove_magic(file_priv->master, file_priv->magic);
+
/* if the master has gone away we can't do anything with the lock */
if (file_priv->minor->master)
drm_master_release(dev, filp);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 76caa67..92f0981 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1328,6 +1328,7 @@ extern int drm_getmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv);
extern int drm_authmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+extern int drm_remove_magic(struct drm_master *master, drm_magic_t magic);
/* Cache management (drm_cache.c) */
void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Fix authentication kernel crash
2012-01-24 9:31 Thomas Hellstrom
@ 2012-01-24 14:47 ` Daniel Vetter
2012-01-25 17:26 ` Thomas Hellstrom
2012-01-24 17:12 ` Greg KH
1 sibling, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2012-01-24 14:47 UTC (permalink / raw
To: Thomas Hellstrom; +Cc: airlied, dri-devel, stable
On Tue, Jan 24, 2012 at 10:31:46AM +0100, Thomas Hellstrom wrote:
> If the master tries to authenticate a client using drm_authmagic and
> that client has already closed its drm file descriptor,
> either wilfully or because it was terminated, the
> call to drm_authmagic will dereference a stale pointer into kmalloc'ed memory
> and corrupt it.
>
> Typically this results in a hard system hang.
>
> This patch fixes that problem by removing any authentication tokens
> (struct drm_magic_entry) open for a file descriptor when that file
> descriptor is closed.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Ok, I've wandered around a bit in this and noticed that the locking is the
usual convoluted disaster. We seem to randomly grab dev->struct_mutex in
the auth and master ioctl, but all the real protect seems to be due to
taking the global mutex in all relevant paths.
I guess I can't volunteer you to clean this up ;-)
Otherwise I couldn't poke a hole into this, so
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ----
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Fix authentication kernel crash
2012-01-24 9:31 Thomas Hellstrom
2012-01-24 14:47 ` Daniel Vetter
@ 2012-01-24 17:12 ` Greg KH
1 sibling, 0 replies; 5+ messages in thread
From: Greg KH @ 2012-01-24 17:12 UTC (permalink / raw
To: Thomas Hellstrom; +Cc: airlied, stable, dri-devel
On Tue, Jan 24, 2012 at 10:31:46AM +0100, Thomas Hellstrom wrote:
> If the master tries to authenticate a client using drm_authmagic and
> that client has already closed its drm file descriptor,
> either wilfully or because it was terminated, the
> call to drm_authmagic will dereference a stale pointer into kmalloc'ed memory
> and corrupt it.
>
> Typically this results in a hard system hang.
>
> This patch fixes that problem by removing any authentication tokens
> (struct drm_magic_entry) open for a file descriptor when that file
> descriptor is closed.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
> Please review. This should also go into stable kernels.
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm: Fix authentication kernel crash
@ 2012-01-24 17:54 Thomas Hellstrom
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Hellstrom @ 2012-01-24 17:54 UTC (permalink / raw
To: airlied; +Cc: dri-devel
If the master tries to authenticate a client using drm_authmagic and
that client has already closed its drm file descriptor,
either wilfully or because it was terminated, the
call to drm_authmagic will dereference a stale pointer into kmalloc'ed memory
and corrupt it.
Typically this results in a hard system hang.
This patch fixes that problem by removing any authentication tokens
(struct drm_magic_entry) open for a file descriptor when that file
descriptor is closed.
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
Resent with Review-by and Cc added.
drivers/gpu/drm/drm_auth.c | 6 +++++-
drivers/gpu/drm/drm_fops.c | 5 +++++
include/drm/drmP.h | 1 +
3 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 3f46772..ba23790 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -101,7 +101,7 @@ static int drm_add_magic(struct drm_master *master, struct drm_file *priv,
* Searches and unlinks the entry in drm_device::magiclist with the magic
* number hash key, while holding the drm_device::struct_mutex lock.
*/
-static int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
+int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
{
struct drm_magic_entry *pt;
struct drm_hash_item *hash;
@@ -136,6 +136,8 @@ static int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
* If there is a magic number in drm_file::magic then use it, otherwise
* searches an unique non-zero magic number and add it associating it with \p
* file_priv.
+ * This ioctl needs protection by the drm_global_mutex, which protects
+ * struct drm_file::magic and struct drm_magic_entry::priv.
*/
int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
{
@@ -173,6 +175,8 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
* \return zero if authentication successed, or a negative number otherwise.
*
* Checks if \p file_priv is associated with the magic number passed in \arg.
+ * This ioctl needs protection by the drm_global_mutex, which protects
+ * struct drm_file::magic and struct drm_magic_entry::priv.
*/
int drm_authmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c00cf15..6263b01 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -487,6 +487,11 @@ int drm_release(struct inode *inode, struct file *filp)
(long)old_encode_dev(file_priv->minor->device),
dev->open_count);
+ /* Release any auth tokens that might point to this file_priv,
+ (do that under the drm_global_mutex) */
+ if (file_priv->magic)
+ (void) drm_remove_magic(file_priv->master, file_priv->magic);
+
/* if the master has gone away we can't do anything with the lock */
if (file_priv->minor->master)
drm_master_release(dev, filp);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 76caa67..92f0981 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1328,6 +1328,7 @@ extern int drm_getmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv);
extern int drm_authmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+extern int drm_remove_magic(struct drm_master *master, drm_magic_t magic);
/* Cache management (drm_cache.c) */
void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Fix authentication kernel crash
2012-01-24 14:47 ` Daniel Vetter
@ 2012-01-25 17:26 ` Thomas Hellstrom
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Hellstrom @ 2012-01-25 17:26 UTC (permalink / raw
To: Daniel Vetter; +Cc: dri-devel
On 01/24/2012 03:47 PM, Daniel Vetter wrote:
> On Tue, Jan 24, 2012 at 10:31:46AM +0100, Thomas Hellstrom wrote:
>> If the master tries to authenticate a client using drm_authmagic and
>> that client has already closed its drm file descriptor,
>> either wilfully or because it was terminated, the
>> call to drm_authmagic will dereference a stale pointer into kmalloc'ed memory
>> and corrupt it.
>>
>> Typically this results in a hard system hang.
>>
>> This patch fixes that problem by removing any authentication tokens
>> (struct drm_magic_entry) open for a file descriptor when that file
>> descriptor is closed.
>>
>> Signed-off-by: Thomas Hellstrom<thellstrom@vmware.com>
> Ok, I've wandered around a bit in this and noticed that the locking is the
> usual convoluted disaster. We seem to randomly grab dev->struct_mutex in
> the auth and master ioctl, but all the real protect seems to be due to
> taking the global mutex in all relevant paths.
>
> I guess I can't volunteer you to clean this up ;-)
It would be pretty easy to make thos ioctls unlocked (we should probably
also have an idr managing the magic number)
but my wife is having twins in a couple of weeks and
I've got a long list of bugs to fix before that for the vmwgfx launch so I
unfortunately have to pass this time.
/Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-01-25 17:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-24 17:54 [PATCH] drm: Fix authentication kernel crash Thomas Hellstrom
-- strict thread matches above, loose matches on Subject: below --
2012-01-24 9:31 Thomas Hellstrom
2012-01-24 14:47 ` Daniel Vetter
2012-01-25 17:26 ` Thomas Hellstrom
2012-01-24 17:12 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).