Re: [PATCH RFC 2/4] drm/fl2000: Add ITE IT66121 HDMI transmitter support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] drm/fl2000: broken I2C probing and return values in it66121_i2c_probe()
- [High] drm/fl2000: UAF and resource leak from usb_dev devres in fl2000_probe()
- [High] drm/fl2000: unsafe teardown ordering and UAF race in it66121_destroy()
- [Medium] drm/fl2000: duplicate IT66121 bridge driver in it66121_create()
- [Medium] drm/fl2000: EDID extension block corruption in it66121_do_get_edid_block()
- [Medium] drm/fl2000: broken IRQ clear, mask, and error handling in it66121_intr_work()
- [Medium] drm/fl2000: stale regmap and EDID cache in it66121_bridge_attach() on rebind
- [Medium] drm/fl2000: unlocked IT66121_SW_RST RMW race in it66121_abort_ddc_ops()
- [Low] drm/fl2000: out-of-mask value in it66121_configure_afe() register write
- [Low] drm/fl2000: uninitialized status logged in it66121_wait_ddc_ready()
--

commit 1d8fe736e9a996195ce28bd6f054bf21a532ce7c
Author: Angel Gabriel Crispin Valdivia <angelcrispinvaldivia@gmail.com>

drm/fl2000: Add ITE IT66121 HDMI transmitter support

This commit adds support for the ITE IT66121 HDMI transmitter on the FL2000
I2C bus and exposes it as a DRM bridge component with hotplug detection and
EDID reading (via both the IT66121 DDC FIFO and a direct FL2000 I2C fallback).

> diff --git a/drivers/gpu/drm/fl2000/fl2000_drv.c b/drivers/gpu/drm/fl2000/fl2000_drv.c
> index ea089d3c1d09c..107eba92c18d3 100644
> --- a/drivers/gpu/drm/fl2000/fl2000_drv.c
> +++ b/drivers/gpu/drm/fl2000/fl2000_drv.c

[ ... ]

> @@ -52,6 +54,44 @@ static struct fl2000_devs *fl2000_get_devices(struct usb_device *usb_dev)
>  	return devs;
>  }
>  
> +static void fl2000_bridge_release(struct device *dev, void *res)
> +{
> +	struct i2c_client **client = res;
> +
> +	dev_dbg(dev, "Destroying IT66121 bridge\n");
> +	it66121_destroy(*client);
> +}
> +
> +/*
> + * Create the IT66121 bridge and tie its destruction to the usb_device
> + * lifetime: it must survive component master del/add cycles (USB resets) and
> + * die only when the USB device is actually gone. The devres is released
> + * before the I2C adapter devres (LIFO), so the client is destroyed while its
> + * adapter still exists.
> + */
> +static int fl2000_bridge_create(struct usb_device *usb_dev, struct fl2000_devs *devs)
> +{
> +	struct i2c_client *client;
> +	struct i2c_client **holder;
> +
> +	holder = devres_alloc(fl2000_bridge_release, sizeof(*holder), GFP_KERNEL);
> +	if (!holder)
> +		return -ENOMEM;
> +
> +	client = it66121_create(devs->adapter);
> +	if (IS_ERR(client)) {
> +		dev_err(&usb_dev->dev, "Cannot create HDMI bridge (%pe)\n", client);
> +		devres_free(holder);
> +		return -ENODEV;
> +	}
> +
> +	*holder = client;
> +	devres_add(&usb_dev->dev, holder);
> +	devs->bridge_client = client;
> +
> +	return 0;
> +}

[ ... ]

> @@ -96,6 +150,12 @@ static void fl2000_disconnect(struct usb_interface *interface)
>  	if (!devs)
>  		return;
>  
> +	/*
> +	 * The IT66121 bridge is intentionally NOT destroyed here: disconnect
> +	 * also runs for the transient unbind/rebind cycles of
> +	 * usb_reset_device(), where the usb_device survives. Its destruction is
> +	 * a devres of the usb_device, see fl2000_bridge_create().
> +	 */

[Severity: High]
Can attaching fl2000_bridge_release to &usb_dev->dev in
fl2000_bridge_create() cause a use-after-free on driver unbind or module
unload, or leak resources when probing an interface fails?

fl2000_driver is a struct usb_driver that binds to struct usb_interface
(&interface->dev), while the parent struct usb_device (&usb_dev->dev) is
bound to usb_generic_driver in usbcore.

