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

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

[Maritu Hone]

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@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.
+	 */
+	gb_tty->buffer = kzalloc(max_payload, GFP_KERNEL);
 	if (!gb_tty->buffer) {
 		retval = -ENOMEM;
 		goto exit_put_port;
-- 
2.43.0

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

[Alex Elder]

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@gmail.com>

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.

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.


I think I agree, but Bryan should confirm.




I will comment that sizeof(struct gb_uart_send_data_request)
should never be used, because it contains a flexible array
element.  Instead, flex_array_size() should be used, for
example:

     struct gb_uart_send_data_request *request;
     size_t send_data_min_payload;

     send_data_min_payload = flex_array_size(request, data, 0);
     buffer_payload_max = max_payload - send_data_min_payload;

I'm not sure flex_array_size() was available when this was written.

					-Alex

> ---
>   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.
> +	 */
> +	gb_tty->buffer = kzalloc(max_payload, GFP_KERNEL);
>   	if (!gb_tty->buffer) {
>   		retval = -ENOMEM;
>   		goto exit_put_port;