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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Use-After-Free of `sbtsi_data` due to misaligned devres and VFS lifecycles
- [High] Unsafe DMA Buffer Allocation from stack variables
- [High] Subsystem Guideline Violation: Redundant custom lock instead of hwmon_lock()
- [High] sysfs name collision for miscdevice on multi-bus systems
- [Medium] Hardware state desynchronization via unrestricted IOCTL writes
- [Medium] UAPI Flaw: Unvalidated padding byte in IOCTL message
--

commit a076ef47ca989260af804712d32ac63c2aaf11cb
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.
Add an ioctl command (SBTSI_IOCTL_REG_XFER_CMD) that accepts a register
address, data byte, and direction flag.

> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index d7ae986d824c4..00e982f4c716c 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c
> @@ -64,12 +64,15 @@ static inline void sbtsi_mc_to_reg(s32 temp, u8 *integer, u8 *decimal)
>  /*
>   * Read integer and decimal parts of an SB-TSI temperature register pair
>   * The read order is determined by the ReadOrder bit to ensure atomic latching.
> + * The mutex protects against concurrent access to the shared I2C/I3C bus by
> + * the hwmon sysfs and a userspace ioctl
>   */
>  static int sbtsi_temp_read(struct sbtsi_data *data, u8 reg1, u8 reg2,
>  			   u8 *val1, u8 *val2)
>  {
>  	int ret;
>  
> +	guard(mutex)(&data->lock);

Does this custom lock bypass the standardized hwmon core synchronization
mechanism? The hwmon subsystem guidelines appear to mandate using
hwmon_lock() and hwmon_unlock() to serialize sysfs operations against
attributes registered by other means.

[ ... ]

> diff --git a/drivers/misc/amd-sbi/tsi-core.c b/drivers/misc/amd-sbi/tsi-core.c
> index 19388737b225e..c5bd60409d5bb 100644
> --- a/drivers/misc/amd-sbi/tsi-core.c
> +++ b/drivers/misc/amd-sbi/tsi-core.c
[ ... ]
> +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 (copy_from_user(&msg, arg, sizeof(struct apml_tsi_xfer_msg)))
> +		return -EFAULT;
> +
> +	ret = sbtsi_xfer_ioctl(data, msg.reg_addr, &msg.data_in_out, msg.rflag);

Could passing &msg.data_in_out to sbtsi_xfer_ioctl() eventually result in
a DMA transfer from stack memory?

If the transfer goes over I3C, sbtsi_i3c_read() uses this pointer directly:

static int sbtsi_i3c_read(struct sbtsi_data *data, u8 reg, u8 *val)
{
	struct i3c_xfer xfers[2] = { };

	xfers[0].rnw = false;
	xfers[0].len = 1;
	xfers[0].data.out = &reg;

	xfers[1].rnw = true;
	xfers[1].len = 1;
	xfers[1].data.in = val;

	return i3c_device_do_xfers(data->i3cdev, xfers, 2, I3C_SDR);
}

Since I3C host controllers commonly use DMA, and stack variables are not
cacheline aligned (and reside in virtually mapped memory with
CONFIG_VMAP_STACK enabled), this might violate the DMA API and lead to adjacent
stack corruption.

Furthermore, does this unrestricted IOCTL write capability allow userspace
to overwrite the SBTSI_REG_CONFIG register?

During probe, the driver caches hardware state (ext_range_mode and
read_order) from this register. If userspace modifies it without the
driver intercepting the write to update its cache, the hwmon interface
could report incorrect temperatures.

[ ... ]

> +static long sbtsi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	struct sbtsi_data *data;
> +
> +	data = container_of(fp->private_data, struct sbtsi_data, sbtsi_misc_dev);

Can this lead to a use-after-free if the device is unbound?

The sbtsi_data structure is allocated with devm_kzalloc(), tying its
lifetime to the device's driver binding. When the device unbinds, devres
calls misc_deregister() and frees sbtsi_data.

Since misc_deregister() does not close or revoke existing file descriptors,
a process could retain its file descriptor and call ioctl() after the unbind,
extracting the freed sbtsi_data pointer and attempting to lock data->lock.

[ ... ]

> +int create_misc_tsi_device(struct sbtsi_data *data, struct device *dev)
> +{
> +	int ret;
> +
> +	data->sbtsi_misc_dev.name = devm_kasprintf(dev, GFP_KERNEL,
> +						   "sbtsi-%x", data->dev_addr);

Will this naming scheme cause a sysfs name collision on systems with
multiple I2C/I3C buses?

If identically-addressed sensors exist on different buses, it looks like
misc_register() will attempt to create a duplicate sysfs entry
(e.g., /sys/class/misc/sbtsi-4c), fail with -EEXIST, and abort the probe.

[ ... ]

> diff --git a/include/uapi/misc/amd-apml.h b/include/uapi/misc/amd-apml.h
> index 745b3338fc06f..8a85f79b0938d 100644
> --- a/include/uapi/misc/amd-apml.h
> +++ b/include/uapi/misc/amd-apml.h
> @@ -73,6 +73,13 @@ struct apml_reg_xfer_msg {
>  	__u8 rflag;
>  };
>  
> +struct apml_tsi_xfer_msg {
> +	__u8 reg_addr;		/* TSI register address offset */
> +	__u8 data_in_out;	/* Register data for read/write */
> +	__u8 rflag;		/* Register read or write */
> +	__u8 pad;		/* Explicit padding */
> +};

Should msg.pad be validated in apml_tsi_reg_xfer() to ensure it is zero?

Without this check, userspace can pass uninitialized garbage in the padding
field, which might prevent safely repurposing this field for new flags or
extensions in the future.

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