Re: [PATCH v14 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 3 potential issue(s) to consider:
- [High] Using devm_drm_panel_bridge_add_typed() in the DSI host attach callback leaks memory and leaves dangling pointers when the downstream panel unbinds.
- [High] Potential use-after-free of the drm_device pointer in the firmware upgrade worker when the DRM display driver is unloaded.
- [High] Using stack buffers for I2C regmap bulk reads violates DMA mapping rules and can cause memory corruption.
--

commit a175e14df601fd080297e05f4671c7c7284dc3f0
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..f7cd89bcc8119
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
[ ... ]
> +static int lt7911exc_read_version(struct lt7911exc *lt7911exc)
> +{
> +	struct device *dev = lt7911exc->dev;
> +	u8 buf[3];
> +	int ret;
> +
> +	ret = regmap_write(lt7911exc->regmap, LT7911EXC_PAGE_CONTROL, 0xe0);
> +	if (ret) {
> +		dev_err(dev, "Failed to switch to page 0xe0\n");
> +		return ret;
> +	}
> +
> +	/* no need to halt MCU for this register access */
> +	ret = regmap_bulk_read(lt7911exc->regmap, 0x81, buf, ARRAY_SIZE(buf));

[Severity: High]
Can using a stack buffer for I2C regmap bulk reads cause memory corruption?

If the underlying I2C adapter uses DMA, the DMA API will attempt to map this
stack memory. The kernel forbids doing DMA on stack variables as it can share
cache lines with other data, resulting in memory corruption.

Should buf be dynamically allocated?

[ ... ]
> +static int lt7911exc_upgrade_result(struct lt7911exc *lt7911exc, u32 crc32)
> +{
> +	struct device *dev = lt7911exc->dev;
> +	u32 read_hw_crc = 0;
> +	u8 crc_tmp[4];
> +	int ret;
> +
> +	ret = regmap_write(lt7911exc->regmap, LT7911EXC_PAGE_CONTROL, 0xe0);
> +	if (ret) {
> +		dev_err(dev, "Failed to switch to page 0xe0 in prog_init: %d\n", ret);
> +		return ret;
> +	}
> +	ret = regmap_write(lt7911exc->regmap, 0xee, 0x01);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(lt7911exc->regmap, 0x7b, 0x60);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(lt7911exc->regmap, 0x7b, 0x40);
> +	if (ret)
> +		return ret;
> +
> +	msleep(150);
> +
> +	ret = regmap_bulk_read(lt7911exc->regmap, 0x22, crc_tmp, ARRAY_SIZE(crc_tmp));

[Severity: High]
Can this stack buffer also cause memory corruption if the I2C adapter uses DMA?
It looks like crc_tmp should be dynamically allocated as well.

[ ... ]
> +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;
> +	struct drm_device *drm_dev = NULL;
[ ... ]
> +	mutex_lock(&lt7911exc->ocm_lock);
> +	lt7911exc_hw_mcu_run(lt7911exc);
> +	lt7911exc->fw_version = lt7911exc_read_version(lt7911exc);
> +	if (lt7911exc->bridge.dev)
> +		drm_dev = lt7911exc->bridge.dev;
> +
> +	mutex_unlock(&lt7911exc->ocm_lock);
> +
> +	/* Notify DRM framework that hardware state changed/needs a modeset */
> +	if (drm_dev)
> +		drm_kms_helper_hotplug_event(drm_dev);

[Severity: High]
Can this sequence lead to a use-after-free of the drm_dev pointer?

The drm_dev pointer is extracted under ocm_lock, but the lock is released
before calling drm_kms_helper_hotplug_event(drm_dev) without taking a reference.
If the main DRM display driver is unbound concurrently right after the lock is
released, the drm_device could be destroyed before this worker finishes.

Should the worker acquire a reference with drm_dev_get() under the lock and
drop it after the hotplug event?

[ ... ]
> +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;
> +
> +	if (lt7911exc->output.dev)
> +		return -EBUSY;
> +
> +	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel, &bridge);
> +	if (ret)
> +		return ret;
> +
> +	if (panel) {
> +		bridge = devm_drm_panel_bridge_add_typed(lt7911exc->dev, panel,
> +							 DRM_MODE_CONNECTOR_DSI);

[Severity: High]
Does this devm allocation cause a memory leak when the downstream panel unbinds?

The allocated bridge is tied to the lifetime of the host device rather than the
panel. If the panel driver is unbound, lt7911exc_dsi_host_detach() clears the
local pointer, but the bridge remains registered and the memory is not freed.
If the panel binds again, a new bridge is allocated.

Should this use drm_panel_bridge_add_typed() instead?

> +		if (IS_ERR(bridge))
> +			return PTR_ERR(bridge);
> +	}
> +	lt7911exc->output.dev = dev;
> +	lt7911exc->output.bridge = bridge;
> +	lt7911exc->output.panel = panel;
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	lt7911exc->output.bridge = NULL;

[Severity: High]
Should this explicitly remove the bridge if it is allocated in attach?

Currently this clears the pointer but leaves the bridge registered in the DRM
core's global bridge list.

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