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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Stack buffer overflow when parsing untrusted I2C response length
- [High] Severe resource leaks and use-after-free vulnerability in driver removal
- [High] NULL pointer dereference when USB role switch is not present
- [High] Flawed I2C error checking ignores read failures and uses uninitialized data
- [High] Integer underflow in current limit calculation configures max draw on low-power sources
- [High] Unconditional IRQ enablement and disablement crashes systems operating in polling mode
- [High] Out-of-bounds stack read when parsing Vendor Defined Messages (VDMs)
- [High] Permanent IRQ disable due to workqueue coalescing
- [Medium] Debugfs writes are completely broken due to incorrect kstrtouint error checking
- [Medium] Build failure due to undeclared identifier when CONFIG_SWITCH is enabled
- [Medium] Memory leak of fwnode references during probe
- [Medium] I2C protocol violation via improper use of I2C_M_NOSTART flag
--

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

This commit adds a driver for the Rohm Semiconductor BM92TXX family of USB 
Type-C and Power Delivery controllers. It exposes the controller through extcon 
and a USB role switch, and applies per-PDO charging current limits.

> diff --git a/drivers/usb/typec/bm92txx.c b/drivers/usb/typec/bm92txx.c
> index 0000000000000..39e20a4394c11
> --- /dev/null
> +++ b/drivers/usb/typec/bm92txx.c
> @@ -0,0 +1,2634 @@
> [ ... ]
> +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;

Is it valid to set the I2C_M_NOSTART flag for a single-message transfer here?
This flag instructs the master controller to skip sending the START condition
and slave address, which is a protocol violation for an independent register
write and is rejected by most upstream I2C master drivers with -EOPNOTSUPP.

> +	xfer_msg[0].buf = buf;
> +
> +	dev_dbg(&info->i2c_client->dev,
> +		     "write reg cmd = 0x%02X len = %d\n", buf[0], len);
> +	return (i2c_transfer(info->i2c_client->adapter, xfer_msg, 1) != 1);
> +}
> +
> +static int bm92t_read_reg(struct bm92t_info *info,
> +			  unsigned char reg, unsigned char *buf, int num)
> +{
> [ ... ]
> +	err = i2c_transfer(info->i2c_client->adapter, xfer_msg, 2);
> +	if (err < 0)
> +		dev_err(&info->i2c_client->dev,
> +		      "transfer error %d\n", err);
> +	return (err != 2);
> +}

Does returning a boolean condition here cause issues for the callers?
The expression (err != 2) evaluates to 0 on success and 1 on failure.

> [ ... ]
> +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 calculation underflow if the source advertises an amperage less than
the reserve amount? If charging_limit wraps around, the min() macro below
will select the configured maximum limit, potentially commanding the regulator
to draw more current than a low-power source can support.

> +
> +	/* Set limits */
> +	switch (voltage) {
> [ ... ]
> +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);

Is it safe to unconditionally call disable_irq() here? The driver allows
polling mode where client->irq might be <= 0, so this could manipulate invalid
IRQ descriptors or core system interrupts.

Also, since this work item is queued, and the interrupt handler also calls
disable_irq_nosync() before queueing info->work, could the workqueue core
collapse the requests? The IRQ disable depth would increment multiple times,
but enable_irq() would only execute once at the end of the handler, leaving
the interrupt permanently disabled.

> +
> +	bm92t_set_vbus_enable(info, false);
> [ ... ]
> +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];
> +	unsigned int prev_wattage = 0;
> +	unsigned int amperage, voltage, wattage, type;
> +
> +	dev = &info->i2c_client->dev;
> +
> +	memset(&info->cable, 0, sizeof(struct bm92t_device));
> +
> +	err = bm92t_read_reg(info, READ_PDOS_SRC_REG, pdos, sizeof(pdos));
> +	pdos_no = pdos[0] / sizeof(struct pd_object);
> +
> +	/* Check if errors or no pdo received */
> +	if (err || !pdos_no)
> +		return 0;
> +
> +	dev_info(dev, "Supported PDOs:\n");
> +	memcpy(pdo, pdos + 1, pdos[0]);

Could a malfunctioning device provide an untrusted pdos[0] length greater than
28? The destination pdo array is sized for 7 elements (28 bytes), so this could
cause a stack buffer overflow if a length up to 255 is provided.

