Skip to contentSkip to conversation

greybus: operation: fix out-of-bounds write in message allocation

PATCH In review Adriano Cordova 2 participants 3 messages Sep 2 – Sep 4

Subsystems (2)
Stable branch Greybus
lore
Share & tools
lore

Adriano Cordova

author
#0 lore raw

PATCH v2 1/1 greybus: operation: fix out-of-bounds write in message allocation

The incoming message size from the device header (header.size) is trusted without checking that it is at least the size of the message header itself, but a value smaller than sizeof(struct gb_operation_msg_hdr) underflows request_size in gb_operation_create_incoming(), wraps around in gb_operation_message_alloc(), and results in a tiny buffer that is then written past its end in gb_operation_message_init().

Reject undersized messages before parsing the message header.

Fixes 87d208feb74f ("greybus: embed message buffer into message structure")
Reported-by [email protected]
Link https://syzkaller.appspot.com/bug?extid=2fd6aefc361af86911d5
Cc [email protected]
Assisted-by opencode:deepseek v4 pro
Signed-off-by Adriano Cordova <[email protected]>

---
v2: point the Fixes tag at the proper commit (87d208feb74f), and add an
Assisted-by tag.

 drivers/greybus/operation.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/greybus/operation.c +8 -0
index 7e12ffb2dd..df6daee4fb 100644
@@ -1047,6 +1047,14 @@ void gb_connection_recv(struct gb_connection *connection,
 	/* Use memcpy as data may be unaligned */
 	memcpy(&header, data, sizeof(header));
 	msg_size = le16_to_cpu(header.size);
+	if (msg_size < sizeof(header)) {
+		dev_err_ratelimited(dev,
+				    "%s: malformed message 0x%04x of type 0x%02x received (%zu < %zu)\n",
+				    connection->name,
+				    le16_to_cpu(header.operation_id),
+				    header.type, msg_size, sizeof(header));
+		return;
+	}
 	if (size < msg_size) {
 		dev_err_ratelimited(dev,
 				    "%s: incomplete message 0x%04x of type 0x%02x received (%zu < %zu)\n",
Signature · 1 line
2.51.0
maintainer
#1 lore raw
Quoted 6 lines · Adriano Cordova

The incoming message size from the device header (header.size) is trusted without checking that it is at least the size of the message header itself, but a value smaller than sizeof(struct gb_operation_msg_hdr) underflows request_size in gb_operation_create_incoming(), wraps around in gb_operation_message_alloc(), and results in a tiny buffer that is then written past its end in gb_operation_message_init().

Is it really? I was under the impression the only issue here was the potential zero-size-pointer deref.

Quoted 11 lines

Reject undersized messages before parsing the message header.

Fixes 87d208feb74f ("greybus: embed message buffer into message structure")
Reported-by [email protected]
Link https://syzkaller.appspot.com/bug?extid=2fd6aefc361af86911d5
Cc [email protected]
Assisted-by opencode:deepseek v4 pro
Signed-off-by Adriano Cordova <[email protected]>

---
v2: point the Fixes tag at the proper commit (87d208feb74f), and add an
Assisted-by tag.

Either way, Yang has already sent a fix for this as I mentioned before (and which is now ready to be merged):

	https://lore.kernel.org/lkml/[email protected]/

Johan

maintainer
#2 lore raw
Quoted 10 lines · Johan Hovold
Quoted 6 lines · Adriano Cordova

The incoming message size from the device header (header.size) is trusted without checking that it is at least the size of the message header itself, but a value smaller than sizeof(struct gb_operation_msg_hdr) underflows request_size in gb_operation_create_incoming(), wraps around in gb_operation_message_alloc(), and results in a tiny buffer that is then written past its end in gb_operation_message_init().

Is it really? I was under the impression the only issue here was the potential zero-size-pointer deref.

You're right of course, there's a potential small (7 byte) OOB write here too.

Johan