When fl2000_driver is unbound via sysfs or unloaded via rmmod fl2000 while
the USB device stays plugged in (or if fl2000_probe() fails on an
interface), usb_unbind_interface() only releases devres on &interface->dev.
Because &usb_dev->dev remains bound to usb_generic_driver,
devres_release_all(&usb_dev->dev) does not run until physical USB unplug, and
fl2000_disconnect() intentionally skips tearing down devs->bridge_client and
devs (allocated on &usb_dev->dev in fl2000_get_devices()).

If the fl2000 module is unloaded while the USB dongle is connected:
- priv->bridge remains in the global DRM bridge_list with bridge.funcs
  pointing to unmapped it66121_bridge_funcs
- priv->client->dev remains in the global component_list with ops pointing
  to unmapped it66121_component_ops
- priv->work_queue stays active running it66121_intr_work() in unmapped
  module text
- &usb_dev->dev retains devres release callbacks (such as
  fl2000_bridge_release) pointing into freed module memory when the USB
  device is later unplugged.

Should the lifecycle of devs->bridge_client and devs instead be reference-
counted across the bound USB interfaces and torn down when active_if drops to
0 outside of a reset (for example using pre_reset and post_reset callbacks)?

> diff --git a/drivers/gpu/drm/fl2000/fl2000_it66121.c b/drivers/gpu/drm/fl2000/fl2000_it66121.c
> new file mode 100644
> index 0000000000000..208f92823954a
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_it66121.c
> @@ -0,0 +1,1030 @@

[ ... ]

