Re: [PATCH v4 8/8] docs: misc: amd-sbi: Document SBTSI userspace interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The I3C transmit buffer (`tx`) is used for DMA but is not cacheline aligned, leading to DMA false sharing with adjacent fields in `struct sbtsi_data`.
- [High] A race condition between `misc_open()` and devres driver unbind causes a use-after-free of `struct sbtsi_data`.
--

--- Patch [8]: [PATCH v4 8/8] docs: misc: amd-sbi: Document SBTSI userspace interface ---
commit 0d1e3dd1fee7d42c8bd4c97352ebe1b9dc763f55
Author: Prathima <Prathima.Lk@amd.com>
Subject: docs: misc: amd-sbi: Document SBTSI userspace interface

This commit documents the SBTSI userspace interface and implements the SB-TSI
driver for AMD SoCs. It adds an ioctl interface to perform I2C/I3C register
transfers and creates a miscdevice node for each sensor. It also registers an
auxiliary device to bind with the hwmon driver.

Link: https://github.com/amd/esmi_oob_library [1]
Link: https://www.amd.com/en/developer/e-sms.html [2]

> diff --git a/drivers/misc/amd-sbi/tsi-core.c b/drivers/misc/amd-sbi/tsi-core.c
> new file mode 100644
> index 0000000000000..5c178702c67ab
> --- /dev/null
> +++ b/drivers/misc/amd-sbi/tsi-core.c
> @@ -0,0 +1,207 @@

[ ... ]

> +/* I3C read transfer function */
> +static int sbtsi_i3c_read(struct sbtsi_data *data, u8 reg, u8 *val)
> +{
> +	struct sbtsi_i3c_priv *priv = to_sbtsi_i3c_priv(data);
> +	struct i3c_xfer xfers[2] = { };
> +	int ret;
> +
> +	priv->tx[0] = reg;
> +
> +	/* Write the register address (DMA_TO_DEVICE). */
> +	xfers[0].rnw = false;
> +	xfers[0].len = 1;
> +	xfers[0].data.out = priv->tx;
> +
> +	/* Read the data byte into a separate buffer (DMA_FROM_DEVICE). */
> +	xfers[1].rnw = true;
> +	xfers[1].len = 1;
> +	xfers[1].data.in = &priv->rx;
> +
> +	ret = i3c_device_do_xfers(data->i3cdev, xfers, 2, I3C_SDR);

[Severity: High]
Does this code lack cacheline alignment for the tx buffer?

Looking at the structure definition in drivers/misc/amd-sbi/tsi-core.h:

struct sbtsi_i3c_priv {
	struct sbtsi_data data;
	u8 tx[2];
	u8 rx __aligned(ARCH_DMA_MINALIGN);
};

Since tx lacks __aligned(ARCH_DMA_MINALIGN), it shares a cacheline with the
preceding fields in struct sbtsi_data. Because tx is used here as a
DMA_TO_DEVICE buffer, couldn't DMA cache maintenance operations on tx
corrupt adjacent fields like kref or lock on non-cache-coherent
architectures?

> +	if (ret)
> +		return ret;
> +
> +	*val = priv->rx;
> +	return ret;
> +}

[ ... ]

> +static int sbtsi_open(struct inode *inode, struct file *fp)
> +{
> +	struct sbtsi_data *data;
> +
> +	data = container_of(fp->private_data, struct sbtsi_data, sbtsi_misc_dev);
> +	scoped_guard(sbtsi, data) {

[Severity: High]
Is it possible for a concurrent device unbind to free struct sbtsi_data
before sbtsi_open() acquires the mutex?

If misc_open() sets fp->private_data and drops the core misc_mtx, and then
devres executes sbtsi_driver_unref() (which drops the final reference and
frees data), would sbtsi_open() accessing data->lock result in a
use-after-free?

drivers/misc/amd-sbi/tsi.c:sbtsi_driver_unref() {
	struct sbtsi_data *data = arg;

	kref_put(&data->kref, sbtsi_data_release);
}

> +		if (data->detached)
> +			return -ENODEV;
> +	}
> +
> +	kref_get(&data->kref);
> +
> +	return 0;
> +}

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