On Fri, May 25, 2018 at 05:20:08AM +0300, Haneen Mohammed wrote:
This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs pointers before derefrencing the variable to avoid NULL pointer dereference and make the helper functions as optional as possible.
Signed-off-by: Haneen Mohammed hamohammed.sa@gmail.com
I started reviewing this, and then realized it's a bit a can of worms. E.g. connector->funcs->detect shouldn't be there, it's a helper callback really (but placed in the wrong function table). So connector->funcs isn't optional, but connector->funcs->detect maybe should be optional.
So I'm not sure anymore whether doing this holesale is a good idea. Do we still need this for vkms? If yes, then a more focused patch to just make the things optional that vkms does not (yet) provide might be better. Including an explanation of what exactly blows up in vkms.
Thanks, Daniel
drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++-------------- drivers/gpu/drm/drm_probe_helper.c | 11 ++++---- 2 files changed, 28 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index c35654591c12..52092deb741d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, if (!new_conn_state->crtc) continue;
if (funcs->atomic_best_encoder)
if (funcs && funcs->atomic_best_encoder) new_encoder = funcs->atomic_best_encoder(connector, new_conn_state);
else if (funcs->best_encoder)
else if (funcs && funcs->best_encoder) new_encoder = funcs->best_encoder(connector);@@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
funcs = connector->helper_private;
- if (funcs->atomic_best_encoder)
- if (funcs && funcs->atomic_best_encoder) new_encoder = funcs->atomic_best_encoder(connector, new_connector_state);
- else if (funcs->best_encoder)
- else if (funcs && funcs->best_encoder) new_encoder = funcs->best_encoder(connector); else new_encoder = drm_atomic_helper_best_encoder(connector);
@@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state) continue;
funcs = crtc->helper_private;
if (!funcs->mode_fixup)
if (!funcs || !funcs->mode_fixup) continue;ret = funcs->mode_fixup(crtc, &new_crtc_state->mode,
@@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, new_crtc_state->connectors_changed = true; }
if (funcs->atomic_check)
if (funcs && funcs->atomic_check) ret = funcs->atomic_check(connector, new_connector_state);@@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (connectors_mask & BIT(i)) continue;
if (funcs->atomic_check)
if (funcs && funcs->atomic_check) ret = funcs->atomic_check(connector, new_connector_state);@@ -972,14 +972,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);elsefuncs->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);elsefuncs->dpms(crtc, DRM_MODE_DPMS_OFF);}if (!(dev->irq_enabled && dev->num_crtcs)) continue;
@@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
funcs = crtc->helper_private;
if (new_crtc_state->enable && funcs->mode_set_nofb) {
if (new_crtc_state->enable && funcs && funcs->mode_set_nofb) { DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n", crtc->base.id, crtc->name);@@ -1197,7 +1199,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
funcs = crtc->helper_private;
if (new_crtc_state->enable) {
if (funcs && new_crtc_state->enable) { DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", crtc->base.id, crtc->name);@@ -2117,7 +2119,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
funcs = plane->helper_private;
if (funcs->prepare_fb) {
if (funcs && funcs->prepare_fb) { ret = funcs->prepare_fb(plane, new_plane_state); if (ret) goto fail;@@ -2135,7 +2137,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
funcs = plane->helper_private;
if (funcs->cleanup_fb)
if (funcs && funcs->cleanup_fb) funcs->cleanup_fb(plane, new_plane_state);@@ -2412,7 +2414,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
funcs = plane->helper_private;
if (funcs->cleanup_fb)
if (funcs && funcs->cleanup_fb) funcs->cleanup_fb(plane, plane_state);} diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 527743394150..42507aa7f763 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -272,9 +272,9 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force) retry: ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx); if (!ret) {
if (funcs->detect_ctx)
if (funcs && funcs->detect_ctx) ret = funcs->detect_ctx(connector, &ctx, force);
else if (connector->funcs->detect)
else if (connector->funcs && connector->funcs->detect) ret = connector->funcs->detect(connector, force);@@ -320,9 +320,9 @@ drm_helper_probe_detect(struct drm_connector *connector, if (ret) return ret;
- if (funcs->detect_ctx)
- if (funcs && funcs->detect_ctx) return funcs->detect_ctx(connector, ctx, force);
- else if (connector->funcs->detect)
- else if (connector->funcs && connector->funcs->detect) return connector->funcs->detect(connector, force); else return connector_status_connected;
@@ -480,7 +480,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, goto prune; }
- count = (*connector_funcs->get_modes)(connector);
if (connector_funcs && connector_funcs->get_modes)
count = (*connector_funcs->get_modes)(connector);if (count == 0 && connector->status == connector_status_connected) count = drm_add_modes_noedid(connector, 1024, 768);
-- 2.17.0