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

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 Fri, Aug 23, 2013 at 11:20:42PM +0300, Pasi K?rkk?inen
wrote:
> 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? 
>
Any comments? 

Maarten's patch works for me, I get that warning instead of a kernel crash,
but it's also a bigger change that doesn't apply to older kernels as-is.

Ilia's original patch in this thread can be applied to older kernels as-is,
and it also prevents the kernel crash from happening.

Should we get both applied, so Ilia's patch can be CC'd to stable ? 
 
-- Pasi

> ---
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);
>  	}
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Op 28-08-13 08:29, Pasi K?rkk?inen schreef:
> On Fri, Aug 23, 2013 at 11:20:42PM +0300, Pasi K?rkk?inen wrote:
>> 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? 
>>
> Any comments? 
>
> Maarten's patch works for me, I get that warning instead of a kernel
crash,
> but it's also a bigger change that doesn't apply to older kernels
as-is.
>
> Ilia's original patch in this thread can be applied to older kernels
as-is,
> and it also prevents the kernel crash from happening.
>
> Should we get both applied, so Ilia's patch can be CC'd to stable ?
>
You haven't understood the root cause then. You're trying to move an
IMPORTED dma-buf into VRAM.
Doing so would seem to work, but at that point it's no longer a dma-buf so
changes done by the exporter
would not show up in nouveau because they no longer refer to the same piece of
memory.

Failing is the only right option here.

~Maarten