Nouveau - Feb 2020 - [PATCH] drm/nouveau: Fix NULL ptr access in nv50_wndw_prepare_fb()

This fixes a kernel oops when loading the nouveau
module with fb console enabled after the change:

  drm/nouveau: Remove field nvbo from struct nouveau_framebuffer

state->fb may be NULL in nv50_wndw_prepare_fb(),
so defer initializing nvbo from its obj[] array
until after the NULL check.

Signed-off-by: James Jones <jajones at nvidia.com>
---
 drivers/gpu/drm/nouveau/dispnv50/wndw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
index 4a67a656e007..68c0dc2dc2d3 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
@@ -490,7 +490,7 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct
drm_plane_state *state)
 	struct nouveau_drm *drm = nouveau_drm(plane->dev);
 	struct nv50_wndw *wndw = nv50_wndw(plane);
 	struct nv50_wndw_atom *asyw = nv50_wndw_atom(state);
-	struct nouveau_bo *nvbo = nouveau_gem_object(fb->obj[0]);
+	struct nouveau_bo *nvbo;
 	struct nv50_head_atom *asyh;
 	struct nv50_wndw_ctxdma *ctxdma;
 	int ret;
@@ -499,6 +499,7 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct
drm_plane_state *state)
 	if (!asyw->state.fb)
 		return 0;
 
+	nvbo = nouveau_gem_object(fb->obj[0]);
 	ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, true);
 	if (ret)
 		return ret;
-- 
2.17.1

Hi

I'm surprised that prepare_fb is called with fb == NULL. But, OK

Acked-by: Thomas Zimmermann <tzimmermann at suse.de>

Thanks for the fix.

Am 11.02.20 um 00:09 schrieb James Jones:
> This fixes a kernel oops when loading the nouveau
> module with fb console enabled after the change:
> 
>   drm/nouveau: Remove field nvbo from struct nouveau_framebuffer
> 
> state->fb may be NULL in nv50_wndw_prepare_fb(),
> so defer initializing nvbo from its obj[] array
> until after the NULL check.
> 
> Signed-off-by: James Jones <jajones at nvidia.com>
> ---
>  drivers/gpu/drm/nouveau/dispnv50/wndw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> index 4a67a656e007..68c0dc2dc2d3 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> @@ -490,7 +490,7 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct
drm_plane_state *state)
>  	struct nouveau_drm *drm = nouveau_drm(plane->dev);
>  	struct nv50_wndw *wndw = nv50_wndw(plane);
>  	struct nv50_wndw_atom *asyw = nv50_wndw_atom(state);
> -	struct nouveau_bo *nvbo = nouveau_gem_object(fb->obj[0]);
> +	struct nouveau_bo *nvbo;
>  	struct nv50_head_atom *asyh;
>  	struct nv50_wndw_ctxdma *ctxdma;
>  	int ret;
> @@ -499,6 +499,7 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct
drm_plane_state *state)
>  	if (!asyw->state.fb)
>  		return 0;
>  
> +	nvbo = nouveau_gem_object(fb->obj[0]);
>  	ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, true);
>  	if (ret)
>  		return ret;
> 
-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 N?rnberg, Germany
(HRB 36809, AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20200211/6ad9e925/attachment.sig>

On Tue, Feb 11, 2020 at 06:28:47AM +0100, Thomas Zimmermann
wrote:
> Hi
> 
> I'm surprised that prepare_fb is called with fb == NULL. But, OK
Yeah we don't filter that ... maybe an oversight? I'm not sure whether
there's any driver that needs to do something special for when the plane
is disabled here (since "plane off" iff "plane_state-fb ==
NULL").

In general I think we have an awful lot of bugs in most drivers for the
case when the plane state is in the atomic commit, but not enabled.
-Daniel
> 
> Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
> 
> Thanks for the fix.
> 
> Am 11.02.20 um 00:09 schrieb James Jones:
> > This fixes a kernel oops when loading the nouveau
> > module with fb console enabled after the change:
> > 
> >   drm/nouveau: Remove field nvbo from struct nouveau_framebuffer
> > 
> > state->fb may be NULL in nv50_wndw_prepare_fb(),
> > so defer initializing nvbo from its obj[] array
> > until after the NULL check.
> > 
> > Signed-off-by: James Jones <jajones at nvidia.com>
> > ---
> >  drivers/gpu/drm/nouveau/dispnv50/wndw.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> > index 4a67a656e007..68c0dc2dc2d3 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> > @@ -490,7 +490,7 @@ nv50_wndw_prepare_fb(struct drm_plane *plane,
struct drm_plane_state *state)
> >  	struct nouveau_drm *drm = nouveau_drm(plane->dev);
> >  	struct nv50_wndw *wndw = nv50_wndw(plane);
> >  	struct nv50_wndw_atom *asyw = nv50_wndw_atom(state);
> > -	struct nouveau_bo *nvbo = nouveau_gem_object(fb->obj[0]);
> > +	struct nouveau_bo *nvbo;
> >  	struct nv50_head_atom *asyh;
> >  	struct nv50_wndw_ctxdma *ctxdma;
> >  	int ret;
> > @@ -499,6 +499,7 @@ nv50_wndw_prepare_fb(struct drm_plane *plane,
struct drm_plane_state *state)
> >  	if (!asyw->state.fb)
> >  		return 0;
> >  
> > +	nvbo = nouveau_gem_object(fb->obj[0]);
> >  	ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, true);
> >  	if (ret)
> >  		return ret;
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N?rnberg, Germany
> (HRB 36809, AG N?rnberg)
> Gesch?ftsf?hrer: Felix Imend?rffer
> 


> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch