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

[Andi Shyti <andi.shyti@kernel.org>]

Hi Weiming,

Jean, any chance you can review this?

On Sat, Jul 18, 2026 at 02:03:30AM -0700, Weiming Shi wrote:
> 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:

How are you fixing in this patch the pending BYTE_DONE interrupt?

Andi

>   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
>