Re: [PATCH v2 1/3] usb: typec: bm92txx: add Rohm BM92TXX support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Missing cleanup in bm92t_remove and error paths leads to use-after-free and resource leaks.
- [Critical] Unvalidated I2C message lengths lead to stack buffer overflow and out-of-bounds reads.
- [High] Integer underflow in current limit calculation allows over-drawing power from weak sources.
- [High] bm92t_probe succeeds without full initialization when role switch is NULL, causing NULL pointer dereference.
- [High] Use of C bitfields to parse hardware responses breaks on big-endian architectures.
- [High] Misuse of disable_irq() and queue_work() causes permanent IRQ disablement.
- [High] I2C_M_NOSTART is incorrectly set for standard standalone writes, violating I2C protocol.
- [High] Race condition due to unsynchronized access to driver state and global arrays.
- [High] Incorrect cancellation of power_work can leave charging enabled after cable disconnect.
- [Medium] Undeclared variable pd_switch_name causes build failure if CONFIG_SWITCH is enabled.
- [Medium] Incorrect return value check for kstrtouint() breaks debugfs interfaces.
--

commit bc2b1e9499614886b3e416da0258ae455e2169d4
Author: CTCaer <ctcaer@gmail.com>
usb: typec: bm92txx: add Rohm BM92TXX support

This commit introduces the driver for the Rohm BM92TXX USB Type-C and
Power Delivery controllers. It provides extcon, USB role switch support,
and VBUS regulation based on PD negotiations.

diff --git a/drivers/usb/typec/bm92txx.c b/drivers/usb/typec/bm92txx.c
--- a/drivers/usb/typec/bm92txx.c
+++ b/drivers/usb/typec/bm92txx.c
@@ -359,6 +359,13 @@
> +struct __packed pd_object {
> +	unsigned int amp:10;
> +	unsigned int volt:10;
> +	unsigned int info:10;
> +	unsigned int type:2;
> +};

Does the use of C bitfields to parse hardware responses via memcpy() break
protocol parsing on big-endian architectures?

[ ... ]

