Re: [PATCH v3 7/8] hwmon: Add mutex protecting for sbtsi read/write through hwmon

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Unsafe DMA buffer allocation risking memory corruption due to cacheline sharing in I3C driver.
- [High] Use-After-Free and resource leak on the probe error path due to missing devres rollback.
- [High] TOCTOU race condition in IOCTL leading to Use-After-Free of underlying I2C/I3C device.

Pre-existing issues:
- [High] Signed integer overflow when setting extended temperature limit results in minimum limit instead of maximum.
--

commit 73c231fb30add9ba7fc881d929121fb0a885ff0d
Author: Prathima <Prathima.Lk@amd.com>
Subject: hwmon: Add mutex protecting for sbtsi read/write through hwmon

This commit adds a mutex using guard(sbtsi) around the SBTSI read and write
paths in the hwmon driver to serialize bus transactions.

> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index d7ae986d824c4..11c8108d69b2a 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c

[ ... ]

While reviewing this patch, I noticed a few other issues in the surrounding code
and the core driver.

drivers/misc/amd-sbi/tsi-core.h:struct sbtsi_i3c_priv {
	struct sbtsi_data data;
	u8 buf[2];
};

[Severity: High]
Does this buf array risk memory corruption during I3C transfers?

Because buf immediately follows struct sbtsi_data without explicit
cacheline alignment (____cacheline_aligned), it shares a CPU cacheline with
the trailing fields of struct sbtsi_data.

When the I3C master controller maps this buffer for DMA, it flushes or
invalidates the cacheline. Will concurrent CPU access to the adjacent
sbtsi_data fields (like data->detached or data->lock) during the
transfer cause cacheline bouncing, leading to either silent loss of state
updates or corruption of the DMA buffer?


drivers/hwmon/sbtsi_temp.c:sbtsi_write() {
    ...
	if (data->ext_range_mode)
		val += SBTSI_TEMP_EXT_RANGE_ADJ;
	val = clamp_val(val, SBTSI_TEMP_MIN, SBTSI_TEMP_MAX);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but can this code wrap to a negative
value when setting an extended temperature limit?

If a user provides an extremely large value (e.g. LONG_MAX) to maximize the
limit via sysfs, the val += SBTSI_TEMP_EXT_RANGE_ADJ operation could cause a
signed integer overflow. In the kernel, this wraps to a large negative number.

When subsequently passed to clamp_val(), it is clamped to SBTSI_TEMP_MIN
(0). Does this mean attempting to disable or maximize a thermal limit sets it
to 0 instead, potentially triggering unintended hardware thermal throttling
or system shutdown?


drivers/misc/amd-sbi/tsi.c:sbtsi_probe_common() {
    ...
	err = create_misc_tsi_device(data, dev);
	if (err)
		return err;

	return devm_add_action(dev, sbtsi_misc_unregister, data);
}

[Severity: High]
Could this error path cause a use-after-free and leave the misc device
registered?

devm_add_action() is used to queue sbtsi_misc_unregister. If
devm_add_action() fails to allocate devres memory, it returns an error but
does not execute the action. This causes the probe to fail and devres unwinding
to start. Devres then executes previously added cleanup actions, such as
sbtsi_driver_unref, which drops the kref and frees the data struct.

Because sbtsi_misc_unregister was not executed, the misc device remains
registered globally with its private_data pointing to the now-freed data.
Should this use devm_add_action_or_reset() to ensure the cleanup executes
if action registration fails?


drivers/misc/amd-sbi/tsi-core.c:apml_tsi_reg_xfer() {
    ...
	if (data->detached)
		return -ENODEV;

	if (copy_from_user(&msg, arg, sizeof(struct apml_tsi_xfer_msg)))
		return -EFAULT;
    ...
}

[Severity: High]
Is there a race condition here that could lead to a use-after-free of the
underlying I2C/I3C device?

The driver checks data->detached before acquiring data->lock and before
sleeping in copy_from_user(). If the device is hot-unplugged concurrently,
the devres unbind path executes sbtsi_misc_unregister():

drivers/misc/amd-sbi/tsi.c:sbtsi_misc_unregister() {
    ...
	misc_deregister(&data->sbtsi_misc_dev);
	data->detached = true;
    ...
}

This sets data->detached = true without acquiring data->lock and completes
device unbind. Completing unbind drops the device reference and frees the
i2c_client or i3c_device structure.

When the IOCTL thread wakes up, it calls sbtsi_xfer_ioctl(), which takes the
lock but does not recheck data->detached. Will it then access the
already-freed data->client?

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