Re: [PATCH v6 06/28] drm/atomic_sro: Create drm_atomic_sro_state container

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>

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/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=6