Re: [PATCH v3 3/3] hwmon: pmbus: add MPQ8646 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Premature debugfs registration exposes userspace interfaces before device probe completes, allowing NULL pointer dereferences and Use-After-Free on probe error.
- [High] Incorrect teardown order in `mpq8646_remove()` allows a concurrent debugfs write to re-queue a delayed work on freed memory.
- [High] Stack-allocated buffers are passed to `i2c_transfer()`, which violates the DMA API rules and can cause kernel panics.
- [Medium] Concurrent I2C probes will cause debugfs registration to fail for some device instances.
- [Low] The debugfs diagnostic fields last_probe_rc and last_probe_data are updated outside of mps_lock, leading to torn reads.
--

--- Patch [3]: [PATCH v3 3/3] hwmon: pmbus: add MPQ8646 driver ---
commit 543735e252009063d43f85076b5597345b0c78ea
Author: Vincent Jardin <vjardin@free.fr>

hwmon: pmbus: add MPQ8646 driver

Add a new single-chip driver for the MPS MPQ8646 that is a PMBus device.
Beyond basic PMBus telemetry, the driver adds several features like
alarm acknowledge and in-driver alarm-poll fallback.

> diff --git a/drivers/hwmon/pmbus/mpq8646.c b/drivers/hwmon/pmbus/mpq8646.c
> index 0000000000000..830f5724b6ee5
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/mpq8646.c
[ ... ]
> +static int mpq8646_raw_xfer_rword(struct i2c_client *client, u8 reg)
> +{
> +	u8 cmd = reg;
> +	__le16 data = 0;
> +	struct i2c_msg msg[] = {
> +		{
> +			.addr = client->addr,
> +			.flags = 0,
> +			.len = sizeof(cmd),
> +			.buf = &cmd,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = I2C_M_RD,
> +			.len = sizeof(data),
> +			.buf = (u8 *)&data,
> +		},
> +	};
> +	int rc;
> +
> +	rc = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));

[Severity: High]
Can this cause a kernel panic if the I2C host controller uses DMA? The cmd
and data buffers are allocated on the stack and passed directly to
i2c_transfer(). On platforms with CONFIG_VMAP_STACK, this can lead to memory
corruption or a crash since stack memory is virtually mapped. Could these
buffers be allocated dynamically or embedded in a DMA-safe structure instead?

[ ... ]
> +static int mpq8646_dbg_clear_protection_last(void *data, u64 val)
> +{
> +	struct mpq8646_priv *priv = data;
> +	int rc;
> +
> +	if (!val)
> +		return 0;
> +
> +	mutex_lock(&priv->mps_lock);
> +	rc = i2c_smbus_write_byte(priv->client, MPS_CLEAR_LAST_FAULT);
> +	mutex_unlock(&priv->mps_lock);
> +
> +	priv->last_probe_rc = rc;
> +	priv->last_probe_data = 0;

[Severity: Low]
Are these fields intentionally updated outside of mps_lock? Since a concurrent
reader in mpq8646_dbg_last_probe_show() takes the lock, it might observe a
torn state between last_probe_rc and last_probe_data.

[ ... ]
> +static int mpq8646_dbg_clear_protection_last_force(void *data, u64 val)
> +{
> +	struct mpq8646_priv *priv = data;
> +	int rc, last_rc;
> +	int wp_orig, cfg_orig;
> +
> +	if (!val)
> +		return 0;
> +
> +	pmbus_lock(priv->client);
> +	mutex_lock(&priv->mps_lock);

[Severity: High]
Could this cause a NULL pointer dereference if it's called before probe
finishes? The debugfs files are registered before pmbus_do_probe() completes,
so pmbus_lock() might try to access an uninitialized clientdata pointer.

[ ... ]
> +static int mpq8646_dbg_store_all(void *data, u64 val)
> +{
> +	struct mpq8646_priv *priv = data;
> +	int rc;
> +
> +	if (!val)
> +		return 0;
> +
> +	mutex_lock(&priv->mps_lock);
> +	rc = i2c_smbus_write_byte(priv->client, PMBUS_STORE_USER_ALL);
> +	mutex_unlock(&priv->mps_lock);
> +
> +	priv->last_probe_rc = rc;
> +	priv->last_probe_data = 0;

[Severity: Low]
Similarly to mpq8646_dbg_clear_protection_last(), these fields are updated
outside the locked region. Might this lead to inconsistent diagnostic data
being read?

[ ... ]
> +static void mpq8646_debugfs_register(struct mpq8646_priv *priv)
> +{
> +	struct dentry *root, *parent;
> +	char name[MPQ8646_DEBUGFS_DIRNAME_MAX];
> +	bool parent_ref = false;
> +	size_t i;
> +
> +	parent = debugfs_lookup(MPQ8646_DEBUGFS_ROOT_NAME, NULL);
> +	if (!parent)
> +		parent = debugfs_create_dir(MPQ8646_DEBUGFS_ROOT_NAME, NULL);
> +	else
> +		parent_ref = true;
> +	if (IS_ERR_OR_NULL(parent))
> +		return;

[Severity: Medium]
Will concurrent probes of multiple devices cause debugfs registration to fail?
Since the creation of the shared root directory is not serialized, a second
thread might see NULL from debugfs_lookup(), attempt debugfs_create_dir(),
and receive -EEXIST. That would cause IS_ERR_OR_NULL(parent) to evaluate to
true, aborting debugfs initialization for the second device.

[ ... ]
> +static int mpq8646_probe(struct i2c_client *client)
> +{
[ ... ]
> +	mpq8646_debugfs_register(priv);
> +
> +	ret = pmbus_do_probe(client, info);
> +	if (ret) {
> +		mpq8646_debugfs_unregister(priv);
> +		return ret;
> +	}

[Severity: High]
Does registering debugfs before pmbus_do_probe() expose the driver interfaces
too early? Besides the pmbus_lock() issue mentioned earlier, a userspace
write to alarm_poll_interval_ms could queue priv->alarm_poll_work. If
pmbus_do_probe() then fails, the work isn't cancelled before priv is freed,
which could lead to a use-after-free when the timer fires.

[ ... ]
> +static void mpq8646_remove(struct i2c_client *client)
> +{
> +	struct mpq8646_priv *priv = mpq8646_priv_from_client(client);
> +
> +	cancel_delayed_work_sync(&priv->alarm_poll_work);
> +	mpq8646_debugfs_unregister(priv);
> +}

[Severity: High]
Is it safe to cancel the delayed work before unregistering debugfs? A
concurrent userspace write to the alarm_poll_interval_ms debugfs file
could re-queue the work right after cancel_delayed_work_sync() returns.
Then mpq8646_remove() completes, freeing priv, which leads to a
use-after-free when the re-armed timer executes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-mpq8646_v0-v3-0-1af1f3d38848@free.fr?part=3