@@ -550,6 +550,13 @@
> +static int bm92t_write_reg(struct bm92t_info *info,
> +			   unsigned char *buf, unsigned int len)
> +{
> +	struct i2c_msg xfer_msg[1];
> +
> +	xfer_msg[0].addr = info->i2c_client->addr;
> +	xfer_msg[0].len = len;
> +	xfer_msg[0].flags = I2C_M_NOSTART;
> +	xfer_msg[0].buf = buf;

Does setting I2C_M_NOSTART on a single-message transfer violate the I2C
protocol? This might cause standard I2C adapter drivers to reject the write.

[ ... ]

@@ -1285,6 +1285,13 @@
> +static void bm92t_calculate_current_limit(struct bm92t_info *info,
> +	unsigned int voltage, unsigned int amperage)
> +{
> +	int i;
> +	unsigned int charging_limit = amperage;
> +	struct bm92t_platform_data *pdata = info->pdata;
> +
> +	/* Subtract a USB2 or USB3 port current */
> +	if (voltage > 5000)
> +		charging_limit -= (PD_POWER_RESERVE_UA / voltage);
> +	else
> +		charging_limit -= (NON_PD_POWER_RESERVE_UA / voltage);

Can this subtraction underflow since charging_limit is unsigned? If the source
advertises an amperage less than the reserve, this could wrap around to a
large positive value and bypass the min() checks, potentially setting a large
current limit on a weak source.

[ ... ]

@@ -1310,6 +1310,13 @@
> +static void bm92t_extcon_cable_set_init_state(struct work_struct *work)
> +{
> +	struct bm92t_info *info = container_of(to_delayed_work(work),
> +					       struct bm92t_info, oneshot_work);
> +
> +	disable_irq(info->i2c_client->irq);
[ ... ]
> +	queue_work(info->event_wq, &info->work);
> +}

And in the interrupt handler:

@@ -1910,6 +1910,13 @@
> +static irqreturn_t bm92t_interrupt_handler(int irq, void *handle)
> +{
> +	struct bm92t_info *info = handle;
> +
> +	disable_irq_nosync(info->i2c_client->irq);
> +	queue_work(info->event_wq, &info->work);
> +	return IRQ_HANDLED;
> +}

Could this sequence permanently disable the interrupt? If the interrupt handler
and the delayed work both call disable_irq() and then queue_work(), the
workqueue might deduplicate the work item. The work will only execute once and
enable the IRQ once, leaving the disable depth unbalanced.

[ ... ]

@@ -1335,6 +1335,13 @@
> +static bool bm92t_check_pdo(struct bm92t_info *info)
> +{
> +	int i, err, pdos_no;
> +	struct device *dev;
> +	unsigned char pdos[29];
> +	struct pd_object pdo[7];
[ ... ]
> +	err = bm92t_read_reg(info, READ_PDOS_SRC_REG, pdos, sizeof(pdos));
> +	pdos_no = pdos[0] / sizeof(struct pd_object);
[ ... ]
> +	memcpy(pdo, pdos + 1, pdos[0]);

Does this code overflow the pdo[] array? The size pdos[0] is read directly from
the I2C device and used in memcpy() into a 28-byte array without validation.

Similarly, in the event handler:

@@ -1765,6 +1765,13 @@
> +	case VDM_ACCEPT_DISC_SVID_REPLY:
> +		if (bm92t_is_success(alert_data)) {
> +			/* Check discovered SVIDs */
> +			err = bm92t_read_reg(info, INCOMING_VDM_REG, vdm,
> +					     sizeof(vdm));
> +
> +			if (vdm[1] == (VDM_ACK | VDM_CMD_DISC_SVID)) {
> +				dev_info(dev, "Supported SVIDs:\n");
> +				for (i = 0; i < ((vdm[0] - 4) / 2); i++) {
> +					dev_info(dev, "SVID%d %04X\n", i,
> +						 vdm[5 + i * 2] |
> +						 (vdm[6 + i * 2] << 8));
> +				}

Can this loop read past the end of the vdm[] stack buffer? The length vdm[0]
is read from the hardware and used as the loop bound without bounds checking.

[ ... ]

@@ -1550,6 +1550,13 @@
> +static void bm92t_usbhub_led_cfg_wait(struct bm92t_info *info,
> +	unsigned char duty, unsigned char time_on,
> +	unsigned char time_off, unsigned char fade)
> +{
> +	int retries = 100;
> +
> +	if (info->state == NINTENDO_CONFIG_HANDLED) {
> +		bm92t_state_machine(info, VDM_ND_CUSTOM_CMD_SENT);
> +		bm92t_usbhub_led_cfg(info, duty, time_on, time_off, fade);
> +		while (info->state != NINTENDO_CONFIG_HANDLED) {
> +			retries--;
> +			if (retries < 0)
> +				break;
> +			usleep_range(1000, 1500);
> +		}
> +	}
> +}

Is there a race condition here? The driver state and global arrays like
vdm_usbhub_led_msg are modified concurrently from the workqueue, debugfs
write handlers, and PM suspend/resume callbacks without locking.

[ ... ]

@@ -1682,6 +1682,13 @@
> +	if (alert_data & ALERT_PLUGPULL) {
> +		if (!bm92t_is_plugged(status1_data)) { /* Pull event */
> +src_fault:
> +			/* Cancel any pending charging enable work */
> +			cancel_delayed_work(&info->power_work);
> +
> +			/* Disable VBUS in case it's enabled */
> +			bm92t_set_vbus_enable(info, false);
> +
> +			/* Disable charging */
> +			if (info->pd_charging_enabled) {

Should this use cancel_delayed_work_sync()? Using the non-sync version might
leave bm92t_power_work() executing on another CPU, which could re-enable
charging right after the unplug handler disables it.

[ ... ]

@@ -1918,6 +1918,13 @@
> +static void bm92t_remove(struct i2c_client *client)
> +{
> +	struct bm92t_info *info = i2c_get_clientdata(client);
> +
> +#ifdef CONFIG_DEBUG_FS
> +	debugfs_remove_recursive(info->debugfs_root);
> +#endif
> +}

Does this code leak the workqueue, fwnode handles, and switch device?

Furthermore, since the IRQ is non-devm and the work items are not synchronously
canceled, can this cause a use-after-free when the devm-managed memory is freed
but the IRQ or delayed works fire later?

[ ... ]

@@ -2110,6 +2110,13 @@
> +static ssize_t bm92t_cmd_write(struct file *file,
> +		     const char __user *userbuf, size_t count, loff_t *ppos)
> +{
[ ... ]
> +	ret = kstrtouint(buf, 0, &val);
> +
> +	if (ret == 1) {
> +		cmd = val;
> +		bm92t_send_cmd(info, &cmd);
> +	} else {
> +		dev_err(&info->i2c_client->dev, "Cmd syntax is: cmd\n");
> +		return -EINVAL;
> +	}

Does kstrtouint() return 0 on success? Checking for a return value of 1 will
cause the debugfs interfaces to reject valid inputs with -EINVAL.

[ ... ]

@@ -2413,6 +2413,13 @@
> +	info->role_sw = fwnode_usb_role_switch_get(remote);
> +	if (IS_ERR_OR_NULL(info->role_sw)) {
> +		err = PTR_ERR(info->role_sw);
> +		dev_err(&client->dev, "Failed to retrieve fwnode: %d\n", err);
> +		return err;
> +	}
> +
> +	i2c_set_clientdata(client, info);

If fwnode_usb_role_switch_get() returns NULL, IS_ERR_OR_NULL is true and err
is set to PTR_ERR(NULL) which is 0. Does this cause the probe function to
succeed without calling i2c_set_clientdata(), leading to a NULL pointer
dereference later when bm92t_remove() retrieves it?

[ ... ]

@@ -2514,6 +2514,13 @@
> +#ifdef CONFIG_SWITCH
> +	info->pd_switch.name = pd_switch_name;
> +	err = switch_dev_register(&info->pd_switch);
> +	if (err)
> +		dev_err(&client->dev, "failed to register pd switch, err=%d\n", err);
> +#endif

Is pd_switch_name declared anywhere? Referencing an undeclared variable will
cause a build failure when CONFIG_SWITCH is enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-bm92t-v2-0-2145e4f4386b@gmail.com?part=1