Kernel patch: validate nouveau_channel_get id argument

Kernel patch: validate nouveau_channel_get id argument

[Michel Hermier]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

Hi,
While hacking libdrm I triggered a kernel oups due to a non checked
argument from user land.
In nouveau_ioctl_notifier_alloc, nouveau_channel_get is invoked, but
it doesn't validate the na->channel input argument. The attached patch
validates the channel index, and change it's type to uint32_t since it
is an index after all.

Cheers,
    Michel

[-- Attachment #2: 0001-Fix-channel-nouveau_channel_get-index-type-and-check.patch --]
[-- Type: application/octet-stream, Size: 2042 bytes --]

From dc00e5ccce3f10e51ae143d6dda6aa8febab271d Mon Sep 17 00:00:00 2001
From: Michel Hermier <hermier@frugalware.org>
Date: Fri, 24 Dec 2010 14:49:13 +0100
Subject: [PATCH] Fix channel nouveau_channel_get index type and check it's value.

---
 drivers/gpu/drm/nouveau/nouveau_channel.c |    5 ++++-
 drivers/gpu/drm/nouveau/nouveau_drv.h     |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c b/drivers/gpu/drm/nouveau/nouveau_channel.c
index e37977d..bc07a61 100644
--- a/drivers/gpu/drm/nouveau/nouveau_channel.c
+++ b/drivers/gpu/drm/nouveau/nouveau_channel.c
@@ -247,12 +247,15 @@ nouveau_channel_get_unlocked(struct nouveau_channel *ref)
 }
 
 struct nouveau_channel *
-nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, int id)
+nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, uint32_t id)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
 	struct nouveau_channel *chan;
 	unsigned long flags;
 
+	if (unlikely(id >= NOUVEAU_MAX_CHANNEL_NR))
+		return ERR_PTR(-EINVAL);
+
 	spin_lock_irqsave(&dev_priv->channels.lock, flags);
 	chan = nouveau_channel_get_unlocked(dev_priv->channels.ptr[id]);
 	spin_unlock_irqrestore(&dev_priv->channels.lock, flags);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index e815756..ec3eed2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -870,7 +870,7 @@ extern int  nouveau_channel_alloc(struct drm_device *dev,
 extern struct nouveau_channel *
 nouveau_channel_get_unlocked(struct nouveau_channel *);
 extern struct nouveau_channel *
-nouveau_channel_get(struct drm_device *, struct drm_file *, int id);
+nouveau_channel_get(struct drm_device *, struct drm_file *, uint32_t id);
 extern void nouveau_channel_put_unlocked(struct nouveau_channel **);
 extern void nouveau_channel_put(struct nouveau_channel **);
 extern void nouveau_channel_ref(struct nouveau_channel *chan,
-- 
1.7.3.4


[-- Attachment #3: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: Kernel patch: validate nouveau_channel_get id argument

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 3380 bytes --]

Michel Hermier <michel.hermier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> Hi,
> While hacking libdrm I triggered a kernel oups due to a non checked
> argument from user land.
> In nouveau_ioctl_notifier_alloc, nouveau_channel_get is invoked, but
> it doesn't validate the na->channel input argument. The attached patch
> validates the channel index, and change it's type to uint32_t since it
> is an index after all.
>
Thank you, some minor comments inline.

> Cheers,
>     Michel
>
> From dc00e5ccce3f10e51ae143d6dda6aa8febab271d Mon Sep 17 00:00:00 2001
> From: Michel Hermier <hermier-SjT/4EORnmY+uJoB2kUjGw@public.gmane.org>
> Date: Fri, 24 Dec 2010 14:49:13 +0100
> Subject: [PATCH] Fix channel nouveau_channel_get index type and check it's value.

We usually prefix our kernel commit messages with "drm/nouveau: " or
something similar, to tell them apart from the huge kernel commit
flow. Also you made a small typo in "it's".

> 
"Signed-off-by" line missing. You should have a look at
"Documentation/SubmittingPatches" and "Documentation/CodingStyle", if
you haven't already.

> ---
>  drivers/gpu/drm/nouveau/nouveau_channel.c |    5 ++++-
>  drivers/gpu/drm/nouveau/nouveau_drv.h     |    2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c b/drivers/gpu/drm/nouveau/nouveau_channel.c
> index e37977d..bc07a61 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_channel.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_channel.c
> @@ -247,12 +247,15 @@ nouveau_channel_get_unlocked(struct nouveau_channel *ref)
>  }
>  
>  struct nouveau_channel *
> -nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, int id)
> +nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, uint32_t id)
This goes above the 80 column limit. Anyway I'd leave this line alone,
we're already using ints as channel indices in most places.

