Nouveau - Aug 2013 - [PATCH] drm/nouveau: avoid null deref on bad arguments to nouveau_vma_getmap

The code expects non-VRAM mem nodes to have a pages list. If that's not
set, it will do a null deref down the line. Warn on that condition and
return an error.

See https://bugs.freedesktop.org/show_bug.cgi?id=64774

Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
Cc: <stable at vger.kernel.org> # 3.8+
---

I don't exactly understand what's going on, but this is just a
straightforward way to avoid a null deref that you see happens in the
bug. I haven't figured out the root cause of this, but it's getting
well into the "I have no idea how TTM works" space. However this seems
like a bit of defensive programming -- nouveau_vm_map_sg will pass
node->pages as a list down, which will be dereferenced by
nvc0_vm_map_sg. Perhaps the other arguments should make that
dereferencing not happen, but it definitely was happening here, as you
can see in the bug.

Ben/Maarten, I'll let you judge whether this check is appropriate,
since like I hope I was able to convey above, I'm just not really sure :)

 drivers/gpu/drm/nouveau/nouveau_bo.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index cdc3282..191145d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -963,6 +963,12 @@ nouveau_vma_getmap(struct nouveau_channel *chan, struct
nouveau_bo *nvbo,
 	struct nouveau_mem *node = mem->mm_node;
 	int ret;
 
+	/* If we ever get here for a non-vram mem node that doesn't
+	 * have pages, we will end up doing a null deref in
+	 * nouveau_vm_map_sg. */
+	if (WARN_ON(mem->mem_type != TTM_PL_VRAM && !node->pages))
+		return -EINVAL;
+
 	ret = nouveau_vm_get(nv_client(chan->cli)->vm, mem->num_pages
<<
 			     PAGE_SHIFT, node->page_shift,
 			     NV_MEM_ACCESS_RW, vma);
-- 
1.8.1.5

On Thu, Aug 22, 2013 at 10:10 AM, Ilia Mirkin <imirkin at alum.mit.edu>
wrote:
> The code expects non-VRAM mem nodes to have a pages list. If that's not
> set, it will do a null deref down the line. Warn on that condition and
> return an error.
>
> See https://bugs.freedesktop.org/show_bug.cgi?id=64774
>
> Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
> Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: <stable at vger.kernel.org> # 3.8+
> ---
>
> I don't exactly understand what's going on, but this is just a
> straightforward way to avoid a null deref that you see happens in the
> bug. I haven't figured out the root cause of this, but it's getting
> well into the "I have no idea how TTM works" space. However this
seems
> like a bit of defensive programming -- nouveau_vm_map_sg will pass
> node->pages as a list down, which will be dereferenced by
> nvc0_vm_map_sg. Perhaps the other arguments should make that
> dereferencing not happen, but it definitely was happening here, as you
> can see in the bug.
>
> Ben/Maarten, I'll let you judge whether this check is appropriate,
> since like I hope I was able to convey above, I'm just not really sure
:)
>
>  drivers/gpu/drm/nouveau/nouveau_bo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index cdc3282..191145d 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -963,6 +963,12 @@ nouveau_vma_getmap(struct nouveau_channel *chan,
struct nouveau_bo *nvbo,
>         struct nouveau_mem *node = mem->mm_node;
>         int ret;
>
> +       /* If we ever get here for a non-vram mem node that doesn't
> +        * have pages, we will end up doing a null deref in
> +        * nouveau_vm_map_sg. */
> +       if (WARN_ON(mem->mem_type != TTM_PL_VRAM &&
!node->pages))
> +               return -EINVAL;
My guess here is that this is a mapping that requires the use of
map_sg_table() (see nouveau_bo_move_ntfy() for the condition).

I'm not entirely sure this should even be happening to be honest.  I
guess TTM is trying to move a shared buffer from GART to VRAM for some
reason (userspace probably asked for it?).. And well, this really
shouldn't be allowed.. The other device won't be able to touch it
then.

If you can confirm this is indeed what's happening, we should find out
why and fix it (and have the kernel completely reject such attempts).

Ben.
> +
>         ret = nouveau_vm_get(nv_client(chan->cli)->vm,
mem->num_pages <<
>                              PAGE_SHIFT, node->page_shift,
>                              NV_MEM_ACCESS_RW, vma);
> --
> 1.8.1.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Thu, Aug 22, 2013 at 04:41:06PM +1000, Ben Skeggs
wrote:
> On Thu, Aug 22, 2013 at 10:10 AM, Ilia Mirkin <imirkin at
alum.mit.edu> wrote:
> > The code expects non-VRAM mem nodes to have a pages list. If
that's not
> > set, it will do a null deref down the line. Warn on that condition and
> > return an error.
> >
> > See https://bugs.freedesktop.org/show_bug.cgi?id=64774
> >
> > Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
> > Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
> > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > Cc: <stable at vger.kernel.org> # 3.8+
> > ---
> >
> > I don't exactly understand what's going on, but this is just a
> > straightforward way to avoid a null deref that you see happens in the
> > bug. I haven't figured out the root cause of this, but it's
getting
> > well into the "I have no idea how TTM works" space. However
this seems
> > like a bit of defensive programming -- nouveau_vm_map_sg will pass
> > node->pages as a list down, which will be dereferenced by
> > nvc0_vm_map_sg. Perhaps the other arguments should make that
> > dereferencing not happen, but it definitely was happening here, as you
> > can see in the bug.
> >
> > Ben/Maarten, I'll let you judge whether this check is appropriate,
> > since like I hope I was able to convey above, I'm just not really
sure :)
> >
> >  drivers/gpu/drm/nouveau/nouveau_bo.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
> > index cdc3282..191145d 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> > @@ -963,6 +963,12 @@ nouveau_vma_getmap(struct nouveau_channel *chan,
struct nouveau_bo *nvbo,
> >         struct nouveau_mem *node = mem->mm_node;
> >         int ret;
> >
> > +       /* If we ever get here for a non-vram mem node that
doesn't
> > +        * have pages, we will end up doing a null deref in
> > +        * nouveau_vm_map_sg. */
> > +       if (WARN_ON(mem->mem_type != TTM_PL_VRAM &&
!node->pages))
> > +               return -EINVAL;
> My guess here is that this is a mapping that requires the use of
> map_sg_table() (see nouveau_bo_move_ntfy() for the condition).
> 
> I'm not entirely sure this should even be happening to be honest.  I
> guess TTM is trying to move a shared buffer from GART to VRAM for some
> reason (userspace probably asked for it?).. And well, this really
> shouldn't be allowed.. The other device won't be able to touch it
> then.
> 
> If you can confirm this is indeed what's happening, we should find out
> why and fix it (and have the kernel completely reject such attempts).
>
Yes it does happen. I've been experiencing the kernel crash with Linux
3.8.x, 3.9.x and 3.10.x.

I'm able to reproduce the crash when having Optimus enabled in BIOS on my 
Lenovo T430 laptop with Intel IGD + Nvidia GF108 GPU, booting to Xorg desktop, 
and when I try to enable external DVI monitor connected to nouveau card, the
kernel crashes hard..

crash traceback and WARN_ON() tracebacks with this patch applied available in
the bugzilla entry:
https://bugs.freedesktop.org/show_bug.cgi?id=64774


-- Pasi

> Ben.
> 
> > +
> >         ret = nouveau_vm_get(nv_client(chan->cli)->vm,
mem->num_pages <<
> >                              PAGE_SHIFT, node->page_shift,
> >                              NV_MEM_ACCESS_RW, vma);
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

Op 22-08-13 02:10, Ilia Mirkin schreef:
> The code expects non-VRAM mem nodes to have a pages list. If that's not
> set, it will do a null deref down the line. Warn on that condition and
> return an error.
>
> See https://bugs.freedesktop.org/show_bug.cgi?id=64774
>
> Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
> Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: <stable at vger.kernel.org> # 3.8+
> ---
>
> I don't exactly understand what's going on, but this is just a
> straightforward way to avoid a null deref that you see happens in the
> bug. I haven't figured out the root cause of this, but it's getting
> well into the "I have no idea how TTM works" space. However this
seems
> like a bit of defensive programming -- nouveau_vm_map_sg will pass
> node->pages as a list down, which will be dereferenced by
> nvc0_vm_map_sg. Perhaps the other arguments should make that
> dereferencing not happen, but it definitely was happening here, as you
> can see in the bug.
>
> Ben/Maarten, I'll let you judge whether this check is appropriate,
> since like I hope I was able to convey above, I'm just not really sure
:)
Not it really isn't appropriate..

