Re: [PATCH v2 18/37] drm/bridge: sil-sii8620: Add atomic bridge state handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The `sii8620_mode_fixup` callback mutates persistent driver state (`ctx->use_packed_pixel`), violating the atomic check phase contract.
- [High] `drm_bridge_attach` ignores the return value of `drm_atomic_private_obj_init`, and `drm_atomic_private_obj_init` fails to roll back on error, leading to a corrupted private object list and a subsequent NULL pointer dereference.
--

--- Patch [18]: [PATCH v2 18/37] drm/bridge: sil-sii8620: Add atomic bridge state handlers ---
commit b25b0389a6c4219ff91245a5da61562f89c23dda
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: sil-sii8620: Add atomic bridge state handlers

The sil-sii8620 bridge doesn't implement any enable or disable
callbacks, but is still missing the atomic state handlers.
In order to remove the legacy bridge callback support from the DRM
bridge core, add them.

> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 982306eb4f0a7..22c81b38b41e8 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c

[ ... ]

> @@ -2280,6 +2281,9 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>  }
>  
>  static const struct drm_bridge_funcs sii8620_bridge_funcs = {
> +	.atomic_create_state = drm_atomic_helper_bridge_create_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.attach = sii8620_attach,
>  	.detach = sii8620_detach,
>  	.mode_fixup = sii8620_mode_fixup,

[Severity: High]
This isn't a bug introduced by this patch, but doesn't sii8620_mode_fixup()
mutate persistent driver state by assigning ctx->use_packed_pixel?

If userspace submits a DRM_MODE_ATOMIC_TEST_ONLY commit with a mode requiring
a different pixel packing, drm_atomic_helper_check_modeset() will call
drm_atomic_bridge_chain_check(), invoking this callback:

drivers/gpu/drm/bridge/sil-sii8620.c:sii8620_mode_fixup() {
	...
	ctx->use_packed_pixel = sii8620_is_packing_required(ctx, adjusted_mode);
	...
}

Will this cause the driver's global state to be modified during a test-only
commit, leading to a subsequent modeset or asynchronous interrupt programming
the hardware using the test-only pixel packing value?

[Severity: High]
This is also a pre-existing issue, but by adding atomic bridge state handlers,
does this bridge now expose an error handling bug in the DRM core?

Looking at drm_bridge_attach() in drivers/gpu/drm/drm_bridge.c, it ignores
the return value of drm_atomic_private_obj_init():

drivers/gpu/drm/drm_bridge.c:drm_bridge_attach() {
	...
	if (drm_bridge_is_atomic(bridge))
		drm_atomic_private_obj_init(bridge->dev, &bridge->base,
					    &drm_bridge_priv_state_funcs);

	return 0;
}

If memory allocation fails in atomic_create_state,
drm_atomic_private_obj_init() returns an error but fails to clean up:

drivers/gpu/drm/drm_atomic.c:drm_atomic_private_obj_init() {
	...
	list_add_tail(&obj->head, &dev->mode_config.privobj_list);

	state = obj->funcs->atomic_create_state(obj);
	if (IS_ERR(state))
		return PTR_ERR(state);
	...
}

Will this leave obj->head in dev->mode_config.privobj_list with a NULL
obj->state?

If so, could this cause a kernel oops later if drm_state_dump() (e.g. via
debugfs) iterates privobj_list and calls
drm_atomic_private_obj_print_state() which dereferences the NULL state
via state->obj?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-drm-all-atomic-bridges-v2-0-21d03cbca446@kernel.org?part=18