> +static int it66121_configure_afe(struct it66121_priv *priv, int clock_khz)
> +{
> +	int ret;
> +
> +	ret = regmap_write(priv->regmap, IT66121_AFE_DRV_CONTROL, IT66121_AFE_RST);
> +	if (ret)
> +		return ret;
> +
> +	/* Values from the vendor reference driver, the bits are undocumented */
> +	if (clock_khz > 80000) {
> +		ret = regmap_write_bits(priv->regmap, IT66121_AFE_XP_CONTROL, 0x90, 0x80);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write_bits(priv->regmap, IT66121_AFE_IP_CONTROL_1, 0x89, 0x80);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write_bits(priv->regmap, IT66121_AFE_IP_CONTROL_3, 0x10, 0x80);
> +		if (ret)
> +			return ret;

[Severity: Low]
In it66121_configure_afe(), when clock_khz > 80000, regmap_write_bits() is
called on IT66121_AFE_IP_CONTROL_3 with mask 0x10 and value 0x80.

Since regmap_update_bits_base() computes (orig & ~mask) | (val & mask), and
(0x80 & 0x10) is 0, bit 7 (0x80) is masked out and only bit 4 (0x10) is
cleared. Was the value meant to be 0x00 to clear bit 4 (matching
drivers/gpu/drm/bridge/ite-it66121.c), or was bit 7 meant to be included in
the mask?

[ ... ]

> +static int it66121_wait_ddc_ready(struct it66121_priv *priv)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	/*
> +	 * Wait for completion only. On this hardware the FL2000 I2C master and
> +	 * the IT66121 DDC master share the physical bus, so 'wait bus' and
> +	 * 'arbitration lose' flags are live/latched transients of the engine
> +	 * retrying on a contended bus, not terminal errors - the only reliable
> +	 * terminal signal is TX_DONE (or the timeout).
> +	 */
> +	ret = regmap_read_poll_timeout(priv->regmap, IT66121_DDC_STATUS, val,
> +				       val & DDC_STATUS_TX_DONE, EDID_SLEEP, EDID_TIMEOUT);
> +	if (ret) {
> +		dev_err(&priv->client->dev, "DDC not done (%d), status 0x%02X\n", ret, val);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
In it66121_wait_ddc_ready(), if the first regmap_read() inside
regmap_read_poll_timeout() fails with a negative error code (for example due
to a USB or I2C transfer error), regmap_read_poll_timeout() returns
immediately without writing to val.

Won't dev_err() then read and print the uninitialized stack variable val?

[ ... ]

> +static int it66121_abort_ddc_ops(struct it66121_priv *priv)
> +{
> +	int ret;
> +	unsigned int swreset, cpdesire;
> +
> +	/*
> +	 * The HDCP engine shares the DDC master and holds the bus after chip
> +	 * reset (DDC status reports 'wait bus' + 'arbitration lose', 0x1A).
> +	 * Follow the ITE reference sequence, as the ite-it66121 driver does:
> +	 * clear HDCP CP_DESIRE, reset the HDCP engine and re-select the host as
> +	 * DDC master before issuing the abort command.
> +	 */
> +	ret = regmap_read(priv->regmap, IT66121_SW_RST, &swreset);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(priv->regmap, IT66121_HDCP_CONTROL, &cpdesire);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, IT66121_HDCP_CONTROL,
> +			   cpdesire & ~IT66121_HDCP_CP_DESIRE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, IT66121_SW_RST, swreset | IT66121_SW_HDCP_RST);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Can it66121_abort_ddc_ops() race with it66121_set_mode() when modifying
IT66121_SW_RST?

In it66121_get_edid_block() -> it66121_do_get_edid_block() ->
it66121_abort_ddc_ops(), IT66121_SW_RST is read into swreset, two USB I2C
transfers are performed on IT66121_HDCP_CONTROL, and then
swreset | IT66121_SW_HDCP_RST is written back under priv->ddc_lock.

Meanwhile, it66121_bridge_atomic_enable() -> it66121_set_mode() runs in the
atomic commit tail without holding priv->ddc_lock, setting and clearing
IT66121_SW_REF_RST_HDMITX | IT66121_SW_HDMI_VID_RST in IT66121_SW_RST:

    regmap_write_bits(priv->regmap, IT66121_SW_RST,
                      IT66121_SW_REF_RST_HDMITX | IT66121_SW_HDMI_VID_RST,
                      IT66121_SW_REF_RST_HDMITX | IT66121_SW_HDMI_VID_RST);
    ...
    regmap_write_bits(priv->regmap, IT66121_SW_RST,
                      IT66121_SW_REF_RST_HDMITX | IT66121_SW_HDMI_VID_RST, 0);

If it66121_abort_ddc_ops() reads swreset while the HDMI reset bits are set
and writes swreset | IT66121_SW_HDCP_RST back after it66121_set_mode() clears
them, won't the transmitter remain stuck in reset? Using regmap_write_bits()
for IT66121_SW_HDCP_RST (and IT66121_HDCP_CP_DESIRE) would keep each update
atomic under the regmap lock.

[ ... ]

> +static void it66121_intr_work(struct work_struct *work_item)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct delayed_work *dwork = to_delayed_work(work_item);
> +	struct it66121_priv *priv = container_of(dwork, struct it66121_priv, work);
> +	/* Do not use bridge.dev here: it is NULL while the bridge is detached */
> +	struct device *dev = &priv->client->dev;
> +
> +	ret = regmap_field_read(priv->irq_pending, &val);
> +	if (ret) {
> +		dev_err(dev, "Cannot read interrupt status (%d)\n", ret);
> +		return;
> +	}
> +
> +	if (val) {
> +		ret = regmap_read(priv->regmap, IT66121_INT_STATUS_1, &val);
> +		if (ret) {
> +			dev_err(dev, "Cannot read IT66121_INT_STATUS_1 (%d)\n", ret);
> +			return;
> +		}
> +
> +		regmap_field_write(priv->clr_irq, 1);
> +	}
> +
> +	queue_delayed_work(priv->work_queue, &priv->work, msecs_to_jiffies(IRQ_POLL_INTRVL));
> +}

[Severity: Medium]
There seem to be three issues with the interrupt polling in
it66121_intr_work() and it66121_start_interrupts():

1. regmap_field_write(priv->clr_irq, 1) calls regmap_field_update_bits_base()
   with force = false. If bit 0 of IT66121_SYS_STATUS (0x0E) already reads as
   1, regmap skips the bus write and the interrupt is not cleared. Should
   this use regmap_field_force_write(priv->clr_irq, 1)?

2. it66121_start_interrupts() only unmasks IT66121_MASK_DDC in
   IT66121_INT_MASK_1, leaving HPD (IT66121_INT_STATUS_1_HPD) masked, and
   it66121_intr_work() reads IT66121_INT_STATUS_1 without writing to
   IT66121_INT_CLEAR_1 to clear latched interrupt sources.

3. If either regmap_field_read() or regmap_read() fails once due to a
   transient USB or I2C transfer error, it66121_intr_work() returns before
   queue_delayed_work(), permanently stopping the polling work until a
   system suspend/resume cycle.

> +/*
> + * EDID read through the IT66121 DDC master (EDID FIFO command), for boards
> + * where the monitor's DDC lines are wired to the IT66121.
> + */
> +static int it66121_do_get_edid_block(struct it66121_priv *priv, u8 *buf, unsigned int block,
> +				     size_t len)
> +{
> +	int ret;
> +	size_t remain = len;
> +	unsigned int val;
> +	unsigned int segment = block >> 1;
> +	unsigned int offset = (block & 1) ? 128 : 0;
> +	static const u8 header[EDID_LOSS_LEN] = { 0x00, 0xFF, 0xFF };
> +
> +	/* Abort any ongoing DDC operation */
> +	ret = it66121_abort_ddc_ops(priv);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Statically fill first 3 bytes (due to EDID reading HW bug they are
> +	 * lost, but they are constant in any valid EDID header).
> +	 */
> +	while (offset < EDID_LOSS_LEN && remain > 0) {
> +		*(buf++) = header[offset];
> +		remain--;
> +		offset++;
> +	}
> +
> +	while (remain > 0) {
> +		/* Add bytes that will be lost during EDID read */
> +		size_t size = remain + EDID_LOSS_LEN;
> +
> +		/* ... and check size fits FIFO */
> +		size = min_t(size_t, size, EDID_FIFO_SIZE);
> +
> +		ret = it66121_clear_ddc_fifo(priv);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(priv->regmap, IT66121_DDC_ADDRESS, EDID_DDC_ADDR);
> +		if (ret)
> +			return ret;
> +
> +		/* Account 3 bytes that will be lost */
> +		ret = regmap_write(priv->regmap, IT66121_DDC_OFFSET, offset - EDID_LOSS_LEN);

[Severity: Medium]
Does it66121_do_get_edid_block() corrupt even-numbered EDID extension blocks
and misalign FIFO reads?

First, for any even-numbered extension block (such as block 2, where
segment = 1 and offset = 0), the loop:

    while (offset < EDID_LOSS_LEN && remain > 0) {
        *(buf++) = header[offset];
        remain--;
        offset++;
    }

checks only offset < EDID_LOSS_LEN without checking block == 0, overwriting
the first 3 bytes of the extension block (the extension tag, revision, and
DTD offset) with the base EDID block 0 header bytes { 0x00, 0xFF, 0xFF }.

Second, IT66121_DDC_STATUS (0x16) and IT66121_DDC_RD_FIFO (0x17) lie in the
same 4-byte aligned dword (0x14..0x17). Because fl2000_i2c_xfer() executes
every 1-byte register read as a 4-byte aligned dword read via
fl2000_i2c_read_dword(), each poll of IT66121_DDC_STATUS in
it66121_wait_ddc_ready() also reads IT66121_DDC_RD_FIFO (0x17) and pops a
byte from the DDC FIFO whenever data is available. With EDID_SLEEP set to
5000 us, the number of polls in it66121_wait_ddc_ready() is not guaranteed
to equal the hardcoded EDID_LOSS_LEN (3), shifting the remaining bytes read
from IT66121_DDC_RD_FIFO.

[ ... ]

> +static int it66121_bridge_attach(struct drm_bridge *bridge, struct drm_encoder *encoder,
> +				 enum drm_bridge_attach_flags flags)
> +{
> +	struct it66121_priv *priv = container_of(bridge, struct it66121_priv, bridge);
> +
> +	/* The connector is the display driver's (drm_bridge_connector_init()) */
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +		drm_err(bridge->dev, "The IT66121 bridge does not create a connector\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!bridge->encoder) {
> +		drm_err(bridge->dev, "Parent encoder object not found\n");
> +		return -ENODEV;
> +	}
> +
> +	it66121_init_chip(priv);
> +
> +	it66121_start_interrupts(priv);
> +
> +	drm_dbg_kms(bridge->dev, "Bridge attached\n");
> +
> +	return 0;
> +}

[ ... ]

> +void it66121_resume(struct drm_bridge *bridge)
> +{
> +	struct it66121_priv *priv = container_of(bridge, struct it66121_priv, bridge);
> +
> +	regcache_drop_region(priv->regmap, 0, it66121_regmap_config.max_register);
> +	it66121_init_chip(priv);
> +	it66121_start_interrupts(priv);
> +
> +	/* Another monitor may have been plugged in meanwhile */
> +	WRITE_ONCE(priv->edid_stale, true);
> +}

[Severity: Medium]
Since the IT66121 bridge persists across usb_reset_device() and USB interface
unbind/rebind cycles, should it66121_bridge_attach() also drop the regmap
cache and mark priv->edid_stale = true like it66121_resume() does?

When the bridge is re-attached on rebind (and after it66121_init_chip()
issues IT66121_SW_REF_RST_HDMITX), priv->regmap still holds cached register
values in REGCACHE_MAPLE from before the reset (including the bank selector
register IT66121_SYS_CONTROL and non-volatile Bank 0/1 registers), which can
cause regmap_write_bits() and bank selection to skip necessary hardware
writes. In addition, priv->edid_stale is not set to true on re-attach.

[ ... ]

> +static int it66121_i2c_probe(struct i2c_adapter *adapter, unsigned short address)
> +{
> +	int ret;
> +	u8 id_regs[] = { IT66121_VENDOR_ID_1, IT66121_VENDOR_ID_2, IT66121_DEVICE_ID_1,
> +			 IT66121_DEVICE_ID_2 };
> +	u8 id[ARRAY_SIZE(id_regs)];
> +	u16 vendor, device;
> +
> +	dev_dbg(&adapter->dev, "Detecting IT66121 at address 0x%X on %s\n", address,
> +		adapter->name);
> +
> +	/* We rely on full I2C protocol + 1 byte SMBUS read for detection */
> +	ret = i2c_check_functionality(adapter, I2C_FUNC_I2C | I2C_FUNC_SMBUS_READ_BYTE);
> +	if (!ret) {
> +		dev_dbg(&adapter->dev, "Adapter does not support I2C functions properly\n");
> +		return -ENODEV;
> +	}
> +
> +	for (int i = 0; i < ARRAY_SIZE(id_regs); i++) {
> +		struct i2c_msg msgs[] = {
> +			{ .addr = address, .flags = 0, .len = 1, .buf = &id_regs[i] },
> +			{ .addr = address, .flags = I2C_M_RD, .len = 1, .buf = &id[i] }
> +		};
> +
> +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +		if (ret < 0) {
> +			dev_err(&adapter->dev, "I2C transfer failed (%d)\n", ret);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	vendor = get_unaligned_le16(&id[0]);
> +	device = get_unaligned_le16(&id[2]);
> +	if (vendor != VENDOR_ID || (device & ~REVISION_MASK) != DEVICE_ID) {
> +		dev_dbg(&adapter->dev, "IT66121 not found (0x%X-0x%X)\n", vendor, device);
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(&adapter->dev, "IT66121 found, revision %d\n",
> +		(device & REVISION_MASK) >> REVISION_SHIFT);
> +
> +	return 0;
> +}

[Severity: High]
Aren't the return values and functionality check in it66121_i2c_probe()
inverted for i2c_new_scanned_device()?

In drivers/i2c/i2c-core-base.c, i2c_new_scanned_device() checks:

    if (probe(adap, addr_list[i]))
        break;

so the custom probe callback must return 1 (non-zero) when a device is found
and 0 when it is not found. Instead, it66121_i2c_probe() returns -ENODEV
(non-zero) on error or ID mismatch and 0 on a valid match.

Furthermore, it66121_i2c_probe() checks for I2C_FUNC_SMBUS_READ_BYTE even
though the loop below uses raw i2c_transfer() messages. In
drivers/gpu/drm/fl2000/fl2000_i2c.c, fl2000_i2c_func() only advertises:

    return I2C_FUNC_I2C | I2C_FUNC_NOSTART;

Because I2C_FUNC_SMBUS_READ_BYTE is not set, i2c_check_functionality() always
returns 0, causing it66121_i2c_probe() to immediately return -ENODEV without
performing any I2C transfers on the bus. i2c_new_scanned_device() treats that
non-zero -ENODEV return value as a positive match at the first address
(0x98 >> 1 = 0x4c), unconditionally instantiating the IT66121 client even on
FL2000 boards without an IT66121 and never scanning the second address
(0x9A >> 1 = 0x4d).

> +static struct i2c_client *it66121_i2c_init(struct i2c_adapter *adapter)
> +{
> +	/*
> +	 * Not "it66121": that name would make the ite-it66121 driver, if built,
> +	 * try to bind this client and release our devres when its probe fails.
> +	 */
> +	struct i2c_board_info board_info = { I2C_BOARD_INFO("fl2000-it66121", 0) };
> +
> +	/* According to datasheet IT66121 addresses are 0x98 or 0x9A including cmd */
> +	const unsigned short it66121_addr[] = { (0x98 >> 1), (0x9A >> 1), I2C_CLIENT_END };
> +
> +	return i2c_new_scanned_device(adapter, &board_info, it66121_addr, it66121_i2c_probe);
> +}

[Severity: Medium]
This isn't a runtime bug in itself, but the kernel already has an in-tree DRM
bridge driver for the ITE IT66121 in drivers/gpu/drm/bridge/ite-it66121.c.
Could drivers/gpu/drm/bridge/ite-it66121.c be extended to support non-OF
instantiation and a custom DDC fallback rather than duplicating the IT66121
driver in drivers/gpu/drm/fl2000/fl2000_it66121.c?

> +void it66121_destroy(struct i2c_client *client)
> +{
> +	struct drm_bridge *bridge;
> +	struct it66121_priv *priv;
> +
> +	if (!client)
> +		return;
> +
> +	bridge = dev_get_drvdata(&client->dev);
> +	priv = container_of(bridge, struct it66121_priv, bridge);
> +
> +	cancel_delayed_work_sync(&priv->work);
> +
> +	destroy_workqueue(priv->work_queue);
> +
> +	component_del(&priv->client->dev, &it66121_component_ops);
> +
> +	drm_edid_free(priv->drm_edid);
> +	priv->drm_edid = NULL;
> +
> +	drm_bridge_remove(&priv->bridge);
> +
> +	/*
> +	 * Drops the devm reference to the bridge; priv is freed together with
> +	 * the bridge once DRM releases the remaining references.
> +	 */
> +	i2c_unregister_device(priv->client);
> +}
> +
> +struct i2c_client *it66121_create(struct i2c_adapter *adapter)
> +{

[ ... ]

> +	priv->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> +	priv->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
> +
> +	drm_bridge_add(&priv->bridge);
> +
> +	/* Setup work queue for interrupt processing work */
> +	priv->work_queue = alloc_ordered_workqueue("it66121_work", 0);
> +	if (!priv->work_queue) {
> +		drm_bridge_remove(&priv->bridge);
> +		i2c_unregister_device(client);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Store bridge pointer at I2C client device for further binding */
> +	dev_set_drvdata(&priv->client->dev, &priv->bridge);
> +
> +	/* Bind component of I2C client to master I2C adapter */
> +	ret = component_add(&priv->client->dev, &it66121_component_ops);
> +	if (ret) {
> +		dev_err(&client->dev, "Cannot register IT66121 component (%d)\n", ret);
> +		destroy_workqueue(priv->work_queue);
> +		drm_bridge_remove(&priv->bridge);
> +		i2c_unregister_device(client);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return client;
> +}

[Severity: High]
Can the teardown ordering in it66121_destroy() and the error path in
it66121_create() trigger a use-after-free?

1. In it66121_destroy(), cancel_delayed_work_sync(&priv->work) and
   destroy_workqueue(priv->work_queue) are called before
   component_del(&priv->client->dev, &it66121_component_ops) and
   drm_bridge_remove(&priv->bridge). Until component_del() finishes, a
   concurrent attach or it66121_resume() call can still invoke
   it66121_start_interrupts() and queue work onto the destroyed
   priv->work_queue.

2. In it66121_create(), drm_bridge_add(&priv->bridge) is called before
   priv->work_queue is allocated, and if component_add() fails after
   it66121_bind() -> drm_bridge_attach() -> it66121_start_interrupts() has
   already queued priv->work during master bring-up, the error path calls
   destroy_workqueue(priv->work_queue) without first calling
   cancel_delayed_work_sync(&priv->work).

3. If userspace holds an open DRM fd across USB unplug, the DRM device and
   encoder can outlive it66121_destroy() until the final drm_dev_put().
   However, it66121_destroy() frees priv->drm_edid without locking against
   it66121_bridge_edid_read(), and i2c_unregister_device(priv->client)
   releases all devm resources on &client->dev (priv->regmap, priv->hpd,
   priv->irq_pending, priv->clr_irq) and frees priv->client without calling
   drm_bridge_unplug() or guarding bridge callbacks with drm_bridge_enter().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-fl2000-upstream-v1-0-bb163ecb2309@gmail.com?part=2