Re: [PATCH v10 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 9 potential issue(s) to consider:
- [High] Hardware backpowering risk due to incorrect reset GPIO power sequencing.
- [High] Use-After-Free vulnerability due to dynamic DRM bridge removal in DSI detach callback.
- [High] Use-After-Free of `regmap` during DRM teardown because firmware upgrade worker defeats removal safeguard.
- [High] Firmware upgrade performs a hardware reset, causing silent loss of DRM state and a blank screen.
- [High] Extended mutex hold during firmware upgrade stalls DRM atomic commits for multiple seconds.
- [Medium] Spurious panel initialization failures due to `mutex_trylock` in DSI transfer.
- [Medium] Use of `kmalloc` for a 64KB software buffer is susceptible to memory fragmentation failures.
- [Medium] MIPI DSI host fakes successful transfers, breaking the panel initialization contract.
- [Medium] Introduction of a custom sysfs attribute for firmware flashing creates non-standard ABI.
--

commit 26f664545bed69c339feae8f2d3005edbcb54627
Author: Sunyun Yang <syyang@lontium.com>
Subject: drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

Add support for the Lontium LT7911EXC eDP to MIPI DSI bridge chip. The
driver includes integration for eDP-to-DSI routing, a MIPI DSI host
interface, firmware upgrade via I2C, and GPIO/regulator management. Display
timing and DSI packet generation are managed by internal firmware.

> diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> new file mode 100644
> --- /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_clear_status;
> +	}
> +
> +	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);

Does this risk memory fragmentation failures? Allocating a 64KB buffer with
kmalloc requires a contiguous order-4 physical allocation, which might fail
with -ENOMEM on a fragmented system. Since this buffer is only used in
software for CRC computation and padding, and is not mapped for DMA, could
this use kvmalloc instead?

> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto out_release_fw;
> +	}
> +
> +	memset(buffer, 0xff, total_size);
> +	memcpy(buffer, fw->data, fw->size);
> +	crc32 = cal_crc32_custom(buffer, total_size);
> +	kfree(buffer);
> +
> +	mutex_lock(&lt7911exc->ocm_lock);

Could this prolonged mutex hold stall DRM atomic commits? The firmware
upgrade sequence holds ocm_lock for multiple seconds while performing flash
erase, a 64KB I2C write, and verification.

Meanwhile, lt7911exc_atomic_pre_enable() and lt7911exc_atomic_post_disable()
use a blocking mutex_lock on the same lock. Any DRM display commit or
teardown occurring during a firmware upgrade might stall for seconds
waiting for this lock.

> +
> +	lt7911exc_reset(lt7911exc);

Will this hardware reset cause a silent loss of DRM state?

The physical chip reset wipes out volatile registers, including the MIPI
stream enable bit configured by lt7911exc_atomic_pre_enable(). After the
upgrade completes, the driver unlocks the mutex without restoring the prior
hardware state or queuing a DRM modeset.

If the display was active during the upgrade, could the screen stay blank
indefinitely while the kernel DRM state still considers the bridge fully
active?

[ ... ]

> +	ret = lt7911exc_upgrade_result(lt7911exc, crc32);
> +	if (ret)
> +		dev_err(dev, "firmware verification failed\n");
> +
> +out_unlock:
> +	lt7911exc_hw_mcu_run(lt7911exc);
> +	lt7911exc->fw_version = lt7911exc_read_version(lt7911exc);
> +	mutex_unlock(&lt7911exc->ocm_lock);
> +
> +out_release_fw:
> +	release_firmware(fw);
> +
> +out_clear_status:
> +	mutex_lock(&lt7911exc->ocm_lock);
> +	lt7911exc->upgrade = false;

Is there a risk of a use-after-free here during module removal?

In lt7911exc_remove(), the driver sets lt7911exc->upgrade = true to block
hardware access from late DRM callbacks, then calls cancel_work_sync(). If
the firmware upgrade worker is running, remove() blocks waiting for it.

However, when the worker finishes, it unconditionally sets upgrade = false.
This leaves the upgrade flag as false after remove() completes. When devres
frees the device memory and regmap, could a delayed DRM atomic disable see
that upgrade is false and attempt to call regmap_write, resulting in a
use-after-free?

> +	mutex_unlock(&lt7911exc->ocm_lock);
> +}

[ ... ]

> +static int lt7911exc_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *dev)
> +{
> +	struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);
> +
> +	if (!lt7911exc->output.dev)
> +		return 0;
> +
> +	drm_bridge_remove(&lt7911exc->bridge);
> +	if (lt7911exc->output.panel)
> +		drm_panel_bridge_remove(lt7911exc->output.bridge);

