Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] drm: NULL pointer dereference in drm_atomic_helper_sro_readout_state() due to missing fallback - [High] drm: NULL pointer dereference for legacy bridges in drm_atomic_helper_sro_readout_bridges_state() - [High] drm/atomic_sro: NULL pointer dereference in disabled bridge readout - [High] drm/atomic_sro: out-of-bounds write in connector state array - [Medium] drm: O(N^2) loop and OOB read risk in drm_atomic_sro_install_state() - [Low] drm/atomic: error code shadowed in drm_atomic_helper_sro_build_state() --
commit 4ec8820a89de05a3189db7a50590573132825dee Author: Maxime Ripard <[email protected]> drm/atomic_sro: Provide helpers to implement hardware state readout Drivers implementing SRO need to iterate over their KMS objects, allocate a pristine state for each, call the readout hook, and store the result in the SRO state container. Provide helpers for each object type and a top-level function that calls them in sequence. > diff --git a/drivers/gpu/drm/drm_atomic_sro_helper.c > b/drivers/gpu/drm/drm_atomic_sro_helper.c > new file mode 100644 > index 0000000000000..50d1af53bfc24 > --- /dev/null > +++ b/drivers/gpu/drm/drm_atomic_sro_helper.c [ ... ] > +int > +drm_atomic_helper_sro_readout_bridges_state(struct drm_device *dev, > + struct drm_atomic_sro_state *state) > +{ > + struct drm_encoder *encoder; > + > + might_alloc(GFP_KERNEL); [ ... ] > + drm_for_each_encoder(encoder, dev) { > + struct drm_bridge *bridge; > + int ret; > + > + list_for_each_entry(bridge, &encoder->bridge_chain, chain_node) > { > + struct drm_private_obj *bridge_obj = &bridge->base; > + const struct drm_private_state_funcs *bridge_obj_funcs = > + bridge_obj->funcs; > + struct drm_private_state *bridge_obj_state; > + > + if (!bridge_obj_funcs->atomic_sro_readout_state) { [Severity: High] Can bridge_obj_funcs be NULL here for legacy non-atomic bridges? Legacy (non-atomic) bridges do not call drm_atomic_private_obj_init() during attachment. This means their embedded bridge->base is uninitialized, and its funcs pointer remains NULL. If this readout helper encounters a non-atomic bridge in the encoder's bridge chain, dereferencing bridge_obj_funcs unconditionally will cause a NULL pointer dereference. > + drm_warn(dev, > + "No bridge %s readout implementation.", > + bridge->name); > + continue; > + } [ ... ] > +struct drm_atomic_sro_state * > +drm_atomic_helper_sro_build_state(struct drm_device *dev) > +{ > + struct drm_atomic_sro_state *state; > + struct drm_printer p = drm_dbg_printer(dev, DRM_UT_ATOMIC, NULL); > + int ret; > + > + drm_dbg_kms(dev, "Starting to build atomic state from hardware > state.\n"); > + > + state = drm_atomic_sro_state_alloc(dev); > + if (drm_WARN_ON(dev, IS_ERR(state))) > + return ERR_PTR(-ENOMEM); [Severity: Low] Does this error handling shadow the actual error pointer returned by drm_atomic_sro_state_alloc()? In drm_atomic_sro_state_alloc(), an allocation failure incorrectly returns ERR_PTR(-EINVAL). Then, this function masks that error pointer and replaces it with ERR_PTR(-ENOMEM) instead of propagating it. While this results in a misleading error code on failure, it also violates standard kernel error propagation patterns where the true error pointer is typically returned directly. > + > + ret = drm_atomic_helper_sro_readout_crtcs_state(dev, state); > + if (ret) > + goto err_state_free; [ ... ] > +int drm_atomic_helper_sro_readout_state(struct drm_device *dev) > +{ > + const struct drm_mode_config_helper_funcs *funcs = > + dev->mode_config.helper_private; > + struct drm_atomic_sro_state *state; > + > + if (!drm_atomic_sro_device_can_readout(dev)) { > + drm_info(dev, "Device doesn't support hardware state readout."); > + return -ENOTSUPP; > + } > + > + state = funcs->atomic_sro_build_state(dev); [Severity: High] Does this code handle cases where funcs is NULL, or where funcs->atomic_sro_build_state is NULL? The documentation for atomic_sro_build_state states that drm_atomic_helper_sro_build_state() is the default implementation, which implies drivers are not required to provide a custom hook. If a driver does not provide this hook, or does not use helper private data, calling funcs->atomic_sro_build_state(dev) unconditionally will result in a NULL pointer dereference. > + if (IS_ERR(state)) > + return PTR_ERR(state); > + > + drm_atomic_sro_install_state(state); > + drm_atomic_sro_state_free(state); > + > + return 0; > +} [Severity: High] Is there a risk of a NULL pointer dereference during disabled bridge readout when using drm_atomic_sro_get_crtc_state()? If the bridge readout is called when the pipeline is inactive, a connector state may not have an associated CRTC. The drm_bridge_atomic_readout_priv_state() hook appears to pass this NULL CRTC directly to drm_atomic_sro_get_crtc_state(): drm_bridge.c:drm_bridge_atomic_readout_priv_state() { ... crtc_state = drm_atomic_sro_get_crtc_state(state, conn_state->crtc); ... } Which unconditionally dereferences the CRTC pointer to fetch its index: drm_atomic_sro.c:drm_atomic_sro_get_crtc_state() { ... unsigned int index = drm_crtc_index(crtc); ... } This can cause a kernel oops on boot if SRO is enabled and a disabled pipeline is read out. Note that this issue was identified as being introduced in a subsequent patch in the series and persists through the final commit. [Severity: Medium] Does drm_atomic_sro_install_state() have an O(N^2) loop and an out-of-bounds read risk? The loop condition in drm_atomic_sro_install_state() evaluates a dynamic list count rather than the allocated array bounds: drm_atomic_sro.c:drm_atomic_sro_install_state() { ... for (i = 0; i < count_private_obj(state->dev); i++) { const struct drm_private_state_funcs *obj_funcs; struct drm_private_state *obj_state; struct drm_private_obj *obj; obj = state->private_objs[i].ptr; ... } Because count_private_obj() iterates over a linked list on every iteration, this creates an O(N^2) time complexity. Furthermore, if a private object is registered between the allocation of the state->private_objs array and the invocation of this loop, count_private_obj() will return a value larger than the allocated array size. This would lead to an out-of-bounds read of state->private_objs[i].ptr. Using state->num_private_objs instead of count_private_obj() would avoid both the complexity and the bounds risk. [Severity: High] Does drm_atomic_sro_set_connector_state() risk an out-of-bounds write when connector indices exceed the active connector count? drm_atomic_sro_state_init() allocates the state->connectors array sized strictly to the current num_connector count: drm_atomic_sro.c:drm_atomic_sro_state_init() { ... state->connectors = kzalloc_objs(*state->connectors, dev->mode_config.num_connector); if (!state->connectors) { ... } However, drm_connector_index(conn) returns an IDA-allocated index. If connectors are dynamically added and removed (for example, via DP MST hot-unplug), the IDA index can be larger than the total number of currently registered connectors. drm_atomic_sro_set_connector_state() blindly uses this potentially out-of-bounds index to write into the array: drm_atomic_sro.c:drm_atomic_sro_set_connector_state() { ... unsigned int index = drm_connector_index(conn); drm_connector_get(conn); state->connectors[index].ptr = conn; state->connectors[index].state = conn_state; ... } This can corrupt kernel memory if state readout is performed after connector removals. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