>  {
>  	struct drm_nouveau_private *dev_priv = dev->dev_private;
>  	struct nouveau_channel *chan;
>  	unsigned long flags;
>  
> +	if (unlikely(id >= NOUVEAU_MAX_CHANNEL_NR))
> +		return ERR_PTR(-EINVAL);
> +
>  	spin_lock_irqsave(&dev_priv->channels.lock, flags);
>  	chan = nouveau_channel_get_unlocked(dev_priv->channels.ptr[id]);
>  	spin_unlock_irqrestore(&dev_priv->channels.lock, flags);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index e815756..ec3eed2 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -870,7 +870,7 @@ extern int  nouveau_channel_alloc(struct drm_device *dev,
>  extern struct nouveau_channel *
>  nouveau_channel_get_unlocked(struct nouveau_channel *);
>  extern struct nouveau_channel *
> -nouveau_channel_get(struct drm_device *, struct drm_file *, int id);
> +nouveau_channel_get(struct drm_device *, struct drm_file *, uint32_t id);
>  extern void nouveau_channel_put_unlocked(struct nouveau_channel **);
>  extern void nouveau_channel_put(struct nouveau_channel **);
>  extern void nouveau_channel_ref(struct nouveau_channel *chan,
> -- 
> 1.7.3.4

> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: Type: application/pgp-signature, Size: 229 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: Kernel patch: validate nouveau_channel_get id argument

[Michel Hermier]

Feel free to correct the patch as you need/want, It is so simple and
tricky, that I don't really care my rights for a 2 liner patch ;) If
you really insist, I'll prepare the patch following the guide lines.

2010/12/25 Francisco Jerez <currojerez@riseup.net>:
> Michel Hermier <michel.hermier@gmail.com> writes:
>
>> Hi,
>> While hacking libdrm I triggered a kernel oups due to a non checked
>> argument from user land.
>> In nouveau_ioctl_notifier_alloc, nouveau_channel_get is invoked, but
>> it doesn't validate the na->channel input argument. The attached patch
>> validates the channel index, and change it's type to uint32_t since it
>> is an index after all.
>>
> Thank you, some minor comments inline.
>
>> Cheers,
>>     Michel
>>
>> From dc00e5ccce3f10e51ae143d6dda6aa8febab271d Mon Sep 17 00:00:00 2001
>> From: Michel Hermier <hermier@frugalware.org>
>> Date: Fri, 24 Dec 2010 14:49:13 +0100
>> Subject: [PATCH] Fix channel nouveau_channel_get index type and check it's value.
>
> We usually prefix our kernel commit messages with "drm/nouveau: " or
> something similar, to tell them apart from the huge kernel commit
> flow. Also you made a small typo in "it's".
>
>>
> "Signed-off-by" line missing. You should have a look at
> "Documentation/SubmittingPatches" and "Documentation/CodingStyle", if
> you haven't already.
>
>> ---
>>  drivers/gpu/drm/nouveau/nouveau_channel.c |    5 ++++-
>>  drivers/gpu/drm/nouveau/nouveau_drv.h     |    2 +-
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c b/drivers/gpu/drm/nouveau/nouveau_channel.c
>> index e37977d..bc07a61 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_channel.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_channel.c
>> @@ -247,12 +247,15 @@ nouveau_channel_get_unlocked(struct nouveau_channel *ref)
>>  }
>>
>>  struct nouveau_channel *
>> -nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, int id)
>> +nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, uint32_t id)
> This goes above the 80 column limit. Anyway I'd leave this line alone,
> we're already using ints as channel indices in most places.
>
>>  {
>>       struct drm_nouveau_private *dev_priv = dev->dev_private;
>>       struct nouveau_channel *chan;
>>       unsigned long flags;
>>
>> +     if (unlikely(id >= NOUVEAU_MAX_CHANNEL_NR))
>> +             return ERR_PTR(-EINVAL);
>> +
>>       spin_lock_irqsave(&dev_priv->channels.lock, flags);
>>       chan = nouveau_channel_get_unlocked(dev_priv->channels.ptr[id]);
>>       spin_unlock_irqrestore(&dev_priv->channels.lock, flags);
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
>> index e815756..ec3eed2 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
>> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
>> @@ -870,7 +870,7 @@ extern int  nouveau_channel_alloc(struct drm_device *dev,
>>  extern struct nouveau_channel *
>>  nouveau_channel_get_unlocked(struct nouveau_channel *);
>>  extern struct nouveau_channel *
>> -nouveau_channel_get(struct drm_device *, struct drm_file *, int id);
>> +nouveau_channel_get(struct drm_device *, struct drm_file *, uint32_t id);
>>  extern void nouveau_channel_put_unlocked(struct nouveau_channel **);
>>  extern void nouveau_channel_put(struct nouveau_channel **);
>>  extern void nouveau_channel_ref(struct nouveau_channel *chan,
>> --
>> 1.7.3.4
>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: Kernel patch: validate nouveau_channel_get id argument

[Michel Hermier]

[-- Attachment #1: Type: text/plain, Size: 64 bytes --]

Second try ^^ I'm not used at all at preparing patch for kernel

[-- Attachment #2: 0001-drm-nouveau-Fix-potential-crash-in-NOUVEAU_NOTIFIERO.patch --]
[-- Type: application/octet-stream, Size: 2392 bytes --]

From 451c532b878f38513db795ee8eea6bf25376c726 Mon Sep 17 00:00:00 2001
From: Michel Hermier <hermier@frugalware.org>
Date: Sat, 25 Dec 2010 16:58:56 +0100
Subject: [PATCH] drm/nouveau: Fix potential crash in NOUVEAU_NOTIFIEROBJ_ALLOC ioctl.

When hacking the libdrm for improvements, I triggered a kernel crash
related to the fact that the NOUVEAU_NOTIFIEROBJ_ALLOC ioctl call
nouveau_channel_get with an unchecked channel index.
The patch ensure that the channel index is an unsigned and validate
its value in nouveau_channel_get.

Signed-off-by: Michel Hermier <hermier@frugalware.org>
---
 drivers/gpu/drm/nouveau/nouveau_channel.c |    5 ++++-
 drivers/gpu/drm/nouveau/nouveau_drv.h     |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c b/drivers/gpu/drm/nouveau/nouveau_channel.c
index e37977d..bc07a61 100644
--- a/drivers/gpu/drm/nouveau/nouveau_channel.c
+++ b/drivers/gpu/drm/nouveau/nouveau_channel.c
@@ -247,12 +247,15 @@ nouveau_channel_get_unlocked(struct nouveau_channel *ref)
 }
 
 struct nouveau_channel *
-nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, int id)
+nouveau_channel_get(struct drm_device *dev, struct drm_file *file_priv, uint32_t id)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
 	struct nouveau_channel *chan;
 	unsigned long flags;
 
+	if (unlikely(id >= NOUVEAU_MAX_CHANNEL_NR))
+		return ERR_PTR(-EINVAL);
+
 	spin_lock_irqsave(&dev_priv->channels.lock, flags);
 	chan = nouveau_channel_get_unlocked(dev_priv->channels.ptr[id]);
 	spin_unlock_irqrestore(&dev_priv->channels.lock, flags);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index e815756..ec3eed2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -870,7 +870,7 @@ extern int  nouveau_channel_alloc(struct drm_device *dev,
 extern struct nouveau_channel *
 nouveau_channel_get_unlocked(struct nouveau_channel *);
 extern struct nouveau_channel *
-nouveau_channel_get(struct drm_device *, struct drm_file *, int id);
+nouveau_channel_get(struct drm_device *, struct drm_file *, uint32_t id);
 extern void nouveau_channel_put_unlocked(struct nouveau_channel **);
 extern void nouveau_channel_put(struct nouveau_channel **);
 extern void nouveau_channel_ref(struct nouveau_channel *chan,
-- 
1.7.3.4


[-- Attachment #3: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: Kernel patch: validate nouveau_channel_get id argument

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 201 bytes --]

Michel Hermier <michel.hermier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> Second try ^^ I'm not used at all at preparing patch for kernel

Thanks. I've pushed it with a couple of minor fixes.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 229 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: Kernel patch: validate nouveau_channel_get id argument

[Michel Hermier]

For my culture, why did you not changed the type to uint32_t like in
my original patch ?
Also looking at include/drm/nouveau_drm.h, I see some inconsistant
type for channel in ioctls structures ?

2010/12/25 Francisco Jerez <currojerez-sGOZH3hwPm2sTnJN9+BGXg@public.gmane.org>:
> Michel Hermier <michel.hermier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>
>> Second try ^^ I'm not used at all at preparing patch for kernel
>
> Thanks. I've pushed it with a couple of minor fixes.
>

Re: Kernel patch: validate nouveau_channel_get id argument

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 879 bytes --]

Michel Hermier <michel.hermier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> For my culture, why did you not changed the type to uint32_t like in
> my original patch ?
As I told you before, those changes went over the 80 column limit so I
just left them out.  I hope you don't mind, the modified patch should do
the same thing as the original one for all practical purposes.

> Also looking at include/drm/nouveau_drm.h, I see some inconsistant
> type for channel in ioctls structures ?
>
Right, explicitly sized integers are usually the preferred choice in the
IOCTL API.

> 2010/12/25 Francisco Jerez <currojerez-sGOZH3hwPm2sTnJN9+BGXg@public.gmane.org>:
>> Michel Hermier <michel.hermier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>
>>> Second try ^^ I'm not used at all at preparing patch for kernel
>>
>> Thanks. I've pushed it with a couple of minor fixes.
>>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 229 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: Kernel patch: validate nouveau_channel_get id argument

[Michel Hermier]

2010/12/26 Francisco Jerez <currojerez@riseup.net>:
> Michel Hermier <michel.hermier@gmail.com> writes:
>
>> For my culture, why did you not changed the type to uint32_t like in
>> my original patch ?
> As I told you before, those changes went over the 80 column limit so I
> just left them out.  I hope you don't mind, the modified patch should do
> the same thing as the original one for all practical purposes.

I don't really care since it fix the issue, but for long term I think we should
change theses to uint32_t

>> Also looking at include/drm/nouveau_drm.h, I see some inconsistant
>> type for channel in ioctls structures ?
>>
> Right, explicitly sized integers are usually the preferred choice in the
> IOCTL API.

Changing them to uint32_t would require an PATCHLEVEL++ even if the
valid values where uint32_t ??
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau