Re: [PATCH RFC 1/4] drm/fl2000: Add USB driver with register and I2C access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/fl2000: UAF and leaks attaching devres to usb_dev in fl2000_get_devices()
- [High] drm/fl2000: unvalidated message length and flags in fl2000_i2c_xfer()
- [Medium] drm/fl2000: destructive second read of status reg in fl2000_read_status()
- [Medium] drm/fl2000: unlocked FL2000_VGA_I2C_SC_REG update in fl2000_usb_magic()
--

commit 65acb73d91d8bb3b60de43aba3fc31aba51bb815
Author: Angel Gabriel Crispin Valdivia <angelcrispinvaldivia@gmail.com>

drm/fl2000: Add USB driver with register and I2C access

This patch adds the USB driver for the Fresco Logic FL2000DX display
controller, binding its three USB interfaces, setting up a regmap over
vendor control transfers, and registering an I2C adapter for the chip's
I2C engine.

> diff --git a/drivers/gpu/drm/fl2000/fl2000_drv.c b/drivers/gpu/drm/fl2000/fl2000_drv.c
> new file mode 100644
> index 0000000000000..ea089d3c1d09c
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_drv.c
> @@ -0,0 +1,150 @@

[ ... ]

> +/* Devices that are independent of interfaces, created for the lifetime of the USB device */
> +struct fl2000_devs {
> +	struct regmap *regmap;
> +	struct i2c_adapter *adapter;
> +	int active_if;
> +};
> +
> +static struct fl2000_devs *fl2000_get_devices(struct usb_device *usb_dev)
> +{
> +	struct fl2000_devs *devs;
> +
> +	devs = devm_kzalloc(&usb_dev->dev, sizeof(*devs), GFP_KERNEL);
> +	if (!devs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	devs->regmap = fl2000_regmap_init(usb_dev);
> +	if (IS_ERR(devs->regmap)) {
> +		dev_err(&usb_dev->dev, "Cannot initialize regmap (%pe)\n", devs->regmap);
> +		return ERR_CAST(devs->regmap);
> +	}
> +
> +	devs->adapter = fl2000_i2c_init(usb_dev);
> +	if (IS_ERR(devs->adapter)) {
> +		dev_err(&usb_dev->dev, "Cannot initialize I2C adapter (%pe)\n", devs->adapter);
> +		return ERR_CAST(devs->adapter);
> +	}
> +
> +	dev_set_drvdata(&usb_dev->dev, devs);
> +
> +	return devs;
> +}

[ ... ]

> +static void fl2000_disconnect(struct usb_interface *interface)
> +{
> +	u8 iface_num = interface->cur_altsetting->desc.bInterfaceNumber;
> +	struct usb_device *usb_dev = interface_to_usbdev(interface);
> +	struct fl2000_devs *devs = dev_get_drvdata(&usb_dev->dev);
> +
> +	if (!devs)
> +		return;
> +
> +	switch (iface_num) {
> +	case FL2000_USBIF_AVCONTROL:
> +	case FL2000_USBIF_STREAMING:
> +	case FL2000_USBIF_INTERRUPT:
> +		devs->active_if &= ~BIT(iface_num);
> +		break;

[Severity: High]
What cleans up devs, devs->regmap, devs->adapter, and usb_dev->dev
driver_data when the driver is unbound or unloaded while the USB device
remains plugged in?

fl2000_driver binds to struct usb_interface devices (&interface->dev), while
the parent struct usb_device (&usb_dev->dev) stays bound to
usb_generic_driver for the entire physical connection. However,
fl2000_get_devices(), fl2000_regmap_init(), and fl2000_i2c_init() attach all
devm allocations and registrations to &usb_dev->dev:

  fl2000_probe()
    fl2000_get_devices()
      devm_kzalloc(&usb_dev->dev, ...)
      fl2000_regmap_init()
        devm_regmap_init(&usb_dev->dev, ...)
      fl2000_i2c_init()
        devm_kzalloc(&usb_dev->dev, ...)
        devm_i2c_add_adapter(&usb_dev->dev, adapter)
      dev_set_drvdata(&usb_dev->dev, devs)

When fl2000_driver is unbound from the interfaces (for example via rmmod
fl2000 or sysfs unbind), device_unbind_cleanup() only releases devres on
&interface->dev, not on &usb_dev->dev, and fl2000_disconnect() only clears
BIT(iface_num) in devs->active_if.

After module unload, the I2C adapter and regmap remain registered with
callbacks and structures (such as fl2000_i2c_algorithm, fl2000_i2c_quirks,
fl2000_regmap_config, fl2000_reg_read, and fl2000_reg_write) pointing into
freed module memory, and dev_get_drvdata(&usb_dev->dev) retains the stale
pointer. Won't any subsequent I2C transfer, regmap debugfs access, module
reload, or USB unplug trigger a use-after-free?

Also, if fl2000_i2c_init() fails in fl2000_get_devices() (or if
fl2000_probe() fails on an unexpected interface), really_probe() only cleans
up &interface->dev, leaking the devm allocations attached to &usb_dev->dev
on each interface probe attempt. Should the shared state be refcounted across
the bound interfaces and explicitly torn down when the last interface
disconnects (or on probe failure)?

> diff --git a/drivers/gpu/drm/fl2000/fl2000_i2c.c b/drivers/gpu/drm/fl2000/fl2000_i2c.c
> new file mode 100644
> index 0000000000000..eadbe0f679eb4
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_i2c.c
> @@ -0,0 +1,182 @@

[ ... ]

> +static int fl2000_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> +{
> +	int ret;
> +	bool read;
> +	u16 addr;
> +	u8 idx;
> +	u8 offset;
> +	u32 data;
> +
> +	addr = msgs[0].addr;
> +	offset = msgs[0].buf[0] & I2C_XFER_ADDR_MASK;
> +	idx = msgs[0].buf[0] - offset;
> +
> +	/*
> +	 * We expect the following:
> +	 * - 2 messages, each 1 byte, first write then read
> +	 * - 1 message, 2 bytes, write
> +	 */
> +	if (num == 2) {
> +		read = true;
> +	} else if (num == 1) {
> +		if (msgs[0].len == 2 && !(msgs[0].flags & I2C_M_RD))
> +			read = false;
> +		else
> +			return -EOPNOTSUPP;
> +	} else {
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/*
> +	 * The FL2000 I2C engine only transfers aligned dwords, so the offset is aligned down to a
> +	 * 4-byte boundary and the requested byte is picked out of (or merged into) the dword.
> +	 */
> +	if (read) {
> +		ret = fl2000_i2c_read_dword(adapter->algo_data, addr, offset, &data);
> +		if (ret)
> +			return ret;
> +
> +		msgs[1].buf[0] = data >> (idx * 8);

[ ... ]

> +static u32 fl2000_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_NOSTART;
> +}
> +
> +static const struct i2c_algorithm fl2000_i2c_algorithm = {
> +	.xfer = fl2000_i2c_xfer,
> +	.functionality = fl2000_i2c_func,
> +};
> +
> +static const struct i2c_adapter_quirks fl2000_i2c_quirks = {
> +	/* Combined transfers only: an address write, then data, on the same address */
> +	.flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR,
> +	.max_num_msgs = I2C_CMESSAGES_NUM,
> +	.max_write_len = 2 * I2C_REG_DATA_SIZE,
> +	.max_read_len = I2C_REG_DATA_SIZE,
> +	.max_comb_1st_msg_len = I2C_REG_ADDR_SIZE,
> +	.max_comb_2nd_msg_len = I2C_REG_DATA_SIZE,
> +};

[Severity: High]
Can fl2000_i2c_xfer() dereference a NULL or ZERO_SIZE_PTR buffer on
zero-length messages, or misclassify a 2-message write as a read?

At the start of fl2000_i2c_xfer(), msgs[0].buf[0] is read before checking
msgs[0].len. Because fl2000_i2c_quirks.flags does not set
I2C_AQ_NO_ZERO_LEN, i2c_check_for_quirks() allows messages with len == 0
(for example, SMBus quick transfers from i2cdetect where msgs[0].buf is
NULL, or zero-length I2C_RDWR ioctls where msgs[i].buf is ZERO_SIZE_PTR),
which will fault on msgs[0].buf[0] or msgs[1].buf[0].

In addition, when num == 2, fl2000_i2c_xfer() sets read = true without
checking that msgs[1].flags & I2C_M_RD is set or that both messages have
len == 1. Meanwhile, fl2000_i2c_func() advertises I2C_FUNC_NOSTART and
fl2000_i2c_quirks.flags omits I2C_AQ_COMB_READ_SECOND (i.e.
I2C_AQ_COMB_WRITE_THEN_READ). If a caller issues a 2-message write (such as
regmap_i2c_gather_write() using I2C_M_NOSTART, or via i2c-dev), won't
fl2000_i2c_xfer() perform a hardware read instead of a write and overwrite
the caller's write buffer msgs[1].buf[0]?

> diff --git a/drivers/gpu/drm/fl2000/fl2000_registers.c b/drivers/gpu/drm/fl2000/fl2000_registers.c
> new file mode 100644
> index 0000000000000..231814471d20a
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_registers.c
> @@ -0,0 +1,320 @@

[ ... ]

> +int fl2000_usb_magic(struct usb_device *usb_dev)
> +{
> +	struct regmap *regmap = dev_get_regmap(&usb_dev->dev, NULL);
> +	u32 val;
> +
> +	val = FL2000_VGA_I2C_SC_MONITOR_DETECT | FL2000_VGA_I2C_SC_EDID_DETECT;
> +	regmap_write_bits(regmap, FL2000_VGA_I2C_SC_REG, val, val);

[Severity: Medium]
Can updating FL2000_VGA_I2C_SC_REG in fl2000_usb_magic() race with an active
I2C transfer or trigger an unintended I2C transaction?

In fl2000_i2c_dword(), writing FL2000_VGA_I2C_SC_REG with bit 31
(FL2000_VGA_I2C_SC_DONE) cleared to 0 initiates an I2C transfer using the
address, command, and offset in bits 15:0, and callers serialize the
multi-step sequence (including polling across 200us sleeps in
regmap_read_poll_timeout()) with i2c_lock_bus().

Here in fl2000_usb_magic(), regmap_write_bits() performs a read-modify-write
on the volatile FL2000_VGA_I2C_SC_REG without holding i2c_lock_bus() and
without setting FL2000_VGA_I2C_SC_DONE in the value written back. If an I2C
transfer is in progress when fl2000_usb_magic() runs, or if
FL2000_VGA_I2C_SC_DONE reads back as 0 after reset, won't
regmap_write_bits() write FL2000_VGA_I2C_SC_DONE = 0 back to
FL2000_VGA_I2C_SC_REG and corrupt or re-trigger an I2C transaction?

[ ... ]

> +/*
> + * Read the status register. Its event bits clear on read, so whoever reads it owns the events it
> + * returns: callers must not drop them.
> + */
> +int fl2000_read_status(struct usb_device *usb_dev, u32 *status)
> +{
> +	struct regmap *regmap = dev_get_regmap(&usb_dev->dev, NULL);
> +	unsigned int val;
> +	u32 mask;
> +	int ret;
> +
> +	ret = regmap_read(regmap, FL2000_VGA_STATUS_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* LBUF issues are recoverable */
> +	mask = val & (FL2000_VGA_STATUS_LBUF_OVERFLOW | FL2000_VGA_STATUS_LBUF_UNDERFLOW);
> +	if (mask)
> +		regmap_write_bits(regmap, FL2000_VGA_STATUS_REG, mask, val);

[Severity: Medium]
Does calling regmap_write_bits() on FL2000_VGA_STATUS_REG in
fl2000_read_status() perform a second hardware read that silently drops
pending event bits?

FL2000_VGA_STATUS_REG has seven read-self-clearing event bits (such as
FL2000_VGA_STATUS_HDMI_EVENT, FL2000_VGA_STATUS_MONITOR_EVENT, and
FL2000_VGA_STATUS_EDID_EVENT) and is marked volatile in
FL2000_REG_VOLATILE().

When FL2000_VGA_STATUS_LBUF_OVERFLOW or FL2000_VGA_STATUS_LBUF_UNDERFLOW is
set in val, fl2000_read_status() calls regmap_write_bits() on
FL2000_VGA_STATUS_REG. Because the register is volatile (and precious_reg
only prevents debugfs reads, not _regmap_update_bits()), regmap_write_bits()
issues a second _regmap_read() of FL2000_VGA_STATUS_REG before writing it
back. Any self-clearing event bits that assert between the initial
regmap_read() and the internal read in regmap_write_bits() will be cleared
by that second read and discarded instead of being returned in *status.
Should this use regmap_write() with val directly instead of
regmap_write_bits()?

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