Re: [PATCH] staging: greybus: uart: fix heap buffer overflow in gb_tty->buffer

[Johan Hovold <johan@kernel.org>]

On Mon, Nov 02, 2026 at 10:53:32PM +0300, Maritu Hone wrote:
> gb_tty->buffer is allocated once in gb_uart_probe() and reused for
> every outgoing chunk as the backing store for a
> struct gb_uart_send_data_request:
> 
> 	struct gb_uart_send_data_request {
> 		__le16	size;
> 		__u8	data[];
> 	} __packed;
> 
> In gb_uart_tx_write_work():
> 
> 	request = gb_tty->buffer;
> 	...
> 	send_size = gb_tty->buffer_payload_max;
> 	if (send_size > gb_tty->credits)
> 		send_size = gb_tty->credits;
> 	send_size = kfifo_out_peek(&gb_tty->write_fifo,
> 				   &request->data[0], send_size);
> 
> request->data starts 2 bytes (sizeof(request->size)) into
> gb_tty->buffer, and up to buffer_payload_max bytes are written there
> whenever enough data is queued and credits allow it. That needs
> buffer_payload_max + sizeof(struct gb_uart_send_data_request) bytes
> of backing storage, i.e. max_payload bytes.
> 
> gb_tty->buffer is allocated with only buffer_payload_max bytes:
> 
> 	gb_tty->buffer_payload_max = max_payload -
> 			sizeof(struct gb_uart_send_data_request);
> 	gb_tty->buffer = kzalloc(gb_tty->buffer_payload_max, GFP_KERNEL);
> 
> so a full-size write overruns the allocation by exactly
> sizeof(struct gb_uart_send_data_request) (2) bytes, corrupting
> whatever follows it on the heap.
> 
> Allocate max_payload bytes instead, which is exactly
> sizeof(struct gb_uart_send_data_request) + buffer_payload_max, so the
> buffer is large enough for the header and the full data payload it is
> used to hold.

I think the above is clear enough.

> Fixes: dd1c64ede97f ("greybus: uart: kmalloc for send_data once only")

This should also go to stable:

Cc: stable@vger.kernel.org

> Signed-off-by: Maritu Hone <marehone12@gmail.com>
> ---
>  drivers/staging/greybus/uart.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
> index e1b73db6a17c..7bbac2603726 100644
> --- a/drivers/staging/greybus/uart.c
> +++ b/drivers/staging/greybus/uart.c
> @@ -842,7 +842,15 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
>  	gb_tty->buffer_payload_max = max_payload -
>  			sizeof(struct gb_uart_send_data_request);
>  
> -	gb_tty->buffer = kzalloc(gb_tty->buffer_payload_max, GFP_KERNEL);
> +	/*
> +	 * gb_tty->buffer is reused in gb_uart_tx_write_work() as the
> +	 * backing store for a struct gb_uart_send_data_request, whose
> +	 * variable-length data[] field holds up to buffer_payload_max
> +	 * bytes starting right after the struct's header. The allocation
> +	 * therefore needs room for the header plus buffer_payload_max
> +	 * bytes of data, i.e. max_payload bytes, not buffer_payload_max.
> +	 */

LLMs like to add comments like this, but please remove it in a v2.

> +	gb_tty->buffer = kzalloc(max_payload, GFP_KERNEL);
>  	if (!gb_tty->buffer) {
>  		retval = -ENOMEM;
>  		goto exit_put_port;

Johan