You'd have to call call nouveau_vm_map_sg_table instead, the only place that
doesn't handle that correctly
is where it's not expected to be called.

Here, have a completely untested patch to fix things...

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -138,17 +143,26 @@ nouveau_user_framebuffer_create(struct drm_device *dev,
 {
 	struct nouveau_framebuffer *nouveau_fb;
 	struct drm_gem_object *gem;
+	struct nouveau_bo *nvbo;
 	int ret = -ENOMEM;
 
 	gem = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[0]);
 	if (!gem)
 		return ERR_PTR(-ENOENT);
 
+	nvbo = nouveau_gem_object(gem);
+	if (!(nvbo->valid_domains & NOUVEAU_GEM_DOMAIN_VRAM)) {
+		nv_warn(nouveau_drm(dev), "Trying to create a fb in vram with"
+			" valid_domains=%08x\n", nvbo->valid_domains);
+		ret = -EINVAL;
+		goto err_unref;
+	}
+
 	nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer), GFP_KERNEL);
 	if (!nouveau_fb)
 		goto err_unref;
 
-	ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd,
nouveau_gem_object(gem));
+	ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd, nvbo);
 	if (ret)
 		goto err;

On Thu, Aug 22, 2013 at 09:12:40AM +0200, Maarten Lankhorst
wrote:
> Op 22-08-13 02:10, Ilia Mirkin schreef:
> > The code expects non-VRAM mem nodes to have a pages list. If
that's not
> > set, it will do a null deref down the line. Warn on that condition and
> > return an error.
> >
> > See https://bugs.freedesktop.org/show_bug.cgi?id=64774
> >
> > Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
> > Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
> > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > Cc: <stable at vger.kernel.org> # 3.8+
> > ---
> >
> > I don't exactly understand what's going on, but this is just a
> > straightforward way to avoid a null deref that you see happens in the
> > bug. I haven't figured out the root cause of this, but it's
getting
> > well into the "I have no idea how TTM works" space. However
this seems
> > like a bit of defensive programming -- nouveau_vm_map_sg will pass
> > node->pages as a list down, which will be dereferenced by
> > nvc0_vm_map_sg. Perhaps the other arguments should make that
> > dereferencing not happen, but it definitely was happening here, as you
> > can see in the bug.
> >
> > Ben/Maarten, I'll let you judge whether this check is appropriate,
> > since like I hope I was able to convey above, I'm just not really
sure :)
> Not it really isn't appropriate..
> 
> You'd have to call call nouveau_vm_map_sg_table instead, the only place
that doesn't handle that correctly
> is where it's not expected to be called.
> 
> Here, have a completely untested patch to fix things...
>
Thanks! I'll give it a try later today.. 


