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

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 Thu, Sep 26, 2013 at 12:41 AM, Pasi K?rkk?inen <pasik at iki.fi>
wrote:
> 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..
I shall be looking at it properly once I'm back from XDC next week.

Thanks,
Ben.
>
> Thanks,
>
> -- Pasi
>

On Thu, Sep 26, 2013 at 02:48:49AM +1000, Ben Skeggs
wrote:
> On Thu, Sep 26, 2013 at 12:41 AM, Pasi K?rkk?inen <pasik at iki.fi>
wrote:
> > 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..
> I shall be looking at it properly once I'm back from XDC next week.
> 
Great, thanks!


-- Pasi
> Thanks,
> Ben.
>

On Thu, Sep 26, 2013 at 02:48:49AM +1000, Ben Skeggs
wrote:
> >
> > 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..
>
> I shall be looking at it properly once I'm back from XDC next week.
> 
Any thoughts about the patch? 


-- Pasi
> Thanks,
> Ben.
> 
> >
> > Thanks,
> >
> > -- Pasi
> >