| Message ID | 20190311210120.fdixvenmqjoxuoqt@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic-helper: Validate pointer before dereference | expand |
Hi Rodrigo On Mon, Mar 11, 2019 at 06:01:20PM -0300, Rodrigo Siqueira wrote: > The function disable_outputs() and > drm_atomic_helper_commit_modeset_enables() tries to retrieve > helper_private from the target CRTC, for dereferencing some operations. > However, the current implementation does not check whether > helper_private is null and, if not, if it has a valid pointer to a dpms > and commit functions. This commit adds pointer validations before > trying to dereference the dpms and commit function. > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > @@ -1277,11 +1279,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > if (new_crtc_state->enable) { > DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", > crtc->base.id, crtc->name); This DEBUG_ print is only relevant if the code actually do something in the following. So it seems more correct to fix the upper if () to: > if (new_crtc_state->enable && funcs != NULL) { > DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", ... The you also loose one indent and the calls are nicer. (If used "funcs != NULL", but this is a matter of taste). Sam
Hi, > - if (funcs->atomic_enable) > - funcs->atomic_enable(crtc, old_crtc_state); > - else > - funcs->commit(crtc); > + if (funcs) { > + if (funcs->atomic_enable) > + funcs->atomic_enable(crtc, > + old_crtc_state); > + else if (funcs->atomic_enable) "if (funcs->commit)" I guess? > + funcs->commit(crtc); > + } cheers, Gerd
On Mon, Mar 11, 2019 at 06:01:20PM -0300, Rodrigo Siqueira wrote: > The function disable_outputs() and > drm_atomic_helper_commit_modeset_enables() tries to retrieve > helper_private from the target CRTC, for dereferencing some operations. > However, the current implementation does not check whether > helper_private is null and, if not, if it has a valid pointer to a dpms > and commit functions. This commit adds pointer validations before > trying to dereference the dpms and commit function. > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Please also adjust the kerneldoc for these functions. And I think the patch subject can be improved, e.g. "Make ->atomic_enable/disable crtc callbacks optional". Describe what you're trying to achieve in the summary, not how you achieve it. > --- > drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 540a77a2ade9..fbeef7c461fc 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1028,14 +1028,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > > /* Right function depends upon target state. */ > - if (new_crtc_state->enable && funcs->prepare) > - funcs->prepare(crtc); > - else if (funcs->atomic_disable) > - funcs->atomic_disable(crtc, old_crtc_state); > - else if (funcs->disable) > - funcs->disable(crtc); > - else > - funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > + if (funcs) { I don't think making funcs optional is a good idea. If you have a crtc with no functions implemented, it's not terribly useful. Also making functions optional just here is not going to help if we still require it everywhere else. -Daniel > + if (new_crtc_state->enable && funcs->prepare) > + funcs->prepare(crtc); > + else if (funcs->atomic_disable) > + funcs->atomic_disable(crtc, old_crtc_state); > + else if (funcs->disable) > + funcs->disable(crtc); > + else if (funcs->dpms) > + funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > + } > > if (!(dev->irq_enabled && dev->num_crtcs)) > continue; > @@ -1277,11 +1279,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > if (new_crtc_state->enable) { > DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", > crtc->base.id, crtc->name); > - > - if (funcs->atomic_enable) > - funcs->atomic_enable(crtc, old_crtc_state); > - else > - funcs->commit(crtc); > + if (funcs) { > + if (funcs->atomic_enable) > + funcs->atomic_enable(crtc, > + old_crtc_state); > + else if (funcs->atomic_enable) > + funcs->commit(crtc); > + } > } > } > > -- > 2.21.0
Hi, First of all, thanks for the feedback. I will fix all the problems pointed out in the review. I just have two inline questions. On 03/12, Daniel Vetter wrote: > On Mon, Mar 11, 2019 at 06:01:20PM -0300, Rodrigo Siqueira wrote: > > The function disable_outputs() and > > drm_atomic_helper_commit_modeset_enables() tries to retrieve > > helper_private from the target CRTC, for dereferencing some operations. > > However, the current implementation does not check whether > > helper_private is null and, if not, if it has a valid pointer to a dpms > > and commit functions. This commit adds pointer validations before > > trying to dereference the dpms and commit function. > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > Please also adjust the kerneldoc for these functions. And I think the > patch subject can be improved, e.g. "Make ->atomic_enable/disable crtc > callbacks optional". Describe what you're trying to achieve in the > summary, not how you achieve it. Do I need to add information which says that both functions are optional? I'm asking because the description related to the affected functions and struct looks good for me [1,2,3]. 1. Documentation for drm_crtc_helper_funcs: https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html?highlight=drm_crtc_helper_funcs#c.drm_crtc_helper_funcs 2. Documentation for drm_atomic_helper_commit_modeset_enables(): https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html?highlight=drm_atomic_helper_commit_modeset_enables#c.drm_atomic_helper_commit_modeset_enables 3. Documentation for drm_atomic_helper_commit_modeset_disables(): https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html?highlight=drm_atomic_helper_commit_modeset_disables#c.drm_atomic_helper_commit_modeset_disables > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++------------- > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 540a77a2ade9..fbeef7c461fc 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1028,14 +1028,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > > > > > /* Right function depends upon target state. */ > > - if (new_crtc_state->enable && funcs->prepare) > > - funcs->prepare(crtc); > > - else if (funcs->atomic_disable) > > - funcs->atomic_disable(crtc, old_crtc_state); > > - else if (funcs->disable) > > - funcs->disable(crtc); > > - else > > - funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > + if (funcs) { > > I don't think making funcs optional is a good idea. If you have a crtc > with no functions implemented, it's not terribly useful. > > Also making functions optional just here is not going to help if we still > require it everywhere else. Should I remove the other occurrence of "if (funcs)" inside disable_outputs() and drm_atomic_helper_commit_modeset_enables()? Both functions, already had this before. Best Regards > -Daniel > > > + if (new_crtc_state->enable && funcs->prepare) > > + funcs->prepare(crtc); > > + else if (funcs->atomic_disable) > > + funcs->atomic_disable(crtc, old_crtc_state); > > + else if (funcs->disable) > > + funcs->disable(crtc); > > + else if (funcs->dpms) > > + funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > + } > > > > if (!(dev->irq_enabled && dev->num_crtcs)) > > continue; > > @@ -1277,11 +1279,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > > if (new_crtc_state->enable) { > > DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", > > crtc->base.id, crtc->name); > > - > > - if (funcs->atomic_enable) > > - funcs->atomic_enable(crtc, old_crtc_state); > > - else > > - funcs->commit(crtc); > > + if (funcs) { > > + if (funcs->atomic_enable) > > + funcs->atomic_enable(crtc, > > + old_crtc_state); > > + else if (funcs->atomic_enable) > > + funcs->commit(crtc); > > + } > > } > > } > > > > -- > > 2.21.0 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..fbeef7c461fc 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1028,14 +1028,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) /* Right function depends upon target state. */ - if (new_crtc_state->enable && funcs->prepare) - funcs->prepare(crtc); - else if (funcs->atomic_disable) - funcs->atomic_disable(crtc, old_crtc_state); - else if (funcs->disable) - funcs->disable(crtc); - else - funcs->dpms(crtc, DRM_MODE_DPMS_OFF); + if (funcs) { + if (new_crtc_state->enable && funcs->prepare) + funcs->prepare(crtc); + else if (funcs->atomic_disable) + funcs->atomic_disable(crtc, old_crtc_state); + else if (funcs->disable) + funcs->disable(crtc); + else if (funcs->dpms) + funcs->dpms(crtc, DRM_MODE_DPMS_OFF); + } if (!(dev->irq_enabled && dev->num_crtcs)) continue; @@ -1277,11 +1279,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, if (new_crtc_state->enable) { DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", crtc->base.id, crtc->name); - - if (funcs->atomic_enable) - funcs->atomic_enable(crtc, old_crtc_state); - else - funcs->commit(crtc); + if (funcs) { + if (funcs->atomic_enable) + funcs->atomic_enable(crtc, + old_crtc_state); + else if (funcs->atomic_enable) + funcs->commit(crtc); + } } }
The function disable_outputs() and drm_atomic_helper_commit_modeset_enables() tries to retrieve helper_private from the target CRTC, for dereferencing some operations. However, the current implementation does not check whether helper_private is null and, if not, if it has a valid pointer to a dpms and commit functions. This commit adds pointer validations before trying to dereference the dpms and commit function. Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> --- drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)