Does removing the bridge here introduce a use-after-free vulnerability?

When the downstream DSI panel unbinds, drm_panel_bridge_remove() frees the
panel bridge memory via devres. However, if the upstream DRM encoder has
already attached to this bridge, the bridge remains linked in the encoder's
bridge_chain.

Subsequent DRM operations like atomic commits or teardowns will traverse
the chain and dereference the freed panel bridge structure. Should
drm_bridge_add() and panel bridge creation happen in probe() instead?

> +
> +	lt7911exc->output.dev = NULL;
> +	lt7911exc->output.bridge = NULL;
> +	lt7911exc->output.panel = NULL;
> +
> +	return 0;
> +}
> +
> +/*
> + * The internal firmware controls the panel initialization
> + * sequence and handles all MIPI DSI command transmission.
> + */
> +static ssize_t lt7911exc_dsi_host_transfer(struct mipi_dsi_host *host,
> +					   const struct mipi_dsi_msg *msg)
> +{
> +	struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);
> +
> +	if (msg->rx_len) {
> +		dev_warn(lt7911exc->dev, "MIPI DSI read is not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	switch (msg->type) {
> +	case MIPI_DSI_DCS_SHORT_WRITE:
> +	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> +	case MIPI_DSI_DCS_LONG_WRITE:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> +	case MIPI_DSI_GENERIC_LONG_WRITE:
> +	break;
> +	default:
> +	return -EOPNOTSUPP;
> +	}
> +
> +	if (!mutex_trylock(&lt7911exc->ocm_lock))
> +		return -EBUSY;

Can this trylock cause spurious panel initialization failures?

The ocm_lock is also acquired for short durations by fast-path operations
like lt7911exc_firmware_show() and lt7911exc_atomic_pre_enable(). If a
panel driver sends a DCS initialization command exactly when a user reads
the sysfs firmware version, mutex_trylock will fail.

Since panel drivers typically do not retry DCS commands, could this race
abort panel initialization and result in a dead display?

> +
> +	if (lt7911exc->upgrade) {
> +		mutex_unlock(&lt7911exc->ocm_lock);
> +		return -EBUSY;
> +	}
> +	mutex_unlock(&lt7911exc->ocm_lock);
> +
> +	return msg->tx_len;

Does faking successful transfers break the panel initialization contract?

This returns msg->tx_len for write commands without actually transmitting
any data to the hardware. Downstream panel drivers might send DCS
initialization commands expecting them to reach the panel, so faking
success means the kernel state and panel state diverge.

While the firmware handles panel init autonomously, could silently dropping
commands mask compatibility issues with panels that require specific
software sequences?

> +}

[ ... ]

> +static ssize_t lt7911exc_firmware_store(struct device *dev, struct device_attribute *attr,
> +					const char *buf, size_t len)

Does the introduction of this custom sysfs attribute create a non-standard
ABI? The driver adds a custom lt7911exc_firmware sysfs file to trigger
firmware updates, which bypasses standard kernel frameworks. Would it be
better to use standardized interfaces like devlink or the standard firmware
loader during probe or initialization instead of creating a custom
user-space ABI?

> +{
> +	struct lt7911exc *lt7911exc = dev_get_drvdata(dev);

[ ... ]

> +static int lt7911exc_probe(struct i2c_client *client)
> +{
> +	struct lt7911exc *lt7911exc;
> +	struct device *dev = &client->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;

[ ... ]

> +	/*
> +	 * reset GPIO is defined as active low in device tree.
> +	 * gpiod_set_value_cansleep() uses logical value:
> +	 * 1 -> asserted (active)  -> physical low  -> reset enabled -> chip stop
> +	 * 0 -> deasserted (inactive) -> physical high -> reset released -> chip run
> +	 */
> +	lt7911exc->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(lt7911exc->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(lt7911exc->reset_gpio),
> +				     "failed to acquire reset gpio\n");
> +
> +	ret = lt7911exc_regulator_enable(lt7911exc);

Is there a hardware backpowering risk due to this reset GPIO power
sequencing?

Here devm_gpiod_get() initializes the reset GPIO to a logical 0. Because
the GPIO is defined as active-low in the device tree, this logical 0
translates to a physical HIGH level. This occurs before the vcc and vdd
regulators are enabled.

Driving a HIGH logic level into an unpowered IC forces current through its
internal ESD protection diodes into the unpowered rails, which risks
latch-up and permanent hardware damage. Should the regulators be enabled
before requesting the GPIO, or should the GPIO be requested with
GPIOD_OUT_HIGH (physical LOW) instead?

> +	if (ret)
> +		return ret;

[ ... ]

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