[PATCH v2] i2c: i801: keep the byte-by-byte ISR buffer private to the driver

[PATCH v2] i2c: i801: keep the byte-by-byte ISR buffer private to the driver

[Weiming Shi]

For interrupt-driven byte-by-byte block transfers,
i801_block_transaction_byte_by_byte() points priv->data at the caller's
union i2c_smbus_data and lets the BYTE_DONE interrupt handler
i801_isr_byte_done() fill or drain it one byte at a time.  In the i2c-dev
ioctl path that buffer lives on the caller's kernel (vmap) stack.

On a wait_for_completion_timeout() the transfer thread returns -ETIMEDOUT
without fencing the interrupt, and a BYTE_DONE that is still in flight or
fires later then writes a byte through priv->data
(priv->data[priv->count++] = inb(...)) after the caller has returned and
its vmap stack has been freed:

  BUG: KASAN: stack-out-of-bounds in i801_isr
  Write of size 1 at addr ffffc90002a2fda9 by task exploit/5144
   <IRQ>
   i801_isr_byte_done drivers/i2c/busses/i2c-i801.c:546 [inlined]
   i801_isr drivers/i2c/busses/i2c-i801.c:613
   __handle_irq_event_percpu kernel/irq/handle.c:158
   handle_irq_event kernel/irq/handle.c:195
   handle_fasteoi_irq kernel/irq/chip.c:661
   __common_interrupt arch/x86/kernel/irq.c:263
   common_interrupt arch/x86/kernel/irq.c:240
   </IRQ>
  The buggy address belongs to a freed vmap kernel stack (task exploit),
  created by kernel_clone -> copy_process.

The completion-based paths (i801_transaction(),
i801_block_transaction_by_block()) never expose the caller's buffer to the
handler: they copy to/from the hardware block buffer in process context
and use the interrupt only to signal completion.  Give the byte-by-byte
path the same property.  The handler now fills a driver-private buffer
(priv->data_buf); the transfer copies the caller's data in before starting
and copies the result back out on success, so a late or spurious BYTE_DONE
can only ever touch driver-owned memory.  i801_block_transaction() already
bounds data->block[0] to I2C_SMBUS_BLOCK_MAX, so the buffer cannot overflow.

Fixes: d3ff6ce40031 ("i2c-i801: Enable IRQ for byte_by_byte transactions")
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
Changes in v2:
- Fix the author: From/Signed-off-by. No code change.

 drivers/i2c/busses/i2c-i801.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c8cb5ed55..37741fdca 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -279,6 +279,9 @@ struct i801_priv {
 	int count;
 	int len;
 	u8 *data;
+	/* Driver-private buffer the isr fills/drains, so a late interrupt
+	 * never dereferences a pointer into the caller's block buffer. */
+	u8 data_buf[I2C_SMBUS_BLOCK_MAX + 2];
 
 #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
 	const struct i801_mux_config *mux_drvdata;
@@ -671,12 +674,23 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
 		priv->len = len;
 		priv->count = 0;
-		priv->data = &data->block[1];
+		/*
+		 * The interrupt handler fills or drains this buffer
+		 * asynchronously and may still run after a timeout, so keep it
+		 * in driver-private storage instead of pointing into the
+		 * caller's block buffer, which is freed once we return.
+		 */
+		memcpy(priv->data_buf, data->block, len + 1);
+		priv->data = &priv->data_buf[1];
 
 		reinit_completion(&priv->done);
 		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
 		result = wait_for_completion_timeout(&priv->done, adap->timeout);
-		return result ? priv->status : -ETIMEDOUT;
+		if (!result)
+			return -ETIMEDOUT;
+		if (priv->is_read)
+			memcpy(data->block, priv->data_buf, priv->len + 1);
+		return priv->status;
 	}
 
 	for (i = 1; i <= len; i++) {
-- 
2.43.0