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

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

Op 04-09-13 05:41, Ben Skeggs schreef:
> 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?
>
If it has a WARN_ON or something that would be ok, I didn't find any other
places that attempt to move buffers to VRAM though, so it's probably
harmless.

When looking into this bug I noticed that nouveau_bo_vma_add needs to have a
check for nvbo->page_shift == vma->vm->vmm->spg_shift,
and only if the check is true it should map the page in TTM_PL_TT. Patch below.
Should probably also be cc'd to stable.

~Maarten

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 89b992e..355a1b7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1560,7 +1560,8 @@ nouveau_bo_vma_add(struct nouveau_bo *nvbo, struct
nouveau_vm *vm,
 
 	if (nvbo->bo.mem.mem_type == TTM_PL_VRAM)
 		nouveau_vm_map(vma, nvbo->bo.mem.mm_node);
-	else if (nvbo->bo.mem.mem_type == TTM_PL_TT) {
+	else if (nvbo->bo.mem.mem_type == TTM_PL_TT &&
+		 nvbo->page_shift == vma->vm->vmm->spg_shift) {
 		if (node->sg)
 			nouveau_vm_map_sg_table(vma, 0, size, node);
 		else

On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst
wrote:
> Op 04-09-13 05:41, Ben Skeggs schreef:
> > 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?
> >
> If it has a WARN_ON or something that would be ok, I didn't find any
other places that attempt to move buffers to VRAM though, so it's probably
harmless.
> 
So hmm.. I guess another patch is needed for the original issue in this thread.
Is someone going to submit that? 

> When looking into this bug I noticed that nouveau_bo_vma_add needs to have
a check for nvbo->page_shift == vma->vm->vmm->spg_shift,
> and only if the check is true it should map the page in TTM_PL_TT. Patch
below.
> Should probably also be cc'd to stable.
>
Thanks! Is this patch ready to be merged?


-- Pasi
 
> ~Maarten
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 89b992e..355a1b7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1560,7 +1560,8 @@ nouveau_bo_vma_add(struct nouveau_bo *nvbo, struct
nouveau_vm *vm,
>  
>  	if (nvbo->bo.mem.mem_type == TTM_PL_VRAM)
>  		nouveau_vm_map(vma, nvbo->bo.mem.mm_node);
> -	else if (nvbo->bo.mem.mem_type == TTM_PL_TT) {
> +	else if (nvbo->bo.mem.mem_type == TTM_PL_TT &&
> +		 nvbo->page_shift == vma->vm->vmm->spg_shift) {
>  		if (node->sg)
>  			nouveau_vm_map_sg_table(vma, 0, size, node);
>  		else
>

Hello,

On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst
wrote:
> Op 04-09-13 05:41, Ben Skeggs schreef:
> > 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?
> >
> If it has a WARN_ON or something that would be ok, I didn't find any
other places that attempt to move buffers to VRAM though, so it's probably
harmless.
>
Ben/Maarten: Are you guys planning to take a look at this and submit another
patch, or.. ?

I tested the two earlier patches from this thread, and they both fixed the
problem (hard kernel crash).
I'm hoping this bug could be finally solved in the kernel..

Thanks,

-- Pasi

On Wed, Sep 04, 2013 at 08:59:13AM +0200, Maarten Lankhorst
wrote:
> 
> When looking into this bug I noticed that nouveau_bo_vma_add needs to have
a check for nvbo->page_shift == vma->vm->vmm->spg_shift,
> and only if the check is true it should map the page in TTM_PL_TT. Patch
below.
> Should probably also be cc'd to stable.
>
How about this patch? Is it ready to go in? 

Thanks,

-- Pasi

 
> ~Maarten
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 89b992e..355a1b7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1560,7 +1560,8 @@ nouveau_bo_vma_add(struct nouveau_bo *nvbo, struct
nouveau_vm *vm,
>  
>  	if (nvbo->bo.mem.mem_type == TTM_PL_VRAM)
>  		nouveau_vm_map(vma, nvbo->bo.mem.mm_node);
> -	else if (nvbo->bo.mem.mem_type == TTM_PL_TT) {
> +	else if (nvbo->bo.mem.mem_type == TTM_PL_TT &&
> +		 nvbo->page_shift == vma->vm->vmm->spg_shift) {
>  		if (node->sg)
>  			nouveau_vm_map_sg_table(vma, 0, size, node);
>  		else
>