From 5e5076404aa57ea232079c0dc60698b37719ac85 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 31 Jan 2018 13:15:51 -0800 Subject: [PATCH 1/2] adb: switch asocket's intrusive linked list to vectors. Test: python test_device.py Change-Id: I24d7f5d0401de77d80c7a2dd5a7dcb551943342d --- adb/socket.h | 6 ------ adb/sockets.cpp | 37 ++++++++++--------------------------- 2 files changed, 10 insertions(+), 33 deletions(-) diff --git a/adb/socket.h b/adb/socket.h index 64d05a92a..563e2c787 100644 --- a/adb/socket.h +++ b/adb/socket.h @@ -31,12 +31,6 @@ class atransport; * remote asocket is bound to the protocol engine. */ struct asocket { - /* chain pointers for the local/remote list of - * asockets that this asocket lives in - */ - asocket* next; - asocket* prev; - /* the unique identifier for this asocket */ unsigned id; diff --git a/adb/sockets.cpp b/adb/sockets.cpp index c53fbb4ff..c2f15292b 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -42,27 +42,22 @@ static std::recursive_mutex& local_socket_list_lock = *new std::recursive_mutex(); static unsigned local_socket_next_id = 1; -static asocket local_socket_list = { - .next = &local_socket_list, .prev = &local_socket_list, -}; +static auto& local_socket_list = *new std::vector(); /* the the list of currently closing local sockets. ** these have no peer anymore, but still packets to ** write to their fd. */ -static asocket local_socket_closing_list = { - .next = &local_socket_closing_list, .prev = &local_socket_closing_list, -}; +static auto& local_socket_closing_list = *new std::vector(); // Parse the global list of sockets to find one with id |local_id|. // If |peer_id| is not 0, also check that it is connected to a peer // with id |peer_id|. Returns an asocket handle on success, NULL on failure. asocket* find_local_socket(unsigned local_id, unsigned peer_id) { - asocket* s; - asocket* result = NULL; + asocket* result = nullptr; std::lock_guard lock(local_socket_list_lock); - for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { + for (asocket* s : local_socket_list) { if (s->id != local_id) { continue; } @@ -75,13 +70,6 @@ asocket* find_local_socket(unsigned local_id, unsigned peer_id) { return result; } -static void insert_local_socket(asocket* s, asocket* list) { - s->next = list; - s->prev = s->next->prev; - s->prev->next = s; - s->next->prev = s; -} - void install_local_socket(asocket* s) { std::lock_guard lock(local_socket_list_lock); @@ -92,29 +80,24 @@ void install_local_socket(asocket* s) { fatal("local socket id overflow"); } - insert_local_socket(s, &local_socket_list); + local_socket_list.push_back(s); } void remove_socket(asocket* s) { std::lock_guard lock(local_socket_list_lock); - if (s->prev && s->next) { - s->prev->next = s->next; - s->next->prev = s->prev; - s->next = 0; - s->prev = 0; - s->id = 0; + for (auto list : { &local_socket_list, &local_socket_closing_list }) { + list->erase(std::remove_if(list->begin(), list->end(), [s](asocket* x) { return x == s; }), + list->end()); } } void close_all_sockets(atransport* t) { - asocket* s; - /* this is a little gross, but since s->close() *will* modify ** the list out from under you, your options are limited. */ std::lock_guard lock(local_socket_list_lock); restart: - for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { + for (asocket* s : local_socket_list) { if (s->transport == t || (s->peer && s->peer->transport == t)) { s->close(s); goto restart; @@ -243,7 +226,7 @@ static void local_socket_close(asocket* s) { fdevent_del(&s->fde, FDE_READ); remove_socket(s); D("LS(%d): put on socket_closing_list fd=%d", s->id, s->fd); - insert_local_socket(s, &local_socket_closing_list); + local_socket_closing_list.push_back(s); CHECK_EQ(FDE_WRITE, s->fde.state & FDE_WRITE); } From 5caaebdc3d3424959b176182b2ecde556a16f554 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 2 Feb 2018 14:38:04 -0800 Subject: [PATCH 2/2] adb: restore packet data length checks. These checks were moved to after the read of the payload, which is too late. Add a check before each read to avoid a heap buffer overflow. Test: python test_device.py with x86_64 emulator, walleye Change-Id: I86bcfaaa9004951cc52ad89af74680cf748e717d --- adb/transport.cpp | 5 +++++ adb/transport_usb.cpp | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/adb/transport.cpp b/adb/transport.cpp index 5acaaece6..c90c59c6f 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -72,6 +72,11 @@ bool FdConnection::Read(apacket* packet) { return false; } + if (packet->msg.data_length > sizeof(packet->data)) { + D("remote local: read overflow (data length = %" PRIu32 ")", packet->msg.data_length); + return false; + } + if (!ReadFdExactly(fd_.get(), &packet->data, packet->msg.data_length)) { D("remote local: terminated (data)"); return false; diff --git a/adb/transport_usb.cpp b/adb/transport_usb.cpp index 73e8e15df..a1086999d 100644 --- a/adb/transport_usb.cpp +++ b/adb/transport_usb.cpp @@ -61,6 +61,10 @@ static int UsbReadMessage(usb_handle* h, amessage* msg) { static int UsbReadPayload(usb_handle* h, apacket* p) { D("UsbReadPayload(%d)", p->msg.data_length); + if (p->msg.data_length > sizeof(p->data)) { + return -1; + } + #if CHECK_PACKET_OVERFLOW size_t usb_packet_size = usb_get_max_packet_size(h); CHECK_EQ(0ULL, sizeof(p->data) % usb_packet_size); @@ -116,6 +120,11 @@ static int remote_read(apacket* p, usb_handle* usb) { } if (p->msg.data_length) { + if (p->msg.data_length > sizeof(p->data)) { + PLOG(ERROR) << "remote usb: read overflow (data length = " << p->msg.data_length << ")"; + return -1; + } + if (usb_read(usb, p->data, p->msg.data_length)) { PLOG(ERROR) << "remote usb: terminated (data)"; return -1;