| Message ID | 20230612075530.681869-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mxsfb: Disable overlay plane in mxsfb_plane_overlay_atomic_disable() | expand |
Hi Liu Ying. On Mon, Jun 12, 2023 at 03:55:30PM +0800, Liu Ying wrote: > When disabling overlay plane in mxsfb_plane_overlay_atomic_update(), > overlay plane's framebuffer pointer is NULL. So, dereferencing it would > cause a kernel Oops(NULL pointer dereferencing). Fix the issue by > disabling overlay plane in mxsfb_plane_overlay_atomic_disable() instead. Reading the above I had expected that some code was dropped from mxsfb_plane_overlay_atomic_update(). I do not know the driver code, but was confused so decided to give feedback. Sam > > Fixes: cb285a5348e7 ("drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()") > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > index 3bcc9c0f2019..7ed2516b6de0 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > @@ -611,6 +611,14 @@ static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane, > writel(ctrl, mxsfb->base + LCDC_AS_CTRL); > } > > +static void mxsfb_plane_overlay_atomic_disable(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev); > + > + writel(0, mxsfb->base + LCDC_AS_CTRL); > +} > + > static bool mxsfb_format_mod_supported(struct drm_plane *plane, > uint32_t format, > uint64_t modifier) > @@ -626,6 +634,7 @@ static const struct drm_plane_helper_funcs mxsfb_plane_primary_helper_funcs = { > static const struct drm_plane_helper_funcs mxsfb_plane_overlay_helper_funcs = { > .atomic_check = mxsfb_plane_atomic_check, > .atomic_update = mxsfb_plane_overlay_atomic_update, > + .atomic_disable = mxsfb_plane_overlay_atomic_disable, > }; > > static const struct drm_plane_funcs mxsfb_plane_funcs = { > -- > 2.37.1
On Mon, Jun 12, 2023 at 4:04 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Liu Ying. Hi Sam, > > On Mon, Jun 12, 2023 at 03:55:30PM +0800, Liu Ying wrote: > > When disabling overlay plane in mxsfb_plane_overlay_atomic_update(), > > overlay plane's framebuffer pointer is NULL. So, dereferencing it would > > cause a kernel Oops(NULL pointer dereferencing). Fix the issue by > > disabling overlay plane in mxsfb_plane_overlay_atomic_disable() instead. > > Reading the above I had expected that some code was dropped from > mxsfb_plane_overlay_atomic_update(). Yes, the offending commit cb285a5348e7 dropped mxsfb_get_fb_paddr() which contains an important !fb check to avoid the NULL pointer dereferencing. > I do not know the driver code, but was confused so decided to give > feedback. drm_fb_{cma, dma}_get_gem_addr() called by mxsfb_plane_primary_atomic_update() don't do !fb check but directly dereference fb. That's why the NULL pointer dereferencing issue happens. With this patch, mxsfb_plane_overlay_atomic_disable() is used to disable overlay plane, not mxsfb_plane_primary_atomic_update(). Please see funcs->atomic_{disable, update} in drm_atomic_helper_commit_planes(). Regards, Liu Ying > > Sam > > > > > Fixes: cb285a5348e7 ("drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()") > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > index 3bcc9c0f2019..7ed2516b6de0 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > @@ -611,6 +611,14 @@ static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane, > > writel(ctrl, mxsfb->base + LCDC_AS_CTRL); > > } > > > > +static void mxsfb_plane_overlay_atomic_disable(struct drm_plane *plane, > > + struct drm_atomic_state *state) > > +{ > > + struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev); > > + > > + writel(0, mxsfb->base + LCDC_AS_CTRL); > > +} > > + > > static bool mxsfb_format_mod_supported(struct drm_plane *plane, > > uint32_t format, > > uint64_t modifier) > > @@ -626,6 +634,7 @@ static const struct drm_plane_helper_funcs mxsfb_plane_primary_helper_funcs = { > > static const struct drm_plane_helper_funcs mxsfb_plane_overlay_helper_funcs = { > > .atomic_check = mxsfb_plane_atomic_check, > > .atomic_update = mxsfb_plane_overlay_atomic_update, > > + .atomic_disable = mxsfb_plane_overlay_atomic_disable, > > }; > > > > static const struct drm_plane_funcs mxsfb_plane_funcs = { > > -- > > 2.37.1
On 6/12/23 09:55, Liu Ying wrote: > When disabling overlay plane in mxsfb_plane_overlay_atomic_update(), > overlay plane's framebuffer pointer is NULL. So, dereferencing it would > cause a kernel Oops(NULL pointer dereferencing). Fix the issue by > disabling overlay plane in mxsfb_plane_overlay_atomic_disable() instead. > > Fixes: cb285a5348e7 ("drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()") > Signed-off-by: Liu Ying <victor.liu@nxp.com> Should this be Cc: stable too ?
On Mon, Jun 12, 2023 at 5:06 PM Marek Vasut <marex@denx.de> wrote: > > On 6/12/23 09:55, Liu Ying wrote: > > When disabling overlay plane in mxsfb_plane_overlay_atomic_update(), > > overlay plane's framebuffer pointer is NULL. So, dereferencing it would > > cause a kernel Oops(NULL pointer dereferencing). Fix the issue by > > disabling overlay plane in mxsfb_plane_overlay_atomic_disable() instead. > > > > Fixes: cb285a5348e7 ("drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()") > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > Should this be Cc: stable too ? Ok, will explicitly Cc: stable. Thanks.
On 6/12/23 11:15, Ying Liu wrote: > On Mon, Jun 12, 2023 at 5:06 PM Marek Vasut <marex@denx.de> wrote: >> >> On 6/12/23 09:55, Liu Ying wrote: >>> When disabling overlay plane in mxsfb_plane_overlay_atomic_update(), >>> overlay plane's framebuffer pointer is NULL. So, dereferencing it would >>> cause a kernel Oops(NULL pointer dereferencing). Fix the issue by >>> disabling overlay plane in mxsfb_plane_overlay_atomic_disable() instead. >>> >>> Fixes: cb285a5348e7 ("drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()") >>> Signed-off-by: Liu Ying <victor.liu@nxp.com> >> >> Should this be Cc: stable too ? > > Ok, will explicitly Cc: stable. Thanks. Add Reviewed-by: Marek Vasut <marex@denx.de> And also, wait a little bit for RB from others. Thanks !
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 3bcc9c0f2019..7ed2516b6de0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -611,6 +611,14 @@ static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane, writel(ctrl, mxsfb->base + LCDC_AS_CTRL); } +static void mxsfb_plane_overlay_atomic_disable(struct drm_plane *plane, + struct drm_atomic_state *state) +{ + struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev); + + writel(0, mxsfb->base + LCDC_AS_CTRL); +} + static bool mxsfb_format_mod_supported(struct drm_plane *plane, uint32_t format, uint64_t modifier) @@ -626,6 +634,7 @@ static const struct drm_plane_helper_funcs mxsfb_plane_primary_helper_funcs = { static const struct drm_plane_helper_funcs mxsfb_plane_overlay_helper_funcs = { .atomic_check = mxsfb_plane_atomic_check, .atomic_update = mxsfb_plane_overlay_atomic_update, + .atomic_disable = mxsfb_plane_overlay_atomic_disable, }; static const struct drm_plane_funcs mxsfb_plane_funcs = {
When disabling overlay plane in mxsfb_plane_overlay_atomic_update(), overlay plane's framebuffer pointer is NULL. So, dereferencing it would cause a kernel Oops(NULL pointer dereferencing). Fix the issue by disabling overlay plane in mxsfb_plane_overlay_atomic_disable() instead. Fixes: cb285a5348e7 ("drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()") Signed-off-by: Liu Ying <victor.liu@nxp.com> --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 9 +++++++++ 1 file changed, 9 insertions(+)