Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix use-after-free in UpdateAdditionalDataCharacteristic #4966

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

mspang
Copy link
Contributor

@mspang mspang commented Feb 23, 2021

This fixes the following error reported by valgrind in lighting-app/linux:

==4071== Invalid read of size 2
==4071==    at 0x4851158: __GI_memcpy (in /usr/lib/aarch64-linux-gnu/valgrind/vgpreload_memcheck-arm64-linux.so)
==4071==    by 0x4CF0893: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1E8B: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1B2F: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1BC7: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1803: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF4D87: g_dbus_message_to_blob (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CE7E1F: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CEB5B3: g_dbus_connection_send_message (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CEF7DF: g_dbus_connection_emit_signal (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x1704F3: _bluez_gatt_characteristic1_emit_changed (DbusBluez.c:12485)
==4071==    by 0x4E909EF: g_main_context_dispatch (in /usr/lib/aarch64-linux-gnu/libglib-2.0.so.0.6600.1)
==4071==  Address 0x5a0df16 is 86 bytes inside a block of size 1,607 free'd
==4071==    at 0x484CF70: free (in /usr/lib/aarch64-linux-gnu/valgrind/vgpreload_memcheck-arm64-linux.so)
==4071==    by 0x12EA23: chip::Platform::MemoryFree(void*) (CHIPMem-Malloc.cpp:132)
==4071==    by 0x14B88B: chip::System::PacketBuffer::Free(chip::System::PacketBuffer*) (SystemPacketBuffer.cpp:545)
==4071==    by 0x127C33: chip::System::PacketBufferHandle::operator=(decltype(nullptr)) (SystemPacketBuffer.h:448)
==4071==    by 0x127BFB: chip::System::PacketBufferHandle::~PacketBufferHandle() (SystemPacketBuffer.h:425)
==4071==    by 0x139F2F: chip::DeviceLayer::Internal::UpdateAdditionalDataCharacteristic(_BluezGattCharacteristic1*) (Helper.cpp:1281)
==4071==    by 0x13A4FB: chip::DeviceLayer::Internal::BluezPeripheralObjectsSetup(void*) (Helper.cpp:1368)
==4071==    by 0x13A5FB: chip::DeviceLayer::Internal::BluezOnBusAcquired(_GDBusConnection*, char const*, void*) (Helper.cpp:1392)
==4071==    by 0x13A733: chip::DeviceLayer::Internal::BluezMainLoop(void*) (Helper.cpp:1427)
==4071==    by 0x486FF73: start_thread (pthread_create.c:463)
==4071==    by 0x523F3DB: thread_start (clone.S:78)
==4071==  Block was alloc'd at
==4071==    at 0x484BDDC: malloc (in /usr/lib/aarch64-linux-gnu/valgrind/vgpreload_memcheck-arm64-linux.so)
==4071==    by 0x12E997: chip::Platform::MemoryAlloc(unsigned long, bool) (CHIPMem-Malloc.cpp:112)
==4071==    by 0x12E96B: chip::Platform::MemoryAlloc(unsigned long) (CHIPMem-Malloc.cpp:106)
==4071==    by 0x14B5D7: chip::System::PacketBufferHandle::New(unsigned long, unsigned short) (SystemPacketBuffer.cpp:458)
==4071==    by 0x1458EF: chip::AdditionalDataPayloadGenerator::generateAdditionalDataPayload(unsigned short, char const*, unsigned long, chip::System::PacketBufferHandle&, chip::BitFlags<unsigned char, chip::AdditionalDataFields>) (AdditionalDataPayloadGenerator.cpp:56)
==4071==    by 0x139E43: chip::DeviceLayer::Internal::UpdateAdditionalDataCharacteristic(_BluezGattCharacteristic1*) (Helper.cpp:1297)
==4071==    by 0x13A4FB: chip::DeviceLayer::Internal::BluezPeripheralObjectsSetup(void*) (Helper.cpp:1368)
==4071==    by 0x13A5FB: chip::DeviceLayer::Internal::BluezOnBusAcquired(_GDBusConnection*, char const*, void*) (Helper.cpp:1392)
==4071==    by 0x13A733: chip::DeviceLayer::Internal::BluezMainLoop(void*) (Helper.cpp:1427)
==4071==    by 0x486FF73: start_thread (pthread_create.c:463)
==4071==    by 0x523F3DB: thread_start (clone.S:78)

We need to pass a pointer to the dup'd data in, not the original packet buffer.

This fixes the following error reported by valgrind in lighting-app/linux:

==4071== Invalid read of size 2
==4071==    at 0x4851158: __GI_memcpy (in /usr/lib/aarch64-linux-gnu/valgrind/vgpreload_memcheck-arm64-linux.so)
==4071==    by 0x4CF0893: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1E8B: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1B2F: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1BC7: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF1803: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CF4D87: g_dbus_message_to_blob (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CE7E1F: ??? (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CEB5B3: g_dbus_connection_send_message (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x4CEF7DF: g_dbus_connection_emit_signal (in /usr/lib/aarch64-linux-gnu/libgio-2.0.so.0.6600.1)
==4071==    by 0x1704F3: _bluez_gatt_characteristic1_emit_changed (DbusBluez.c:12485)
==4071==    by 0x4E909EF: g_main_context_dispatch (in /usr/lib/aarch64-linux-gnu/libglib-2.0.so.0.6600.1)
==4071==  Address 0x5a0df16 is 86 bytes inside a block of size 1,607 free'd
==4071==    at 0x484CF70: free (in /usr/lib/aarch64-linux-gnu/valgrind/vgpreload_memcheck-arm64-linux.so)
==4071==    by 0x12EA23: chip::Platform::MemoryFree(void*) (CHIPMem-Malloc.cpp:132)
==4071==    by 0x14B88B: chip::System::PacketBuffer::Free(chip::System::PacketBuffer*) (SystemPacketBuffer.cpp:545)
==4071==    by 0x127C33: chip::System::PacketBufferHandle::operator=(decltype(nullptr)) (SystemPacketBuffer.h:448)
==4071==    by 0x127BFB: chip::System::PacketBufferHandle::~PacketBufferHandle() (SystemPacketBuffer.h:425)
==4071==    by 0x139F2F: chip::DeviceLayer::Internal::UpdateAdditionalDataCharacteristic(_BluezGattCharacteristic1*) (Helper.cpp:1281)
==4071==    by 0x13A4FB: chip::DeviceLayer::Internal::BluezPeripheralObjectsSetup(void*) (Helper.cpp:1368)
==4071==    by 0x13A5FB: chip::DeviceLayer::Internal::BluezOnBusAcquired(_GDBusConnection*, char const*, void*) (Helper.cpp:1392)
==4071==    by 0x13A733: chip::DeviceLayer::Internal::BluezMainLoop(void*) (Helper.cpp:1427)
==4071==    by 0x486FF73: start_thread (pthread_create.c:463)
==4071==    by 0x523F3DB: thread_start (clone.S:78)
==4071==  Block was alloc'd at
==4071==    at 0x484BDDC: malloc (in /usr/lib/aarch64-linux-gnu/valgrind/vgpreload_memcheck-arm64-linux.so)
==4071==    by 0x12E997: chip::Platform::MemoryAlloc(unsigned long, bool) (CHIPMem-Malloc.cpp:112)
==4071==    by 0x12E96B: chip::Platform::MemoryAlloc(unsigned long) (CHIPMem-Malloc.cpp:106)
==4071==    by 0x14B5D7: chip::System::PacketBufferHandle::New(unsigned long, unsigned short) (SystemPacketBuffer.cpp:458)
==4071==    by 0x1458EF: chip::AdditionalDataPayloadGenerator::generateAdditionalDataPayload(unsigned short, char const*, unsigned long, chip::System::PacketBufferHandle&, chip::BitFlags<unsigned char, chip::AdditionalDataFields>) (AdditionalDataPayloadGenerator.cpp:56)
==4071==    by 0x139E43: chip::DeviceLayer::Internal::UpdateAdditionalDataCharacteristic(_BluezGattCharacteristic1*) (Helper.cpp:1297)
==4071==    by 0x13A4FB: chip::DeviceLayer::Internal::BluezPeripheralObjectsSetup(void*) (Helper.cpp:1368)
==4071==    by 0x13A5FB: chip::DeviceLayer::Internal::BluezOnBusAcquired(_GDBusConnection*, char const*, void*) (Helper.cpp:1392)
==4071==    by 0x13A733: chip::DeviceLayer::Internal::BluezMainLoop(void*) (Helper.cpp:1427)
==4071==    by 0x486FF73: start_thread (pthread_create.c:463)
==4071==    by 0x523F3DB: thread_start (clone.S:78)

We need to pass a pointer to the dup'd data in, not the original packet buffer.
@andy31415 andy31415 merged commit 536a33f into project-chip:master Feb 23, 2021
@mspang mspang deleted the for-chip/valgrind-additional branch April 2, 2021 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants