Re: [PATCH v7 2/7] mfd: Add driver for ASUS Transformer embedded controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Unsynchronized reallocation of `ddata->ec.model` causes Use-After-Free during interrupts.
- [High] Stale and uninitialized data is accessed on short I2C reads.
- [Medium] `asus_dockram_access_ctl()` silently ignores I2C write errors.
- [Medium] Memory leak of `ddata->ec.model` upon driver unbind or probe failure.
- [Medium] The original IRQ event payload is corrupted before being passed to child notifiers.
- [Medium] Subsystem Guideline Violation: Mixing `goto` error handling with `__free()` cleanup.
- [Medium] Missing error check for `kstrndup()` allocation failure in `asus_ec_log_info()`.
- [Low] Incorrect length check causes string truncation in `asus_ec_log_info()`.
--

commit d95a94a77b9b37ec021eabccb7ff81e4dd52730e
Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>

mfd: Add driver for ASUS Transformer embedded controller
    
Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
and TF600T, P1801-T and TF701T pad. This is a glue driver handling
detection and common operations for EC's functions.

> diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> new file mode 100644
> index 0000000000000..18bb3e5f384d9
> --- /dev/null
> +++ b/drivers/mfd/asus-transformer-ec.c

[ ... ]

> +	if (mask || xor) {
> +		put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> +		ret = i2c_smbus_write_i2c_block_data(client,
> +						     ASUSEC_DOCKRAM_CONTROL,
> +						     DOCKRAM_ENTRY_SIZE, buf);
> +	}
> +
> +	return 0;
> +}

Does this accidentally hide hardware write failures? Since ret is ignored,
callers might falsely assume the I2C block write succeeded.

[ ... ]

> +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> +{
> +	int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> +
> +	/*
> +	 * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> +	 * of the status byte or till we reach end of the 256 byte buffer.
> +	 */
> +	while (retry--) {
> +		ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> +						    sizeof(ddata->ec_buf),
> +						    ddata->ec_buf);
> +		if (ret < 0)
> +			continue;
> +
> +		if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> +			continue;

Can a short read leave stale data in ddata->ec_buf? The function
i2c_smbus_read_i2c_block_data() returns the actual number of bytes read. If it
returns a positive value less than ASUSEC_IRQ_STATUS + 1, the buffer will
retain data from previous iterations.

[ ... ]

> +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> +			    const char *name, char **out)
> +{
> +	struct device *dev = &ddata->client->dev;
> +	u8 buf[DOCKRAM_ENTRY_BUFSIZE];
> +	int ret;
> +
> +	memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> +	ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> +					    DOCKRAM_ENTRY_SIZE, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> +		dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> +			DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> +		return -EPROTO;
> +	}

Is there a risk of an out-of-bounds read here? The I2C read fills up to 32
bytes (DOCKRAM_ENTRY_SIZE), meaning buf[0] to buf[31] are valid. If buf[0]
equals 32, the kstrndup() below will attempt to copy 32 bytes starting from
buf + 1, reading up to buf[32].

> +
> +	if (!ddata->logging_disabled)
> +		dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> +
> +	if (out)
> +		*out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> +
> +	return 0;
> +}

If kstrndup() fails and returns NULL, asus_ec_detect() will unknowingly assign
NULL to ddata->ec.model. Should this return -ENOMEM on allocation failure?

[ ... ]

> +static int asus_ec_detect(struct asus_ec_data *ddata)
> +{
> +	char *model __free(kfree) = NULL;
> +	int ret;
> +
> +	ret = asus_ec_reset(ddata);
> +	if (ret)
> +		goto err_exit;

This isn't a bug, but mixing goto error handling with __free() cleanup in the
same function violates the subsystem guidelines. Functions are expected to use
either scope-based cleanup or goto-based cleanup exclusively to prevent
confusing ownership semantics.

[ ... ]

> +	/* Check and inform about EC firmware behavior */
> +	ret = asus_ec_susb_on_status(ddata);
> +	if (ret)
> +		goto err_exit;
> +
> +	kfree(ddata->ec.model);
> +
> +	ddata->ec.model = no_free_ptr(model);

Could this cause a use-after-free or a memory leak?

Since asus_ec_detect() can be called from the threaded interrupt handler at
runtime, ddata->ec.model might be freed and replaced without synchronization
while child drivers or sysfs attributes are concurrently accessing it.

Additionally, ddata->ec.model is never freed on driver unbind or probe
failure, causing a memory leak.

[ ... ]

> +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> +{
> +	struct asus_ec_data *ddata = dev_id;
> +	unsigned long notify_action;
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> +					    sizeof(ddata->ec_buf), ddata->ec_buf);
> +	if (ret <= 0 || !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
> +		return IRQ_NONE;

Similar to asus_ec_clear_buffer(), if ret is a positive value smaller than
the required length, this could process stale data from previous interrupt
events.

> +
> +	notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> +	if (notify_action & ASUSEC_SMI_MASK) {
> +		unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> +
> +		asus_ec_handle_smi(ddata, code);
> +
> +		notify_action |= code << 8;
> +	}
> +
> +	blocking_notifier_call_chain(&ddata->ec.notify_list,
> +				     notify_action, ddata->ec_buf);
> +
> +	return IRQ_HANDLED;
> +}

Does asus_ec_handle_smi() corrupt the event payload before it is passed to
child notifiers?

If asus_ec_handle_smi() triggers a reset, it eventually calls
asus_ec_clear_buffer(), which performs new I2C reads directly into
ddata->ec_buf. This overwrites the original event data, meaning
blocking_notifier_call_chain() will pass a clobbered buffer to child
devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518095907.36158-1-clamor95@gmail.com?part=2