From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Ken Rockot Date: Mon, 30 Sep 2024 06:33:13 +0000 Subject: ipcz: Validate link state fragment before adoption (cherry picked from commit c333ed99544992f66e6e03621fa938d75ad01f70) Fixed: 368208152 Change-Id: I0e2ece4b0857b225d229134b2e55abc3e08348ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5876623 Commit-Queue: Ken Rockot Reviewed-by: Daniel Cheng Cr-Original-Commit-Position: refs/heads/main@{#1358968} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5893005 Bot-Commit: Rubber Stamper Auto-Submit: Ken Rockot Cr-Commit-Position: refs/branch-heads/6613@{#2136} Cr-Branched-From: 03c1799e6f9c7239802827eab5e935b9e14fceae-refs/heads/main@{#1331488} diff --git a/third_party/ipcz/src/ipcz/node_link.cc b/third_party/ipcz/src/ipcz/node_link.cc index 420377ac3d0c5a1f745e8c5d0030c21b5d3639f2..412cedc59eaabd79a35d7e299e42f720de9ac148 100644 --- a/third_party/ipcz/src/ipcz/node_link.cc +++ b/third_party/ipcz/src/ipcz/node_link.cc @@ -37,21 +37,6 @@ namespace ipcz { -namespace { - -template -FragmentRef MaybeAdoptFragmentRef(NodeLinkMemory& memory, - const FragmentDescriptor& descriptor) { - if (descriptor.is_null() || descriptor.size() < sizeof(T) || - descriptor.offset() % 8 != 0) { - return {}; - } - - return memory.AdoptFragmentRef(memory.GetFragment(descriptor)); -} - -} // namespace - // static Ref NodeLink::CreateActive(Ref node, LinkSide link_side, @@ -721,8 +706,8 @@ bool NodeLink::OnAcceptBypassLink(msg::AcceptBypassLink& accept) { return true; } - auto link_state = MaybeAdoptFragmentRef( - memory(), accept.v0()->new_link_state_fragment); + auto link_state = memory().AdoptFragmentRefIfValid( + accept.v0()->new_link_state_fragment); if (link_state.is_null()) { // Bypass links must always come with a valid fragment for their // RouterLinkState. If one has not been provided, that's a validation @@ -764,8 +749,8 @@ bool NodeLink::OnBypassPeerWithLink(msg::BypassPeerWithLink& bypass) { return true; } - auto link_state = MaybeAdoptFragmentRef( - memory(), bypass.v0()->new_link_state_fragment); + auto link_state = memory().AdoptFragmentRefIfValid( + bypass.v0()->new_link_state_fragment); if (link_state.is_null()) { return false; } diff --git a/third_party/ipcz/src/ipcz/node_link_memory.h b/third_party/ipcz/src/ipcz/node_link_memory.h index c5e3d6494580412abf71e75f0736ad5a82abe95c..f457757cb703d56b8dd7d152b10f3fe6f8f8c84e 100644 --- a/third_party/ipcz/src/ipcz/node_link_memory.h +++ b/third_party/ipcz/src/ipcz/node_link_memory.h @@ -98,14 +98,29 @@ class NodeLinkMemory : public RefCounted { // with the same BufferId and dimensions as `descriptor`. Fragment GetFragment(const FragmentDescriptor& descriptor); - // Adopts an existing reference to a RefCountedFragment within `fragment`. - // This does NOT increment the ref count of the RefCountedFragment. + // Adopts an existing reference to a RefCountedFragment within `fragment`, + // which must be a valid, properly aligned, and sufficiently sized fragment to + // hold a T. This does NOT increment the ref count of the RefCountedFragment. template FragmentRef AdoptFragmentRef(const Fragment& fragment) { ABSL_ASSERT(sizeof(T) <= fragment.size()); return FragmentRef(kAdoptExistingRef, WrapRefCounted(this), fragment); } + // Attempts to adopt an existing reference to a RefCountedFragment located at + // `fragment`. Returns null if the fragment descriptor is null, misaligned, + // or of insufficient size. This does NOT increment the ref count of the + // RefCountedFragment. + template + FragmentRef AdoptFragmentRefIfValid(const FragmentDescriptor& descriptor) { + if (descriptor.is_null() || descriptor.size() < sizeof(T) || + descriptor.offset() % 8 != 0) { + return {}; + } + + return AdoptFragmentRef(GetFragment(descriptor)); + } + // Adds a new buffer to the underlying BufferPool to use as additional // allocation capacity for blocks of size `block_size`. Note that the // contents of the mapped region must already be initialized as a diff --git a/third_party/ipcz/src/ipcz/node_link_memory_test.cc b/third_party/ipcz/src/ipcz/node_link_memory_test.cc index bcdd45ee866ec39d557ee1c762af04ae0af26b6a..fd51b78a2a4475b54f047310da4c91e40a61342e 100644 --- a/third_party/ipcz/src/ipcz/node_link_memory_test.cc +++ b/third_party/ipcz/src/ipcz/node_link_memory_test.cc @@ -306,5 +306,54 @@ TEST_F(NodeLinkMemoryTest, ParcelDataAllocation) { node_c->Close(); } +struct TestObject : public RefCountedFragment { + public: + int x; + int y; +}; + +TEST_F(NodeLinkMemoryTest, AdoptFragmentRefIfValid) { + auto object = memory_a().AdoptFragmentRef( + memory_a().AllocateFragment(sizeof(TestObject))); + object->x = 5; + object->y = 42; + + const FragmentDescriptor valid_descriptor(object.fragment().buffer_id(), + object.fragment().offset(), + sizeof(TestObject)); + + const FragmentDescriptor null_descriptor( + kInvalidBufferId, valid_descriptor.offset(), valid_descriptor.size()); + EXPECT_TRUE(memory_a() + .AdoptFragmentRefIfValid(null_descriptor) + .is_null()); + + const FragmentDescriptor empty_descriptor( + valid_descriptor.buffer_id(), valid_descriptor.offset(), /*size=*/0); + EXPECT_TRUE(memory_a() + .AdoptFragmentRefIfValid(empty_descriptor) + .is_null()); + + const FragmentDescriptor short_descriptor(valid_descriptor.buffer_id(), + valid_descriptor.offset(), + sizeof(TestObject) - 4); + EXPECT_TRUE(memory_a() + .AdoptFragmentRefIfValid(short_descriptor) + .is_null()); + + const FragmentDescriptor unaligned_descriptor(valid_descriptor.buffer_id(), + valid_descriptor.offset() + 2, + valid_descriptor.size() - 2); + EXPECT_TRUE(memory_a() + .AdoptFragmentRefIfValid(unaligned_descriptor) + .is_null()); + + const auto adopted_object = + memory_a().AdoptFragmentRefIfValid(valid_descriptor); + ASSERT_TRUE(adopted_object.is_addressable()); + EXPECT_EQ(5, adopted_object->x); + EXPECT_EQ(42, adopted_object->y); +} + } // namespace } // namespace ipcz diff --git a/third_party/ipcz/src/ipcz/router.cc b/third_party/ipcz/src/ipcz/router.cc index 79c443d942c6613ea8a52990b93c1811e2d3d166..b1dc593427ecae67b6758edd82257f88daefcde1 100644 --- a/third_party/ipcz/src/ipcz/router.cc +++ b/third_party/ipcz/src/ipcz/router.cc @@ -765,12 +765,16 @@ Ref Router::Deserialize(const RouterDescriptor& descriptor, ? descriptor.decaying_incoming_sequence_length : descriptor.next_incoming_sequence_number); + auto link_state = + from_node_link.memory().AdoptFragmentRefIfValid( + descriptor.new_link_state_fragment); + if (link_state.is_null()) { + // Central links require a valid link state fragment. + return nullptr; + } new_outward_link = from_node_link.AddRemoteRouterLink( - context, descriptor.new_sublink, - from_node_link.memory().AdoptFragmentRef( - from_node_link.memory().GetFragment( - descriptor.new_link_state_fragment)), - LinkType::kCentral, LinkSide::kB, router); + context, descriptor.new_sublink, std::move(link_state), LinkType::kCentral, + LinkSide::kB, router); if (!new_outward_link) { return nullptr; }