[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@xxxxxxxxx>
---
 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