Re: [PATCH v2 08/37] drm/bridge: lontium-lt8713sx: Add atomic bridge state handlers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double-free of the firmware object in the error paths of firmware update.
- [High] Missing minimum firmware size check causes massive buffer overflow/underflow.
- [High] drm_atomic_private_obj_init() leaves the object in privobj_list on error, and drm_bridge_attach() ignores the error, leading to crashes.
--

--- Patch [8]: [PATCH v2 08/37] drm/bridge: lontium-lt8713sx: Add atomic bridge state handlers ---
commit cc389f313911703ff4573192b0ab28ddca13b386
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: lontium-lt8713sx: Add atomic bridge state handlers

The lontium-lt8713sx 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/lontium-lt8713sx.c b/drivers/gpu/drm/bridge/lontium-lt8713sx.c
> index cee485adf5e56..4a6420b876da6 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt8713sx.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt8713sx.c

[ ... ]

> @@ -508,6 +509,9 @@ static const struct attribute_group *lt8713sx_attr_groups[] = {
>  };
>  
>  static const struct drm_bridge_funcs lt8713sx_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 = lt8713sx_bridge_attach,
>  };

[Severity: High]
This isn't a bug introduced by this patch, but by adding .atomic_create_state
you are enabling the atomic path in drm_bridge_attach() which will now call
drm_atomic_private_obj_init().

If drm_atomic_private_obj_init() fails, it returns an error but leaves the
object in dev->mode_config.privobj_list with a NULL obj->state:

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

	obj->state = state;

Then drm_bridge_attach() ignores this return value:

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 a state dump is triggered later, drm_state_dump() iterates over
privobj_list and calls drm_atomic_private_obj_print_state(p, obj->state).
Since obj->state is NULL, this will cause a NULL pointer dereference.

Should the return value of drm_atomic_private_obj_init() be checked in
drm_bridge_attach(), and should drm_atomic_private_obj_init() remove the
object from the list on failure?

[Severity: High]
This is a pre-existing issue, but I noticed a potential double-free in the
firmware update error paths of this driver.

In lt8713sx_prepare_firmware_data():

	release_firmware(lt8713sx->fw);
	return -EINVAL;

This frees the firmware object but leaves lt8713sx->fw pointing to the freed
memory. Then the caller lt8713sx_firmware_update() handles the error:

error:
	if (lt8713sx->fw) {
		release_firmware(lt8713sx->fw);
		lt8713sx->fw = NULL;
	}

Could this result in a double-free of the firmware object?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential buffer overflow
in lt8713sx_prepare_firmware_data():

	memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1);
	...
	memcpy(lt8713sx->fw_buffer + SZ_64K,
	       lt8713sx->fw->data + SZ_64K,
	       lt8713sx->fw->size - SZ_64K);

If a user provides a firmware file smaller than 64KB, would the first memcpy
read out of bounds, and would lt8713sx->fw->size - SZ_64K underflow resulting
in a massive out-of-bounds copy?

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