Re: Kernel patch: validate nouveau_channel_get id argument

[Francisco Jerez <currojerez-sGOZH3hwPm2sTnJN9+BGXg@public.gmane.org>]

[-- 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