-- Pasi
 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -138,17 +143,26 @@ nouveau_user_framebuffer_create(struct drm_device
*dev,
>  {
>  	struct nouveau_framebuffer *nouveau_fb;
>  	struct drm_gem_object *gem;
> +	struct nouveau_bo *nvbo;
>  	int ret = -ENOMEM;
>  
>  	gem = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[0]);
>  	if (!gem)
>  		return ERR_PTR(-ENOENT);
>  
> +	nvbo = nouveau_gem_object(gem);
> +	if (!(nvbo->valid_domains & NOUVEAU_GEM_DOMAIN_VRAM)) {
> +		nv_warn(nouveau_drm(dev), "Trying to create a fb in vram with"
> +			" valid_domains=%08x\n", nvbo->valid_domains);
> +		ret = -EINVAL;
> +		goto err_unref;
> +	}
> +
>  	nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer), GFP_KERNEL);
>  	if (!nouveau_fb)
>  		goto err_unref;
>  
> -	ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd,
nouveau_gem_object(gem));
> +	ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd, nvbo);
>  	if (ret)
>  		goto err;
>  
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Thu, Aug 22, 2013 at 09:12:40AM +0200, Maarten Lankhorst
wrote:
> Op 22-08-13 02:10, Ilia Mirkin schreef:
> > The code expects non-VRAM mem nodes to have a pages list. If
that's not
> > set, it will do a null deref down the line. Warn on that condition and
> > return an error.
> >
> > See https://bugs.freedesktop.org/show_bug.cgi?id=64774
> >
> > Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
> > Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
> > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > Cc: <stable at vger.kernel.org> # 3.8+
> > ---
> >
> > I don't exactly understand what's going on, but this is just a
> > straightforward way to avoid a null deref that you see happens in the
> > bug. I haven't figured out the root cause of this, but it's
getting
> > well into the "I have no idea how TTM works" space. However
this seems
> > like a bit of defensive programming -- nouveau_vm_map_sg will pass
> > node->pages as a list down, which will be dereferenced by
> > nvc0_vm_map_sg. Perhaps the other arguments should make that
> > dereferencing not happen, but it definitely was happening here, as you
> > can see in the bug.
> >
> > Ben/Maarten, I'll let you judge whether this check is appropriate,
> > since like I hope I was able to convey above, I'm just not really
sure :)
> Not it really isn't appropriate..
> 
> You'd have to call call nouveau_vm_map_sg_table instead, the only place
that doesn't handle that correctly
> is where it's not expected to be called.
> 
> Here, have a completely untested patch to fix things...
> 
Maarten: I've been testing this stuff with Linux 3.10.x, so I had to modify
the patch a bit to make it apply there..
I've attached the modified copy that applies to 3.10.9, hopefully I did the
backport correctly.

