Re: [PATCH v6 3/5] mfd: aaeon: Add SRG-IMX8P MCU driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Heap buffer overflow in `aaeon_mcu_regmap_write()` due to unconstrained write sizes during bulk writes.
- [High] Unsafe use of `I2C_M_DMA_SAFE` with caller-supplied buffers causes DMA mapping of stack memory in `aaeon_mcu_regmap_read()`.
- [High] DMA cacheline corruption due to `devm_kzalloc()` usage for I2C DMA buffers.
- [Medium] I2C bus lock is dropped between write and read phases of a single MCU command, breaking atomicity.
--

commit aa13d82b96cec6318efcd3d2b062f9b98afc3a61
Author: Thomas Perrot (Schneider Electric) <[email protected]>

mfd: aaeon: Add SRG-IMX8P MCU driver
    
Add Multi-Function Device (MFD) driver for the Aaeon SRG-IMX8P
embedded controller. This driver provides the core I2C communication
interface and registers child devices (GPIO and watchdog controllers).

> diff --git a/drivers/mfd/aaeon-mcu.c b/drivers/mfd/aaeon-mcu.c
> new file mode 100644
> index 0000000000000..306aaac1bd60c
> --- /dev/null
> +++ b/drivers/mfd/aaeon-mcu.c

[ ... ]

> +static int aaeon_mcu_regmap_write(void *context, const void *data, size_t count)
> +{
> +	struct aaeon_mcu *mcu = context;
> +	struct i2c_client *client = mcu->client;
> +	struct i2c_msg write_msg;
> +	/* The MCU always sends a response byte after each command; discard it. */
> +	struct i2c_msg response_msg;
> +	int ret;
> +
> +	memcpy(mcu->cmd, data, count);

[Severity: High]
Does this code overflow the mcu->cmd buffer if a bulk write is triggered?

The mcu->cmd buffer is strictly allocated with a length of 3 bytes.
If a bulk write occurs (e.g., via regcache_sync() or regmap debugfs), the
count could exceed 3 bytes.

Could the aaeon_mcu_regmap_config missing .use_single_write = true allow
these bulk writes to occur?

> +
> +	write_msg.addr  = client->addr;
> +	write_msg.flags = I2C_M_DMA_SAFE;
> +	write_msg.buf   = mcu->cmd;
> +	write_msg.len   = count;
> +
> +	response_msg.addr  = client->addr;
> +	response_msg.flags = I2C_M_RD | I2C_M_DMA_SAFE;
> +	response_msg.buf   = mcu->response;
> +	response_msg.len   = 1;
> +
> +	ret = i2c_transfer(client->adapter, &write_msg, 1);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +
> +	ret = i2c_transfer(client->adapter, &response_msg, 1);

[Severity: Medium]
Could this sequence of two separate i2c_transfer() calls cause the MCU
command phases to be interleaved with other bus traffic?

The I2C bus lock is dropped between the write phase and the response read
phase.

If another I2C device shares the bus, can its transfers interleave here and
break the atomicity of the MCU command?

> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int aaeon_mcu_regmap_read(void *context, const void *reg_buf,
> +				 size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct aaeon_mcu *mcu = context;
> +	struct i2c_client *client = mcu->client;
> +	struct i2c_msg write_msg;
> +	struct i2c_msg read_msg;
> +	int ret;
> +
> +	/*
> +	 * reg_buf holds the 2-byte big-endian register address [opcode, arg].
> +	 * Append a trailing 0x00 to form the full 3-byte MCU command.
> +	 */
> +	mcu->cmd[0] = ((u8 *)reg_buf)[0];
> +	mcu->cmd[1] = ((u8 *)reg_buf)[1];
> +	mcu->cmd[2] = 0x00;
> +
> +	write_msg.addr  = client->addr;
> +	write_msg.flags = I2C_M_DMA_SAFE;
> +	write_msg.buf   = mcu->cmd;
> +	write_msg.len   = AAEON_MCU_CMD_LEN;
> +
> +	read_msg.addr  = client->addr;
> +	read_msg.flags = I2C_M_RD | I2C_M_DMA_SAFE;
> +	read_msg.buf   = val_buf;

[Severity: High]
Is it safe to flag a caller-supplied buffer with I2C_M_DMA_SAFE here?

If a child driver or the regmap core passes a stack-allocated buffer for
val_buf (e.g., during regmap_bulk_read()), tagging it with I2C_M_DMA_SAFE
bypasses the I2C core's bounce buffers.

Could this cause a kernel panic due to a DMA API violation on architectures
with virtually mapped stacks?

> +	read_msg.len   = val_size;
> +
> +	ret = i2c_transfer(client->adapter, &write_msg, 1);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +
> +	ret = i2c_transfer(client->adapter, &read_msg, 1);

[Severity: Medium]
Similarly to the write path, could dropping the I2C adapter lock between
the write_msg and read_msg transfers allow other traffic to interrupt the
sequence?

> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +
> +	return 0;
> +}

[ ... ]

> +static int aaeon_mcu_probe(struct i2c_client *client)
> +{
> +	struct aaeon_mcu *ddata;
> +	struct regmap *regmap;
> +
> +	ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ddata->client = client;
> +
> +	ddata->cmd = devm_kzalloc(&client->dev, AAEON_MCU_CMD_LEN * sizeof(*ddata->cmd),
> +				   GFP_KERNEL);

[Severity: High]
Does allocating the DMA buffers with devm_kzalloc() cause cacheline
corruption?

Since devres allocations prepend a hidden struct devres header which is not
padded to ARCH_DMA_MINALIGN, the returned buffer can share a cacheline
with this header.

During I2C DMA transactions on non-cache-coherent architectures, could the
cacheline invalidation destroy concurrent CPU writes to the devres header?

> +	if (!ddata->cmd)
> +		return -ENOMEM;
> +
> +	ddata->response = devm_kzalloc(&client->dev, sizeof(*ddata->response), GFP_KERNEL);
> +	if (!ddata->response)
> +		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3