egl/drm: ensure the backing gbm is set before using it

Submitted by Emil Velikov on July 5, 2019, 10:21 a.m.

Details

Message ID 20190705102141.4947-1-emil.l.velikov@gmail.com
State New
Headers show
Series "egl/drm: ensure the backing gbm is set before using it" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Emil Velikov July 5, 2019, 10:21 a.m.
From: Emil Velikov <emil.velikov@collabora.com>

Currently, if we error out before gbm_dri is set (say due to a different
name of the backing GBM implementation, or otherwise) the tear down will
trigger a NULL ptr deref and crash out.

Move the gbm_dri initialization as early as possible. To be on the extra
safe side add a NULL check in the teardown.

Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Eric Engestrom <eric.engestrom@intel.com>
Cc: mesa-stable@lists.freedesktop.org
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Supersedes https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1182
---
 src/egl/drivers/dri2/platform_drm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
index a811834114d..3e2124ad39c 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -704,6 +704,7 @@  dri2_initialize_drm(_EGLDisplay *disp)
          goto cleanup;
       }
    }
+   dri2_dpy->gbm_dri = gbm_dri_device(gbm);
 
    if (strcmp(gbm_device_get_backend_name(gbm), "drm") != 0) {
       err = "DRI2: gbm device using incorrect/incompatible backend";
@@ -718,7 +719,6 @@  dri2_initialize_drm(_EGLDisplay *disp)
 
    disp->Device = dev;
 
-   dri2_dpy->gbm_dri = gbm_dri_device(gbm);
    dri2_dpy->driver_name = strdup(dri2_dpy->gbm_dri->driver_name);
 
    dri2_dpy->dri_screen = dri2_dpy->gbm_dri->screen;
@@ -777,6 +777,6 @@  cleanup:
 void
 dri2_teardown_drm(struct dri2_egl_display *dri2_dpy)
 {
-   if (dri2_dpy->own_device)
+   if (dri2_dpy->own_device && dri2_dpy->gbm_dri)
       gbm_device_destroy(&dri2_dpy->gbm_dri->base);
 }

Comments

On Friday, 2019-07-05 11:21:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently, if we error out before gbm_dri is set (say due to a different
> name of the backing GBM implementation, or otherwise) the tear down will
> trigger a NULL ptr deref and crash out.
> 
> Move the gbm_dri initialization as early as possible. To be on the extra
> safe side add a NULL check in the teardown.
> 
> Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Eric Engestrom <eric.engestrom@intel.com>
> Cc: mesa-stable@lists.freedesktop.org
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Supersedes https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1182
> ---
>  src/egl/drivers/dri2/platform_drm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
> index a811834114d..3e2124ad39c 100644
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -704,6 +704,7 @@ dri2_initialize_drm(_EGLDisplay *disp)
>           goto cleanup;
>        }
>     }
> +   dri2_dpy->gbm_dri = gbm_dri_device(gbm);
>  
>     if (strcmp(gbm_device_get_backend_name(gbm), "drm") != 0) {
>        err = "DRI2: gbm device using incorrect/incompatible backend";
> @@ -718,7 +719,6 @@ dri2_initialize_drm(_EGLDisplay *disp)
>  
>     disp->Device = dev;
>  
> -   dri2_dpy->gbm_dri = gbm_dri_device(gbm);
>     dri2_dpy->driver_name = strdup(dri2_dpy->gbm_dri->driver_name);
>  
>     dri2_dpy->dri_screen = dri2_dpy->gbm_dri->screen;
> @@ -777,6 +777,6 @@ cleanup:
>  void
>  dri2_teardown_drm(struct dri2_egl_display *dri2_dpy)
>  {
> -   if (dri2_dpy->own_device)
> +   if (dri2_dpy->own_device && dri2_dpy->gbm_dri)

This would only be reachable by eglTerminate()ing an uninitialised
display, which is explicitly invalid anyway.

I think you can drop this check.

The gbm_dri_device(gbm) move above is:
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

>        gbm_device_destroy(&dri2_dpy->gbm_dri->base);
>  }
> -- 
> 2.21.0
>
On Fri, 5 Jul 2019 at 22:58, Eric Engestrom <eric.engestrom@intel.com> wrote:
>
> On Friday, 2019-07-05 11:21:41 +0100, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > Currently, if we error out before gbm_dri is set (say due to a different
> > name of the backing GBM implementation, or otherwise) the tear down will
> > trigger a NULL ptr deref and crash out.
> >
> > Move the gbm_dri initialization as early as possible. To be on the extra
> > safe side add a NULL check in the teardown.
> >
> > Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> > Cc: Eric Engestrom <eric.engestrom@intel.com>
> > Cc: mesa-stable@lists.freedesktop.org
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > Supersedes https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1182
> > ---
> >  src/egl/drivers/dri2/platform_drm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
> > index a811834114d..3e2124ad39c 100644
> > --- a/src/egl/drivers/dri2/platform_drm.c
> > +++ b/src/egl/drivers/dri2/platform_drm.c
> > @@ -704,6 +704,7 @@ dri2_initialize_drm(_EGLDisplay *disp)
> >           goto cleanup;
> >        }
> >     }
> > +   dri2_dpy->gbm_dri = gbm_dri_device(gbm);
> >
> >     if (strcmp(gbm_device_get_backend_name(gbm), "drm") != 0) {
> >        err = "DRI2: gbm device using incorrect/incompatible backend";
> > @@ -718,7 +719,6 @@ dri2_initialize_drm(_EGLDisplay *disp)
> >
> >     disp->Device = dev;
> >
> > -   dri2_dpy->gbm_dri = gbm_dri_device(gbm);
> >     dri2_dpy->driver_name = strdup(dri2_dpy->gbm_dri->driver_name);
> >
> >     dri2_dpy->dri_screen = dri2_dpy->gbm_dri->screen;
> > @@ -777,6 +777,6 @@ cleanup:
> >  void
> >  dri2_teardown_drm(struct dri2_egl_display *dri2_dpy)
> >  {
> > -   if (dri2_dpy->own_device)
> > +   if (dri2_dpy->own_device && dri2_dpy->gbm_dri)
>
> This would only be reachable by eglTerminate()ing an uninitialised
> display, which is explicitly invalid anyway.
>
> I think you can drop this check.
>
> The gbm_dri_device(gbm) move above is:
> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
>
Dropped the check in dri2_teardown_drm() and pushed to master.

Thank you
Emil