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

[Dan Carpenter]

On Mon, Nov 02, 2026 at 03:33:58PM -0600, Alex Elder wrote:
> On 11/2/26 1:53 PM, 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.
> > 
> > Fixes: dd1c64ede97f ("greybus: uart: kmalloc for send_data once only")
> > Signed-off-by: Maritu Hone <marehone12@xxxxxxxxx>
> 
> I think I understand what you're saying, but it took a bit
> for me to follow it.  I try explaining it in my own words;
> please make sure it aligns with what you mean.  I think it's
> also helpful to explain how this issue was found (by a tool,
> or by inspection, or through examining the corruption that
> was produced as a result of this).
> 
> 
> The UART Greybus driver allocates a single buffer to be used
> for all send_data operations on a connection.  It allocates it
> in gb_uart_connection_init(), with a size allocated to be as
> big as possible for the connection, but the size used is wrong.

gb_uart_connection_init() was renamed to gb_uart_probe() ten
and a half years ago...  :P

> 
> The error is in gb_uart_connection_init(), where the buffer
> size is computed and the buffer is allocated.  There, the size
> of the buffer is computed to be the maximum size of the data
> portion of a gb_uart_send_data_request structure (*without*
> the size field).
> 
> The problem occurs in gb_uart_tx_write_work(), where the buffer
> is cast to be a gb_uart_send_data_request structure.  That means
> the buffer contains a *full* gb_uart_send_data_request structure,
> not just that structure's data portion.
> 
> You're saying that the buffer size calculated in gb_uart_probe()
> should be max_payload (the maximum operation message payload
> size), not the calculated buffer_payload_max value.

Wow..  Heh...  I have read both the original message and your
message and I still have no idea which function contains the
actual memcpy() overflow.  :P

There are two sizes:
1.  The size of the whole struct from
    gb_operation_get_payload_size_max(connection).
2.  The size of the variable length array stored in
    gb_tty->buffer_payload_max.

I have reviewed gb_uart_tx_write_work() and it uses
send_size (which is the size of the variable length array)
consistently and correctly so far as I can see.

I didn't follow through each function to see if there was a math
error somewhere else.  I'm not going to do that because either
way the commit message needs to be more clear where the actual
corruption occurs.

regards,
dan carpenter