Re: [PATCH v6 07/28] drm/atomic_sro: Create kernel parameter to force or disable readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm: heap out-of-bounds write in drm_atomic_sro_set_private_obj_state
- [High] drm: OOB read/write in drm_atomic_sro_state_free due to dynamic num_connector
- [High] drm: heap out-of-bounds write in drm_atomic_sro_set_connector_state()
--

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: the one read out from the hardware. Create struct
drm_atomic_sro_state with arrays for planes, CRTCs, connectors, and private
objects, along with accessors to get and set individual object states within
it.

> 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

[ ... ]

> +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;

[ ... ]

> +	kfree(state->private_objs);
> +
> +	for (i = 0; i < state->dev->mode_config.num_connector; i++) {
> +		struct drm_connector *conn = state->connectors[i].ptr;
> +		struct drm_connector_state *conn_state =
> +			state->connectors[i].state;
> +
> +		if (!conn || !conn_state)
> +			continue;
> +
> +		conn->funcs->atomic_destroy_state(conn, conn_state);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code read past the end of the state->connectors[] array?

Because the loop uses the potentially updated
state->dev->mode_config.num_connector as its upper bound, what happens if a
concurrent hotplug (e.g. DP MST) increases num_connector between the
array's allocation and this free operation?

It seems this could dereference uninitialized memory.

> +		state->connectors[i].state = NULL;
> +		state->connectors[i].ptr = NULL;
> +		drm_connector_put(conn);
> +	}
> +
> +	kfree(state->connectors);

[ ... ]

> +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;
> +	state->connectors[index].state = conn_state;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can this code write out of bounds for state->connectors[]?

Since the array is sized by dev->mode_config.num_connector, but accessed
here using the sparse IDA-allocated drm_connector_index(conn), what happens
if a connector is dynamically removed (e.g. unplugging an MST display)?

If num_connector is decremented but the remaining connectors retain their
higher IDA indices, won't index exceed the allocated array capacity?

> +
> +	drm_dbg_atomic(conn->dev,
> +		       "Added [CONNECTOR:%d:%s] %p state to readout state %p\n",
> +		       conn->base.id, conn->name, conn_state, state);
> +}

[ ... ]

> +void drm_atomic_sro_set_private_obj_state(struct drm_atomic_sro_state *state,
> +					  struct drm_private_obj *obj,
> +					  struct drm_private_state *obj_state)
> +{
> +	unsigned int index = state->num_private_objs;
> +
> +	state->private_objs[index].ptr = obj;
> +	state->private_objs[index].state = obj_state;
> +	state->num_private_objs += 1;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code overflow state->private_objs[]?

Since state->private_objs[] is allocated in drm_atomic_sro_state_init()
strictly based on the lockless count_private_obj(dev) snapshot, what
happens if the number of private objects dynamically increases before
readout?

There doesn't seem to be a capacity check to ensure index doesn't exceed
the allocated size if a driver calls this for dynamically created private
objects.

> +
> +	drm_dbg_atomic(state->dev,
> +		       "Added new private object %p state %p to readout state %p\n", obj,
> +		       obj_state, state);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=7