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

[Ben Skeggs]

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