Re: [PATCH v7 3/7] input: serio: Add driver for ASUS Transformer dock keyboard and touchpad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleeping in atomic context due to synchronous I2C transfer in serio->write callback.
- [High] Off-by-one error in packet length calculation causes a buffer over-read and breaks keycode remapping.
- [Medium] In-place modification of a shared notifier data buffer.
--

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

input: serio: Add driver for ASUS Transformer dock keyboard and touchpad

This commit adds a serio input driver for the ASUS Transformer dock keyboard
and touchpad. It registers serio ports that act as a bridge over I2C and
remaps specific keycodes according to their position.

> diff --git a/drivers/input/serio/asus-transformer-ec-kbc.c b/drivers/input/serio/asus-transformer-ec-kbc.c
> new file mode 100644
> index 0000000000000..c9707c7fbede5
> --- /dev/null
> +++ b/drivers/input/serio/asus-transformer-ec-kbc.c

[ ... ]

> +static int asus_ec_kbc_notify(struct notifier_block *nb,
> +			      unsigned long action, void *data_)
> +{
> +	struct asus_ec_kbc_data *priv = container_of(nb, struct asus_ec_kbc_data, nb);
> +	unsigned int port_idx, n;
> +	u8 *data = data_;
> +
> +	if (action & (ASUSEC_SMI_MASK | ASUSEC_SCI_MASK))
> +		return NOTIFY_DONE;
> +	else if (action & ASUSEC_AUX_MASK)
> +		port_idx = 1;
> +	else if (action & (ASUSEC_KBC_MASK | ASUSEC_KEY_MASK))
> +		port_idx = 0;
> +	else
> +		return NOTIFY_DONE;
> +
> +	/*
> +	 * The data[0] is the length of the packet including itself. It has to
> +	 * be at least 2 bytes (length + 1 data byte) and must not exceed
> +	 * the EC buffer size.
> +	 */
> +	if (data[0] < 2 || data[0] > DOCKRAM_ENTRY_BUFSIZE)
> +		return NOTIFY_BAD;
> +
> +	n = data[0] - 1;
> +	data += 2;

Is the payload length calculation off by one?

The comment states data[0] is the length of the packet including itself.
The code then skips two bytes (data += 2), which suggests the payload
starts at offset 2. If the payload length is data[0] - 2, will setting
n = data[0] - 1 cause the loop at the end of the function to read one byte
past the actual payload and inject garbage data into the serio port?

If an external device sends a packet with data[0] == DOCKRAM_ENTRY_BUFSIZE,
could this off-by-one error cause an out-of-bounds read past the end of
the EC buffer array?

Additionally, will this artificially inflated n cause the exact-length
validation in the key remapping logic (if (n == 2 ...) and if (n == 3 ...))
to always fail for valid payload sequences, breaking the layout translation?

> +
> +	if (port_idx == 0) {
> +		/*
> +		 * Remap keyboard key codes to match AT layout:
> +		 * SEARCH: RIGHT-META [E0 27] -> LEFT-ALT   [11]
> +		 * MENU:   COMPOSE    [E0 2F] -> RIGHT-META [E0 27]
> +		 */
> +		if ((n == 2 || (n == 3 && data[1] == 0xF0)) && data[0] == 0xE0) {
> +			u8 *keycode = &data[n - 1];
> +
> +			switch (*keycode) {
> +			case 0x27:
> +				*keycode = 0x11;

Is it safe to modify the data_ buffer in place?

Since data_ is passed to this function via the notifier chain, it points to a
shared event buffer that is broadcast to all registered listeners. Will
modifying this payload permanently corrupt the event data for any downstream
notifiers that might inspect the packet subsequently?

> +				++data;
> +				--n;
> +				break;
> +			case 0x2F:
> +				*keycode = 0x27;
> +				break;
> +			}
> +		}
> +	}
> +
> +	while (n--)
> +		serio_interrupt(priv->sdev[port_idx], *data++, 0);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int asus_ec_serio_write(struct serio *port, unsigned char data)
> +{
> +	struct asus_ec_kbc_data *priv = port->port_data;
> +
> +	return i2c_smbus_write_word_data(priv->parent, ASUSEC_WRITE_BUF,
> +					 (data << 8) | port->id.extra);

Can this sleep while in an atomic context?

i2c_smbus_write_word_data() performs a synchronous I2C transaction which
acquires mutexes and can sleep. However, serio->write callbacks can be
invoked from atomic contexts.

For example, asus_ec_kbc_notify() calls serio_interrupt() which acquires a
spinlock and disables interrupts:

serio_interrupt()
  spin_lock_irqsave(&serio->lock, flags);
  serio->drv->interrupt()
    serio_write()
      asus_ec_serio_write()
        i2c_smbus_write_word_data()

Will this trigger a scheduling-while-atomic panic if the input driver issues
a command back to the port while the spinlock is held?

> +}

[ ... ]

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