Re: [PATCH v9 2/2] drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] State machine lockup and resource leak on firmware upgrade error paths.
- [High] Missing exclusivity check in `lt7911exc_dsi_host_attach` causes memory leaks and list corruption.
- [High] Data race and hardware state corruption due to lockless access in DRM atomic hooks.
- [High] Use-After-Free (UAF) due to sysfs attribute lifetime extending past driver removal.
- [Medium] Concurrency flaw in `lt7911exc_firmware_store` blocks sysfs writers and causes redundant upgrades.
--

commit aa1e91d28c6a1d1b9faea6c1c05acd8d8780f392
Author: Sunyun Yang <syyang@lontium.com>

drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

Add support for the Lontium LT7911EXC bridge chip, which converts eDP input
to MIPI DSI output using an internal firmware-controlled pipeline.

> diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> new file mode 100644
> index 0000000000000..e94188e7987ab
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c

[ ... ]

> +static void lt7911exc_firmware_upgrade_work(struct work_struct *work)
> +{
> +	struct lt7911exc *lt7911exc = container_of(work, struct lt7911exc, work);
> +	struct device *dev = lt7911exc->dev;
> +	const struct firmware *fw;
> +	u8 *buffer;
> +	size_t total_size = FW_SIZE - 4;
> +	u32 crc32;
> +	int ret;
> +
> +	ret = request_firmware(&fw, FW_FILE, dev);
> +	if (ret) {
> +		dev_err(dev, "failed to load '%s'\n", FW_FILE);
> +		goto out_status_only;
> +	}
> +
> +	if (fw->size > total_size) {
> +		dev_err(dev, "firmware too large (%zu > %zu)\n", fw->size, total_size);
> +		goto out_release_fw;
> +	}
> +
> +	buffer = kmalloc(total_size, GFP_KERNEL);
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto out_release_fw;
> +	}

Will jumping to out_release_fw leave the upgrade flag permanently set?

If the firmware size is too large or kmalloc() fails, the code jumps to
out_release_fw, which calls release_firmware() and returns without clearing
lt7911exc->upgrade.

Since the flag remains set to true, all subsequent attempts to write to the
sysfs firmware attribute will fail with -EBUSY, and
lt7911exc_atomic_pre_enable() will silently return early without enabling the
MIPI stream.

[ ... ]

> +out_release_fw:
> +	release_firmware(fw);
> +	return;
> +
> +out_status_only:
> +	mutex_lock(&lt7911exc->ocm_lock);
> +	lt7911exc->upgrade = false;
> +	mutex_unlock(&lt7911exc->ocm_lock);
> +}
> +
> +static void lt7911exc_atomic_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *state)
> +{
> +	struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);
> +
> +	if (lt7911exc->upgrade)
> +		return;
> +
> +	//enable mipi stream
> +	regmap_write(lt7911exc->regmap, 0xe0b0, 0x01);
> +}

Can this unsynchronized check result in hardware state corruption?

This function checks lt7911exc->upgrade and proceeds to perform I2C
writes without holding ocm_lock. If a display modeset happens
concurrently with a firmware upgrade, the unsynchronized I2C write can
interleave with the firmware flashing sequence (for example, while the MCU
is halted or flash is being erased). 

The same applies to lt7911exc_atomic_post_disable(). Both hooks likely need
to hold ocm_lock while checking the flag and performing the register access.

[ ... ]

> +static int lt7911exc_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *dev)
> +{
> +	struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel, &bridge);
> +	if (ret)
> +		return ret;
> +
> +	if (panel) {
> +		bridge = drm_panel_bridge_add_typed(panel, DRM_MODE_CONNECTOR_DSI);
> +		if (IS_ERR(bridge))
> +			return PTR_ERR(bridge);
> +	}
> +	lt7911exc->output.dev = dev;
> +	lt7911exc->output.bridge = bridge;
> +	lt7911exc->output.panel = panel;
> +
> +	drm_bridge_add(&lt7911exc->bridge);
> +	return 0;
> +}

Is an exclusivity check needed here to prevent memory leaks and list
corruption?

The function unconditionally allocates a new panel bridge via
drm_panel_bridge_add_typed() and calls drm_bridge_add(). If multiple
downstream DSI devices are attached, the previous panel bridge allocation
would be leaked.

Repeatedly calling drm_bridge_add() on the same bridge also re-initializes
its hpd_mutex, which destroys any existing lock state and imbalances the
bridge reference counts.

[ ... ]

> +static ssize_t lt7911exc_firmware_store(struct device *dev, struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct lt7911exc *lt7911exc = dev_get_drvdata(dev);
> +
> +	mutex_lock(&lt7911exc->ocm_lock);
> +	if (lt7911exc->upgrade) {
> +		mutex_unlock(&lt7911exc->ocm_lock);
> +		return -EBUSY;
> +	}
> +
> +	lt7911exc->upgrade = true;
> +	mutex_unlock(&lt7911exc->ocm_lock);
> +
> +	schedule_work(&lt7911exc->work);
> +
> +	return len;
> +}

Could this concurrency approach block sysfs writers and cause redundant
upgrades?

The upgrade worker lt7911exc_firmware_upgrade_work() holds ocm_lock for
the entire duration of the firmware flash process. If userspace writes to
this attribute while an upgrade is running, the store function blocks on
mutex_lock() instead of immediately returning -EBUSY.

Once the running upgrade completes, the worker clears the upgrade flag and
releases the lock. The blocked sysfs writer will then acquire the lock, see
the flag is false, set it back to true, and queue a second, redundant
firmware upgrade.

[ ... ]

> +static void lt7911exc_remove(struct i2c_client *client)
> +{
> +	struct lt7911exc *lt7911exc = i2c_get_clientdata(client);
> +
> +	mipi_dsi_host_unregister(&lt7911exc->dsi_host);
> +	cancel_work_sync(&lt7911exc->work);
> +	gpiod_set_value_cansleep(lt7911exc->reset_gpio, 1);
> +}

Does this leave a use-after-free window between driver removal and sysfs
group cleanup?

The lt7911exc_firmware sysfs attribute is registered via the driver's
dev_groups. The driver core removes these groups after this remove callback
returns.

Userspace can still write to the sysfs attribute after cancel_work_sync()
is executed here. If this occurs, lt7911exc_firmware_store() will
successfully call schedule_work() again. When the device's devres resources
are later freed, the workqueue will access the freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519105019.22622-1-syyang@lontium.com?part=2