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

[Dan Carpenter <error27@gmail.com>]

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