Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/atomic_sro: heap out-of-bounds array access due to sparse connector IDA - [Medium] drm: connector reference leak in drm_atomic_sro_state_free() when conn_state is NULL - [Low] drm: incorrect error code -EINVAL on OOM in drm_atomic_sro_state_alloc() --
commit 7fa33b0c7c68e508f3bb44a6225923e6e800d132 Author: Maxime Ripard <[email protected]> drm/atomic_sro: Create drm_atomic_sro_state container The SRO infrastructure needs a state container to collect the readout states for all KMS objects. Unlike drm_atomic_commit which tracks old and new states for atomic commits, this container only holds a single state per object. This commit creates struct drm_atomic_sro_state with arrays for planes, CRTCs, connectors, and private objects. > diff --git a/drivers/gpu/drm/drm_atomic_sro.c > b/drivers/gpu/drm/drm_atomic_sro.c > new file mode 100644 > index 0000000000000..096fdb7088190 > --- /dev/null > +++ b/drivers/gpu/drm/drm_atomic_sro.c [ ... ] > +static int drm_atomic_sro_state_init(struct drm_device *dev, > + struct drm_atomic_sro_state *state) > +{ > + int err; > + > + state->planes = > + kzalloc_objs(*state->planes, dev->mode_config.num_total_plane); > + if (!state->planes) > + return -ENOMEM; > + > + state->crtcs = kzalloc_objs(*state->crtcs, dev->mode_config.num_crtc); > + if (!state->crtcs) { > + err = -ENOMEM; > + goto err_free_planes; > + } > + > + state->connectors = kzalloc_objs(*state->connectors, > + dev->mode_config.num_connector); [Severity: High] Does allocating the array using num_connector account for sparse IDA indices? Connector indices are allocated dynamically and can exceed the active connector count if connectors are hotplugged or removed. Using num_connector as the size limits the array, which may cause out-of-bounds access when indexed directly. > + if (!state->connectors) { > + err = -ENOMEM; > + goto err_free_crtcs; > + } > + > + state->private_objs = > + kzalloc_objs(*state->private_objs, count_private_obj(dev)); > + if (!state->private_objs) { > + err = -ENOMEM; > + goto err_free_connectors; > + } > + > + state->num_private_objs = 0; > + > + drm_dev_get(dev); > + state->dev = dev; > + > + return 0; > + > +err_free_connectors: > + kfree(state->connectors); > +err_free_crtcs: > + kfree(state->crtcs); > +err_free_planes: > + kfree(state->planes); > + return err; > +} > + > +/** > + * drm_atomic_sro_state_alloc - allocate an SRO state container > + * @dev: DRM device to allocate the state for > + * > + * Allocates and initializes a &struct drm_atomic_sro_state that can hold > + * readout states for all KMS objects on @dev. > + * > + * The returned state must be freed with drm_atomic_sro_state_free(). > + * > + * RETURNS: > + * > + * A new &struct drm_atomic_sro_state on success, an error pointer on > + * failure. > + */ > +struct drm_atomic_sro_state *drm_atomic_sro_state_alloc(struct drm_device > *dev) > +{ > + struct drm_atomic_sro_state *state; > + int ret; > + > + state = kzalloc_obj(*state); > + if (!state) > + return ERR_PTR(-EINVAL); [Severity: Low] Should this return -ENOMEM instead of -EINVAL when memory allocation fails? > + > + ret = drm_atomic_sro_state_init(dev, state); > + if (ret) { > + kfree(state); > + return ERR_PTR(ret); > + } > + > + return state; > +} > +EXPORT_SYMBOL(drm_atomic_sro_state_alloc); > + > +/** > + * drm_atomic_sro_state_free - free an SRO state container > + * @state: SRO state to free > + * > + * Frees a &struct drm_atomic_sro_state previously allocated with > + * drm_atomic_sro_state_alloc(). > + */ > +void drm_atomic_sro_state_free(struct drm_atomic_sro_state *state) > +{ > + struct drm_device *dev = state->dev; > + unsigned int i; > + > + for (i = 0; i < state->num_private_objs; i++) { > + struct drm_private_obj *obj = state->private_objs[i].ptr; > + struct drm_private_state *obj_state = > state->private_objs[i].state; > + > + if (!obj || !obj_state) > + continue; > + > + obj->funcs->atomic_destroy_state(obj, obj_state); > + state->private_objs[i].state = NULL; > + state->private_objs[i].ptr = NULL; > + } > + > + kfree(state->private_objs); > + > + for (i = 0; i < state->dev->mode_config.num_connector; i++) { [Severity: High] Does iterating up to the live num_connector cause out-of-bounds reads? Since num_connector can increase after the SRO state was allocated if connectors are added, this loop might read past the end of the originally allocated state->connectors array. > + struct drm_connector *conn = state->connectors[i].ptr; > + struct drm_connector_state *conn_state = > + state->connectors[i].state; > + > + if (!conn || !conn_state) > + continue; [Severity: Medium] Does this skip drm_connector_put() and leak the reference if conn is valid but conn_state is NULL? In drm_atomic_sro_set_connector_state(), drm_connector_get() unconditionally takes a reference on conn and stores it. If conn_state is NULL during cleanup, the early continue bypasses drm_connector_put() completely. > + > + conn->funcs->atomic_destroy_state(conn, conn_state); > + state->connectors[i].state = NULL; > + state->connectors[i].ptr = NULL; > + drm_connector_put(conn); > + } > + [ ... ] > +/** > + * drm_atomic_sro_set_connector_state - store a connector state into an SRO > state > + * @state: SRO state container > + * @conn: connector the state belongs to > + * @conn_state: connector state to store > + * > + * Takes a reference on @conn which is released when the state is > + * installed or freed. > + */ > +void drm_atomic_sro_set_connector_state(struct drm_atomic_sro_state *state, > + struct drm_connector *conn, > + struct drm_connector_state *conn_state) > +{ > + unsigned int index = drm_connector_index(conn); > + > + drm_connector_get(conn); > + state->connectors[index].ptr = conn; [Severity: High] Can this write out of bounds? Since the connectors array was allocated using num_connector rather than accommodating sparse indices, a connector with an index greater than or equal to num_connector will cause this write to overwrite heap memory. > + state->connectors[index].state = conn_state; > + > + drm_dbg_atomic(conn->dev, > + "Added [CONNECTOR:%d:%s] %p state to readout state %p\n", > + conn->base.id, conn->name, conn_state, state); > +} > +EXPORT_SYMBOL(drm_atomic_sro_set_connector_state); > + [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
