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

Weiming Shi <[email protected]>
Newsgroups org.kernel.vger.linux-i2c,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
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 <[email protected]>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <[email protected]>
---
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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.