Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm: NULL pointer dereference in drm_bridge_connector_atomic_check() on disabled connector - [Low] drm: mixed declarations and code in drm_bridge_connector_atomic_check() --
commit bf56f41939cb53804040ce55667f5edb2cca09b7 Author: Jan Claußen <[email protected]> drm/bridge: connector: Support the colorspace property for HDMI This commit adds support for the colorspace property to HDMI bridge connectors, allowing userspace to signal colorimetry like BT.2020. It attaches the property during initialization and forces a modeset when the colorspace changes. > diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c > b/drivers/gpu/drm/display/drm_bridge_connector.c > index 81f3e26f6fdf5..0639fe094b108 100644 > --- a/drivers/gpu/drm/display/drm_bridge_connector.c > +++ b/drivers/gpu/drm/display/drm_bridge_connector.c > @@ -412,6 +412,20 @@ drm_bridge_connector_mode_valid(struct drm_connector > *connector, > static int drm_bridge_connector_atomic_check(struct drm_connector *connector, > struct drm_atomic_commit *state) > { > + struct drm_connector_state *old_state = > + drm_atomic_get_old_connector_state(state, connector); > + struct drm_connector_state *new_state = > + drm_atomic_get_new_connector_state(state, connector); > + struct drm_crtc_state *crtc_state; > + > + if (old_state->colorspace != new_state->colorspace) { > + crtc_state = drm_atomic_get_crtc_state(state, new_state->crtc); [Severity: High] Can this cause a NULL pointer dereference if the connector is disabled? Userspace can submit an atomic commit that modifies the colorspace property on a disconnected or disabled connector. It might also concurrently disable the connector while modifying the property. In these scenarios, new_state->crtc will be NULL. If new_state->crtc is NULL, drm_atomic_get_crtc_state() immediately passes the pointer to drm_crtc_index(), which dereferences it: drm_atomic_get_crtc_state(state, crtc) { int ret, index = drm_crtc_index(crtc); ... Should the CRTC retrieval be guarded by an if (new_state->crtc) check to prevent crashes during such commits? > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + > + crtc_state->mode_changed = true; > + } > + > struct drm_bridge_connector *bridge_connector = > to_drm_bridge_connector(connector); [Severity: Low] This isn't a bug, but does this placement mix declarations and executable code? The newly inserted if block pushes the declaration of bridge_connector down, placing it after executable code. While supported in newer C standards, this violates the Linux kernel coding style and will trigger warnings from checkpatch.pl. Could this declaration be moved to the top of the block? > > if (bridge_connector->bridge_hdmi) -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
