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

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

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

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

[Johan Hovold]

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

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

[Johan Hovold]

On Tue, Nov 03, 2026 at 11:44:55AM +0300, Dan Carpenter wrote:
> 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);

> 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

It's the kfifo_out_peek() in gb_uart_tx_write_work() that may write 2
bytes beyond gb_tty->buffer (as its size is currently just
gb_tty->buffer_payload_max).

Johan

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

[Dan Carpenter]

On Tue, Nov 03, 2026 at 10:12:50AM +0100, Johan Hovold wrote:
> On Tue, Nov 03, 2026 at 11:44:55AM +0300, Dan Carpenter wrote:
> > 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);
> 
> > 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
> 
> It's the kfifo_out_peek() in gb_uart_tx_write_work() that may write 2
> bytes beyond gb_tty->buffer (as its size is currently just
> gb_tty->buffer_payload_max).

Ah, right.

The right way to fix this is to have a commit message that
says.  "The gb_tty->buffer pointer is supposed to hold a struct with
a flex array at the end, but gb_uart_probe() only allocates enough
space for the flex array so it's two bytes too small.

Part of the reason for this bug is because it uses a void pointer
instead of a struct pointer.  Clean that up as well to prevent
future bugs."

Please send something like this in v2.  (Send exactly this code
but edit the commit message if you want or leave it as-is).

regards,
dan carpenter

diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index e1b73db6a17c..7d1985d63482 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -43,7 +43,7 @@
 struct gb_tty {
 	struct gbphy_device *gbphy_dev;
 	struct tty_port port;
-	void *buffer;
+	struct gb_uart_send_data_request *request;
 	size_t buffer_payload_max;
 	struct gb_connection *connection;
 	u16 cport_id;
@@ -230,7 +230,7 @@ static void  gb_uart_tx_write_work(struct work_struct *work)
 	int ret;
 
 	gb_tty = container_of(work, struct gb_tty, tx_work);
-	request = gb_tty->buffer;
+	request = gb_tty->request;
 
 	while (1) {
 		if (gb_tty->close_pending)
@@ -775,7 +775,7 @@ static void gb_tty_port_destruct(struct tty_port *port)
 	if (gb_tty->minor != GB_NUM_MINORS)
 		release_minor(gb_tty);
 	kfifo_free(&gb_tty->write_fifo);
-	kfree(gb_tty->buffer);
+	kfree(gb_tty->request);
 	kfree(gb_tty);
 }
 
@@ -842,8 +842,9 @@ 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);
-	if (!gb_tty->buffer) {
+	gb_tty->request = kzalloc_flex(*gb_tty->request, data,
+				       gb_tty->buffer_payload_max);
+	if (!gb_tty->request) {
 		retval = -ENOMEM;
 		goto exit_put_port;
 	}

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

[Johan Hovold]

On Tue, Nov 03, 2026 at 12:28:19PM +0300, Dan Carpenter wrote:
> On Tue, Nov 03, 2026 at 10:12:50AM +0100, Johan Hovold wrote:

> > It's the kfifo_out_peek() in gb_uart_tx_write_work() that may write 2
> > bytes beyond gb_tty->buffer (as its size is currently just
> > gb_tty->buffer_payload_max).
> 
> Ah, right.
> 
> The right way to fix this is to have a commit message that
> says.  "The gb_tty->buffer pointer is supposed to hold a struct with
> a flex array at the end, but gb_uart_probe() only allocates enough
> space for the flex array so it's two bytes too small.
> 
> Part of the reason for this bug is because it uses a void pointer
> instead of a struct pointer.  Clean that up as well to prevent
> future bugs."
> 
> Please send something like this in v2.  (Send exactly this code
> but edit the commit message if you want or leave it as-is).

Looks good, but since kzalloc_flex() was introduced in 7.0 (and this
bugs goes back to 4.9 it may be better to do that conversion in a
follow-up patch to assist the backporters.

Johan

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

[Dan Carpenter]

On Tue, Nov 03, 2026 at 10:49:10AM +0100, Johan Hovold wrote:
> On Tue, Nov 03, 2026 at 12:28:19PM +0300, Dan Carpenter wrote:
> > On Tue, Nov 03, 2026 at 10:12:50AM +0100, Johan Hovold wrote:
> 
> > > It's the kfifo_out_peek() in gb_uart_tx_write_work() that may write 2
> > > bytes beyond gb_tty->buffer (as its size is currently just
> > > gb_tty->buffer_payload_max).
> > 
> > Ah, right.
> > 
> > The right way to fix this is to have a commit message that
> > says.  "The gb_tty->buffer pointer is supposed to hold a struct with
> > a flex array at the end, but gb_uart_probe() only allocates enough
> > space for the flex array so it's two bytes too small.
> > 
> > Part of the reason for this bug is because it uses a void pointer
> > instead of a struct pointer.  Clean that up as well to prevent
> > future bugs."
> > 
> > Please send something like this in v2.  (Send exactly this code
> > but edit the commit message if you want or leave it as-is).
> 
> Looks good, but since kzalloc_flex() was introduced in 7.0 (and this
> bugs goes back to 4.9 it may be better to do that conversion in a
> follow-up patch to assist the backporters.

Yep.  Good point.

regards,
dan carpenter