With Linux 3.10.9 and the patch applied the kernel doesn't crash anymore,
and I get this error in dmesg:

[   76.105643] nouveau W[     DRM] Trying to create a fb in vram with
valid_domains=00000004

Does that help? 


-- Pasi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: nouveau_test_fix_null_deref_v3.10.patch
Type: text/x-diff
Size: 1202 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20130823/b13e064d/attachment.patch>

On Thu, Aug 22, 2013 at 5:12 PM, Maarten Lankhorst
<maarten.lankhorst at canonical.com> wrote:
> Op 22-08-13 02:10, Ilia Mirkin schreef:
>> The code expects non-VRAM mem nodes to have a pages list. If that's
not
>> set, it will do a null deref down the line. Warn on that condition and
>> return an error.
>>
>> See https://bugs.freedesktop.org/show_bug.cgi?id=64774
>>
>> Reported-by: Pasi K?rkk?inen <pasik at iki.fi>
>> Tested-by: Pasi K?rkk?inen <pasik at iki.fi>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> Cc: <stable at vger.kernel.org> # 3.8+
>> ---
>>
>> I don't exactly understand what's going on, but this is just a
>> straightforward way to avoid a null deref that you see happens in the
>> bug. I haven't figured out the root cause of this, but it's
getting
>> well into the "I have no idea how TTM works" space. However
this seems
>> like a bit of defensive programming -- nouveau_vm_map_sg will pass
>> node->pages as a list down, which will be dereferenced by
>> nvc0_vm_map_sg. Perhaps the other arguments should make that
>> dereferencing not happen, but it definitely was happening here, as you
>> can see in the bug.
>>
>> Ben/Maarten, I'll let you judge whether this check is appropriate,
>> since like I hope I was able to convey above, I'm just not really
sure :)
> Not it really isn't appropriate..
>
> You'd have to call call nouveau_vm_map_sg_table instead, the only place
that doesn't handle that correctly
> is where it's not expected to be called.
>
> Here, have a completely untested patch to fix things...
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -138,17 +143,26 @@ nouveau_user_framebuffer_create(struct drm_device
*dev,
>  {
>         struct nouveau_framebuffer *nouveau_fb;
>         struct drm_gem_object *gem;
> +       struct nouveau_bo *nvbo;
>         int ret = -ENOMEM;
>
>         gem = drm_gem_object_lookup(dev, file_priv,
mode_cmd->handles[0]);
>         if (!gem)
>                 return ERR_PTR(-ENOENT);
>
> +       nvbo = nouveau_gem_object(gem);
> +       if (!(nvbo->valid_domains & NOUVEAU_GEM_DOMAIN_VRAM)) {
> +               nv_warn(nouveau_drm(dev), "Trying to create a fb in
vram with"
> +                       " valid_domains=%08x\n",
nvbo->valid_domains);
> +               ret = -EINVAL;
> +               goto err_unref;
> +       }
> +
Definitely the right idea, we can't handle this case right now.
However, we may someday want/need to be able to scan out of system
memory, so this is the wrong place.

I suspect the correct thing to do (which'll also handle the
"defensive" part) is to bail in nouveau_bo_move() on attempts to move
a DMA-BUF backed object into VRAM.

Sound OK?

Ben.
>         nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer),
GFP_KERNEL);
>         if (!nouveau_fb)
>                 goto err_unref;
>
> -       ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd,
nouveau_gem_object(gem));
> +       ret = nouveau_framebuffer_init(dev, nouveau_fb, mode_cmd, nvbo);
>         if (ret)
>                 goto err;
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel