Re: [PATCH v3 6/8] misc: amd-sbi: Add SBTSI ioctl register transfer interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] TOCTOU race in IOCTL during device unbind leads to use-after-free of I2C/I3C device.
- [High] Using `devm_add_action()` instead of `devm_add_action_or_reset()` fails to unregister the misc device on error, causing a use-after-free.
--

commit 68509c62bd1860c432a2e05da7ef0faad793172b
Author: Prathima <Prathima.Lk@amd.com>

misc: amd-sbi: Add SBTSI ioctl register transfer interface

Implement ioctl interface for SB-TSI driver to enable userspace access
to TSI register read/write operations through the AMD Advanced Platform
Management Link (APML) protocol. Serialize access with a mutex shared
between the hwmon and ioctl paths.

> diff --git a/drivers/misc/amd-sbi/tsi-core.c b/drivers/misc/amd-sbi/tsi-core.c
> index 9278d06d8e5f6..688b9221868fe 100644
> --- a/drivers/misc/amd-sbi/tsi-core.c
> +++ b/drivers/misc/amd-sbi/tsi-core.c
[ ... ]
> @@ -80,7 +95,108 @@ int sbtsi_xfer(struct sbtsi_data *data, u8 reg, u8 *val, bool is_read)
[ ... ]
> +static int apml_tsi_reg_xfer(struct sbtsi_data *data,
> +			     struct apml_tsi_xfer_msg __user *arg)
> +{
> +	struct apml_tsi_xfer_msg msg = { 0 };
> +	int ret;
> +
> +	if (data->detached)
> +		return -ENODEV;
> +
> +	if (copy_from_user(&msg, arg, sizeof(struct apml_tsi_xfer_msg)))
> +		return -EFAULT;

[Severity: High]
Is there a time-of-check to time-of-use race here if the device is unbound
during this ioctl?

Since copy_from_user() can sleep, if the device is removed or unbound while
the thread is sleeping, devres cleanup will execute sbtsi_misc_unregister().

This sets data->detached = true locklessly and the subsystem frees
the data->client or data->i3cdev pointer.

Since sbtsi_data is kept alive by the kref, the ioctl thread can wake up, call
sbtsi_xfer_ioctl(), acquire data->lock, and call sbtsi_xfer().

Could this cause a use-after-free since sbtsi_xfer_ioctl() doesn't
re-check data->detached under the lock before accessing the freed client?

> +
> +	if (msg.pad)
> +		return -EINVAL;
> +
> +	ret = sbtsi_xfer_ioctl(data, msg.reg_addr, &msg.data_in_out, msg.rflag);
> +
> +	if (msg.rflag && !ret) {
> +		if (copy_to_user(arg, &msg, sizeof(struct apml_tsi_xfer_msg)))
> +			return -EFAULT;
> +	}
> +	return ret;
> +}
[ ... ]
> diff --git a/drivers/misc/amd-sbi/tsi.c b/drivers/misc/amd-sbi/tsi.c
> index 8fb17ccab73de..6649cd8cdf850 100644
> --- a/drivers/misc/amd-sbi/tsi.c
> +++ b/drivers/misc/amd-sbi/tsi.c
[ ... ]
> @@ -89,6 +104,13 @@ static int sbtsi_probe_common(struct device *dev, struct sbtsi_data *data)
[ ... ]
>  	dev_set_drvdata(dev, data);
> -	return sbtsi_create_hwmon_adev(dev, data->dev_addr);
> +	err = sbtsi_create_hwmon_adev(dev, data->dev_addr);
> +	if (err < 0)
> +		return err;
> +
> +	err = create_misc_tsi_device(data, dev);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action(dev, sbtsi_misc_unregister, data);

[Severity: High]
If devm_add_action() fails here, will it leave the misc device registered?

Unlike devm_add_action_or_reset(), devm_add_action() returns an error on
failure without executing the provided callback.

If this fails (e.g. due to memory allocation failure), the probe aborts and
devres cleanup frees struct sbtsi_data.

Because sbtsi_misc_unregister() is never called, the misc device would remain
registered. Any subsequent attempt to open or interact with the leaked device
node would access the freed sbtsi_data.

Should this use devm_add_action_or_reset() instead?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622135821.2190260-1-Akshay.Gupta@amd.com?part=6