Re: [PATCH v2 04/15] drm/bridge: inno-hdmi: Split probe out of bind

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `inno_hdmi_remove` implementation leads to hardware interrupt storms and violates API contract.

Pre-existing issues:
- [Critical] HPD IRQ is requested and enabled before bridge attach, leading to NULL pointer dereference in the interrupt handler.
- [High] Resource leak of devm_-managed allocations during component bind retries.
- [High] TOCTOU race exposing the I2C adapter to userspace before internal initialization completes.
- [High] Use-after-free in the interrupt handler during device teardown due to incorrect devres registration order.
--

commit 355630a3f7bbfe2d55d6902048b35afc6cb4d58d
Author: Michal Wilczynski <m.wilczynski@samsung.com>

drm/bridge: inno-hdmi: Split probe out of bind
    
inno_hdmi_bind() both sets up the bridge and attaches it to a DRM
encoder. A platform whose HDMI controller is a child of a larger device
needs the first half without the second, since it registers as its own
platform driver and lets the DRM core bind the bridge later.
    
Move the setup into a new exported inno_hdmi_probe(), with a matching
inno_hdmi_remove(), and reduce inno_hdmi_bind() to a wrapper around it.
    
No functional change intended.

> Move the setup into a new exported inno_hdmi_probe(), with a matching
> inno_hdmi_remove(), and reduce inno_hdmi_bind() to a wrapper around it.

[Severity: High]
The commit message mentions adding a matching inno_hdmi_remove() function,
but it does not appear to be implemented in this patch.

Without an exported remove function to safely tear down the hardware and
unmask interrupts, could this lead to hardware interrupt storms if the
platform module is removed or the driver is unbound?

> diff --git a/drivers/gpu/drm/bridge/inno-hdmi.c b/drivers/gpu/drm/bridge/inno-hdmi.c
> index 9a62bf59a4033..12fd208d5e852 100644
> --- a/drivers/gpu/drm/bridge/inno-hdmi.c
> +++ b/drivers/gpu/drm/bridge/inno-hdmi.c
[ ... ]
> +struct inno_hdmi *inno_hdmi_probe(struct platform_device *pdev,
> +				  const struct inno_hdmi_plat_data *plat_data)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> +	struct device *dev = &pdev->dev;

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a
time-of-check to time-of-use (TOCTOU) race condition during I2C adapter
registration within this setup path.

In inno_hdmi_i2c_adapter(), devm_i2c_add_adapter() publishes the adapter
to userspace before assigning hdmi->i2c = i2c. If a userspace tool
initiates an I2C transaction concurrently during this small window,
inno_hdmi_i2c_xfer() will attempt to lock i2c->lock using the still-NULL
hdmi->i2c pointer.

Can this lead to a NULL pointer dereference?

[Severity: Critical]
This is a pre-existing issue, but requesting the IRQ before the bridge
is attached seems to cause a crash on decoupled platforms.

During the hardware setup in this path, devm_request_threaded_irq()
requests the hotplug interrupt. If a monitor is already connected, the
interrupt can fire immediately. The handler, inno_hdmi_irq(), unconditionally
calls drm_helper_hpd_irq_event(hdmi->bridge.dev). However, hdmi->bridge.dev
remains NULL until drm_bridge_attach() is called later.

Since this patch explicitly supports decoupled drivers that register
the bridge without immediately attaching it, does this guarantee a boot-time
NULL pointer dereference if a display is connected?

[Severity: High]
This is a pre-existing issue, but there appears to be a devres registration
order issue that leads to a use-after-free.

The IRQ is requested via devm_request_threaded_irq() before the I2C
adapter memory is allocated with devm_kzalloc(). During device teardown,
devres cleans up resources in reverse order, meaning the I2C memory is freed
while the IRQ remains active.

If a hardware interrupt fires during this teardown window,
inno_hdmi_hardirq() will check the dangling pointer and call
inno_hdmi_i2c_irq(), which accesses the already freed memory.

Should the IRQ be requested after all resources it depends on are allocated?

[ ... ]
> +struct inno_hdmi *inno_hdmi_bind(struct device *dev,
> +				 struct drm_encoder *encoder,
> +				 const struct inno_hdmi_plat_data *plat_data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct inno_hdmi *hdmi;
> +	int ret;
> +
> +	hdmi = inno_hdmi_probe(pdev, plat_data);
> +	if (IS_ERR(hdmi))
> +		return hdmi;
> +
> +	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		return ERR_PTR(ret);

[Severity: High]
This isn't a bug introduced by this patch, but if drm_bridge_attach() fails
(for example, due to probe deferral of another bridge), this returns an
error directly without explicitly destroying the devm_ resources allocated
during inno_hdmi_probe().

Since devm_ allocations are tied to the component device's lifetime rather
than the component bind cycle, won't this leak memory and accumulate
duplicate IRQ registrations on each component bind retry?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-jh7110-clean-send-v2-0-331680c8b9d1@samsung.com?part=4