> +	for (i = 0; i < pdos_no; ++i) {
> [ ... ]
> +static void bm92t_event_handler(struct work_struct *work)
> +{
> [ ... ]
> +	err = bm92t_read_reg(info, ALERT_STATUS_REG,
> +			     (unsigned char *) &alert_data,
> +			     sizeof(alert_data));
> +	if (err < 0)
> +		goto ret;

Since bm92t_read_reg() returns 0 or 1, checking if (err < 0) will always be
false. Does this cause the driver to silently ignore read failures and continue
executing with uninitialized stack variables like alert_data?

> [ ... ]
> +	case VDM_ACCEPT_DISC_MODE_REPLY:
> +		if (bm92t_is_success(alert_data)) {
> +			/* Check incoming VDM */
> +			err = bm92t_read_reg(info, INCOMING_VDM_REG, vdm,
> +					     sizeof(vdm));
> +
> +			/* Check if DisplayPort Alt mode is supported */
> +			if (vdm[0] > 4 && /* Has VDO objects */
> +			    vdm[1] == (VDM_ACK | VDM_CMD_DISC_MODE) &&
> +			    vdm[2] == VDM_STRUCTURED &&
> +			    vdm[3] == 0x01 && vdm[4] == 0xFF && /* SVID DP */
> +			    vdm[5] & VDO_DP_UFP_D &&
> +			    vdm[5] & VDO_DP_SUPPORT) {
> +				dev_info(dev, "DisplayPort Alt Mode supported");
> +				for (i = 0; i < ((vdm[0] - 4) / 4); i++) {
> +					dev_info(dev, "DPCap%d %08X\n",
> +						 i, vdm[5 + i * 4]      |
> +						 (vdm[6 + i * 4] << 8)  |
> +						 (vdm[7 + i * 4] << 16) |
> +						 (vdm[8 + i * 4] << 24));

Can this loop go out of bounds? vdm is declared as unsigned char vdm[29].
If vdm[0] is large, the loop could calculate indices past the end of the array,
leading to an out-of-bounds stack read that gets logged by dev_info().

> +				}
> [ ... ]
> +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 function leak the threaded IRQ, the dedicated workqueue, and the
scheduled delayed works? The probe function manually allocates these resources,
and if they are not explicitly torn down here before the devm-allocated info
structure is freed, it could lead to a use-after-free panic during module
unload.

> [ ... ]
> +static ssize_t bm92t_cmd_write(struct file *file,
> +		     const char __user *userbuf, size_t count, loff_t *ppos)
> +{
> +	struct bm92t_info *info = (struct bm92t_info *) (file->private_data);
> +	unsigned int val;
> +	unsigned short cmd;
> +	char buf[8];
> +	int ret;
> +
> +	count = min_t(size_t, count, (sizeof(buf)-1));
> +	if (copy_from_user(buf, userbuf, count))
> +		return -EFAULT;
> +
> +	buf[count] = 0;
> +
> +	ret = kstrtouint(buf, 0, &val);
> +
> +	if (ret == 1) {

Is it possible for kstrtouint() to return 1? It returns 0 on success and a
negative error code on failure. Checking for 1 means valid inputs are rejected,
rendering the debugfs interface non-functional. (This also applies to
bm92t_usbhub_dp_sleep_write).

> +		cmd = val;
> +		bm92t_send_cmd(info, &cmd);
> +	} else {
> +		dev_err(&info->i2c_client->dev, "Cmd syntax is: cmd\n");
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> [ ... ]
> +static int bm92t_probe(struct i2c_client *client)
> +{
> [ ... ]
> +	ep = fwnode_graph_get_endpoint_by_id(
> +		dev_fwnode(&client->dev), 0, 0, 0);
> +	if (!ep) {
> +		dev_err(&client->dev, "Endpoint not found\n");
> +		return -ENODEV;
> +	}
> +
> +	remote = fwnode_graph_get_remote_endpoint(ep);
> +	if (!remote) {
> +		dev_err(&client->dev, "Remote not found\n");
> +		return -ENODEV;
> +	}
> +
> +	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;

If info->role_sw is NULL, PTR_ERR returns 0. The probe function would then exit
early and return 0 without calling i2c_set_clientdata(). Would this lead to a
NULL pointer dereference later when bm92t_remove() or suspend callbacks try to
fetch the clientdata?

Also, do the fwnode references for ep and remote need to be released using
fwnode_handle_put() to prevent a memory leak?

> +	}
> +
> +	i2c_set_clientdata(client, info);
> [ ... ]
> +#ifdef CONFIG_SWITCH
> +	info->pd_switch.name = pd_switch_name;

Is pd_switch_name declared anywhere? This looks like it would cause a build
failure due to an undeclared identifier when CONFIG_SWITCH is enabled.

> +	err = switch_dev_register(&info->pd_switch);
> +	if (err)
> +		dev_err(&client->dev, "failed to register pd switch, err=%d\n", err);
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-bm92t-v1-0-dff3c9d22426@gmail.com?part=1