Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mfd: asus-transformer-ec: UAF on dockram_ec_data in devm_asus_dockram_get()
- [Medium] mfd: asus-transformer-ec: ec_buf clobbered in asus_ec_interrupt() on SMI
- [Medium] mfd: asus-transformer-ec: fixed retry count in asus_ec_clear_buffer()
- [Low] mfd: asus-transformer-ec: inverted comment in asus_ec_clear_buffer()
- [Low] mfd: asus-transformer-ec: sibling input state in asusec_core and mfd naming
--
commit 127ab1fbe238a806a6be88edfad37b1c227e1824
Author: Michał Mirosław [off-list ref]
mfd: Add driver for ASUS Transformer embedded controller
This commit adds a core MFD driver for the Nuvoton NPCE795-based embedded
controller found in ASUS Transformer tablets and docks, handling I2C
communication, detection, and sub-device registration.
quoted hunk ↗ jump to hunk
diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
new file mode 100644
index 0000000000000..2f575ecb4f58b
--- /dev/null
+++ b/drivers/mfd/asus-transformer-ec.c
[ ... ]
quoted hunk ↗ jump to hunk
+#define ASUSEC_ACCESS_TIMEOUT 300
+#define ASUSEC_DOCKRAM_OFFSET 2
+#define ASUSEC_ECREQ_DELAY 50
+#define ASUSEC_ECREQ_TIMEOUT 200
+#define ASUSEC_RESET 0
+#define ASUSEC_RETRY_MAX 3
+#define ASUSEC_RSP_BUFFER_SIZE (ASUSEC_ENTRIES / ASUSEC_ENTRY_SIZE)
+#define ASUSEC_SL101_ENTRY_SIZE 8
[ ... ]
quoted hunk ↗ jump to hunk
+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,
+ ddata->info->read_size, ddata->ec_buf);
+ if (ret < ddata->info->read_size)
+ continue;
+
+ if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
+ continue;
+
+ break;
+ }
+}
[Severity: Medium]
In asus_ec_clear_buffer(), retry is initialized to ASUSEC_RSP_BUFFER_SIZE,
which divides ASUSEC_ENTRIES (256) by ASUSEC_ENTRY_SIZE (32) for 8
iterations.
However, each iteration reads ddata->info->read_size bytes. On the SL101
dock variant (asus_ec_sl101_dock_data), read_size is ASUSEC_SL101_ENTRY_SIZE
(8), so 8 iterations only drain at most 64 bytes instead of the full 256-byte
EC buffer.
Should the retry count be computed from ddata->info->read_size instead of
the fixed ASUSEC_ENTRY_SIZE?
[Severity: Low]
Also in asus_ec_clear_buffer(), the comment states "Read the buffer till we
get valid data by checking ASUSEC_OBF_MASK", whereas the loop actually
continues while ASUSEC_OBF_MASK is set (valid data present) and breaks once
the buffer is empty. Should the comment say it reads until the buffer no
longer has valid data?
[ ... ]
quoted hunk ↗ jump to hunk
+static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
+{
+ switch (code) {
+ case ASUSEC_SMI_HANDSHAKE:
+ case ASUSEC_SMI_RESET:
+ asus_ec_init(ddata);
+ break;
+ }
+}
+
+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,
+ ddata->info->read_size, ddata->ec_buf);
+ if (ret < ddata->info->read_size)
+ return IRQ_NONE;
+
+ /* Check status byte with ASUSEC_OBF_MASK if data is valid */
+ ret = ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK;
+ if (!ret)
+ return IRQ_NONE;
+
+ 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;
+}
[Severity: Medium]
When asus_ec_interrupt() receives an SMI event with ASUSEC_SMI_HANDSHAKE or
ASUSEC_SMI_RESET, it calls:
asus_ec_interrupt()
-> asus_ec_handle_smi()
-> asus_ec_init()
-> asus_ec_clear_buffer()
asus_ec_clear_buffer() reads into the same shared ddata->ec_buf buffer until
ASUSEC_OBF_MASK is clear, overwriting the SMI packet that triggered the
interrupt before blocking_notifier_call_chain() passes ddata->ec_buf to
registered listeners. In addition, asus_ec_handle_smi() ignores the return
value of asus_ec_init(), so notifier callbacks still run even if
reinitialization failed.
Should asus_ec_clear_buffer() use a separate buffer (or should
asus_ec_interrupt() avoid passing a clobbered ddata->ec_buf and check for
asus_ec_init() errors)?
quoted hunk ↗ jump to hunk
+static void asus_ec_release_dockram_dev(void *client)
+{
+ i2c_unregister_device(client);
+}
+
+static struct i2c_client *devm_asus_dockram_get(struct device *dev)
+{
+ struct i2c_client *parent = to_i2c_client(dev);
+ struct i2c_client *dockram;
+ struct dockram_ec_data *ddata;
+ int ret;
+
+ dockram = i2c_new_ancillary_device(parent, "dockram",
+ parent->addr + ASUSEC_DOCKRAM_OFFSET);
+ if (IS_ERR(dockram))
+ return dockram;
+
+ ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev, dockram);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ddata = devm_kzalloc(&dockram->dev, sizeof(*ddata), GFP_KERNEL);
+ if (!ddata)
+ return ERR_PTR(-ENOMEM);
+
+ i2c_set_clientdata(dockram, ddata);
+ mutex_init(&ddata->ctl_lock);
+
+ return dockram;
+}
[Severity: High]
As reported by Dmitry Torokhov on v11 [1], devm_asus_dockram_get() allocates
ddata with devm_kzalloc(&dockram->dev, ...) and attaches it via
i2c_set_clientdata(dockram, ddata) on the ancillary dummy I2C device rather
than the parent device dev.
Because dockram is bound to the I2C dummy driver, unbinding dockram from the
dummy driver via sysfs triggers device_unbind_cleanup(&dockram->dev), which
frees &dockram->dev devres allocations and clears its driver_data to NULL
while the parent MFD driver and child sub-devices remain active. Concurrent
calls to asus_dockram_access_ctl() can then suffer a use-after-free on
ddata->ctl_lock and ddata->ctl_buf across sleeping SMBus transfers, and
subsequent calls will fail with -ENODEV.
Could ddata be allocated with devm_kzalloc(dev, ...) on the parent device instead?
quoted hunk ↗ jump to hunk
+static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
+ MFD_CELL_NAME("asus-transformer-ec-kbc"),
+};
+
+static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
+ MFD_CELL_NAME("asus-transformer-ec-battery"),
+ MFD_CELL_NAME("asus-transformer-ec-charger"),
+ MFD_CELL_NAME("asus-transformer-ec-led"),
+ MFD_CELL_NAME("asus-transformer-ec-kbc"),
+ MFD_CELL_NAME("asus-transformer-ec-keys"),
+};
+
+static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
+ MFD_CELL_NAME("asus-transformer-ec-battery"),
+ MFD_CELL_NAME("asus-transformer-ec-led"),
+};
+
+static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
+ MFD_CELL_NAME("asus-transformer-ec-battery"),
+ MFD_CELL_NAME("asus-transformer-ec-charger"),
+ MFD_CELL_NAME("asus-transformer-ec-led"),
+};
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
new file mode 100644
index 0000000000000..fda6fb08cc41a
--- /dev/null
+++ b/include/linux/mfd/asus-transformer-ec.h
[ ... ]
quoted hunk ↗ jump to hunk
+struct asusec_core {
+ const char *model;
+ const char *name;
+ struct i2c_client *dockram;
+ struct blocking_notifier_head notify_list;
+ u8 keyboard_mask;
+ bool ext_keys_enabled;
+ bool right_alt_pressed;
+ bool special_combo_triggered;
+};
[Severity: Low]
This isn't a bug, but in struct asusec_core, the fields ext_keys_enabled,
right_alt_pressed, and special_combo_triggered are unused by the core MFD
driver in drivers/mfd/asus-transformer-ec.c and appear to be private input
state shared between sibling sub-devices. Also, the mfd_cell arrays in
drivers/mfd/asus-transformer-ec.c (such as asus_ec_sl101_dock_mfd_devices)
include "mfd" in their names. Would it be cleaner to avoid exposing
sibling-specific input state in the core MFD structure and drop "mfd" from
the cell array names in accordance with MFD conventions?
[1] https://lore.kernel.org/r/d5f54636c25e0e60731adc200a63ceb5@gmail.com (local)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261104122606.215629-1-clamor95@gmail.com?part=2