Re: [PATCH] drm/nouveau: avoid null deref on bad arguments to nouveau_vma_getmap

["Pasi Kärkkäinen" <pasik@iki.fi>]

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

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@iki.fi>
> > Tested-by: Pasi Kärkkäinen <pasik@iki.fi>
> > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
> > Cc: <stable@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


[-- Attachment #2: nouveau_test_fix_null_deref_v3.10.patch --]
[-- Type: text/x-diff, Size: 1202 bytes --]

--- linux-3.10.9-100.fc18.x86_64/drivers/gpu/drm/nouveau/nouveau_display.c.orig	2013-07-01 01:13:29.000000000 +0300
+++ linux-3.10.9-100.fc18.x86_64/drivers/gpu/drm/nouveau/nouveau_display.c	2013-08-23 19:56:52.038234281 +0300
@@ -137,18 +137,31 @@
 {
 	struct nouveau_framebuffer *nouveau_fb;
 	struct drm_gem_object *gem;
+	struct nouveau_bo *nvbo;
 	int ret;
 
 	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;
+		drm_gem_object_unreference(gem);
+		return ERR_PTR(ret);
+	}
+
 	nouveau_fb = kzalloc(sizeof(struct nouveau_framebuffer), GFP_KERNEL);
-	if (!nouveau_fb)
+	if (!nouveau_fb) {
+		drm_gem_object_unreference(gem);
 		return ERR_PTR(-ENOMEM);
+	}
 
-	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) {
+		kfree(nouveau_fb);
 		drm_gem_object_unreference(gem);
 		return ERR_PTR(ret);
 	}

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel