Fix potential UB in ipcz and Mojo

Various message sizes and field layouts were not properly aligned,
introducing potential UB when serializing or deserializing internal
ipcz messages or MojoIpcz driver objects.

This change corrects such issues and adds some new guards against
future misalignment.

This also adds some validation to ensure that extensible structs
(including the MessageHeader itself) encode a properly aligned
aligned struct size when received over an ipcz transport.

A few instances of memcpy() called with a nullptr (albeit with
zero size) have been corrected too, since UBSan complains about
that.

RouterDescriptor and FragmentDescriptor can now be properly
classified as trivially copyable so that UBSan doesn't complain
about memcpying them.

Finally, this corrects a minor encoding mistake in MojoIpcz'
SharedBuffer header, where the struct size field was used to convey
the buffer size, and the buffer size field was unused.

Fixed: 1412962
Change-Id: I3e7da73a313d0c758e55429f6e49fe5285278b6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4265194
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107234}

mojo/core/ipcz_driver/data_pipe.cc

@@ -40,6 +40,7 @@ struct IPCZ_ALIGN(8) DataPipeHeader {
 
   RingBuffer::SerializedState ring_buffer_state;
 };
+static_assert(sizeof(DataPipeHeader) == 24, "Invalid DataPipeHeader size");
 
 // Attempts to put a single (32-bit) integer into the given `portal`. Returns
 // true if successful, or false to indicate that the peer portal is closed.
@@ -521,7 +522,7 @@ scoped_refptr<DataPipe> DataPipe::Deserialize(
 
   const auto& header = *reinterpret_cast<const DataPipeHeader*>(data.data());
   const size_t header_size = header.size;
-  if (header_size < sizeof(header)) {
+  if (header_size < sizeof(header) || header_size % 8 != 0) {
     return nullptr;
   }
 

mojo/core/ipcz_driver/ring_buffer.h

@@ -152,6 +152,7 @@ class MOJO_SYSTEM_IMPL_EXPORT RingBuffer {
     uint32_t offset = 0;
     uint32_t size = 0;
   };
+  static_assert(sizeof(SerializedState) == 8, "Invalid SerializedState size");
   void Serialize(SerializedState& state);
   bool Deserialize(const SerializedState& state);
 

mojo/core/ipcz_driver/shared_buffer.cc

@@ -36,11 +36,15 @@ struct IPCZ_ALIGN(8) BufferHeader {
   // Access mode for the region.
   BufferMode mode;
 
+  // Explicit padding for the next field to be 8-byte-aligned.
+  uint32_t padding;
+
   // The low and high components of the 128-bit GUID used to identify this
   // buffer.
   uint64_t guid_low;
   uint64_t guid_high;
 };
+static_assert(sizeof(BufferHeader) == 32, "Invalid BufferHeader size");
 
 // Produces a ScopedPlatformSharedMemoryHandle from a set of PlatformHandles and
 // an access mode.
@@ -190,7 +194,9 @@ bool SharedBuffer::Serialize(Transport& transmitter,
 
   DCHECK_GE(data.size(), sizeof(BufferHeader));
   BufferHeader& header = *reinterpret_cast<BufferHeader*>(data.data());
-  header.size = static_cast<uint32_t>(region_.GetSize());
+  header.size = sizeof(header);
+  header.buffer_size = static_cast<uint32_t>(region_.GetSize());
+  header.padding = 0;
   switch (region_.GetMode()) {
     case base::subtle::PlatformSharedMemoryRegion::Mode::kReadOnly:
       header.mode = BufferMode::kReadOnly;
@@ -235,6 +241,11 @@ scoped_refptr<SharedBuffer> SharedBuffer::Deserialize(
 
   const BufferHeader& header =
       *reinterpret_cast<const BufferHeader*>(data.data());
+  const size_t header_size = header.size;
+  if (header_size < sizeof(BufferHeader) || header_size % 8 != 0) {
+    return nullptr;
+  }
+
   base::subtle::PlatformSharedMemoryRegion::Mode mode;
   switch (header.mode) {
     case BufferMode::kReadOnly:
@@ -258,7 +269,7 @@ scoped_refptr<SharedBuffer> SharedBuffer::Deserialize(
 
   auto handle = CreateRegionHandleFromPlatformHandles(handles, mode);
   auto region = base::subtle::PlatformSharedMemoryRegion::Take(
-      std::move(handle), mode, header.size, guid.value());
+      std::move(handle), mode, header.buffer_size, guid.value());
   if (!region.IsValid()) {
     return nullptr;
   }

mojo/core/ipcz_driver/wrapped_platform_handle.cc

@@ -60,6 +60,8 @@ struct IPCZ_ALIGN(8) WrappedPlatformHandleHeader {
   // Indicates what specific type of handle is wrapped.
   WrapperType type;
 };
+static_assert(sizeof(WrappedPlatformHandleHeader) == 8,
+              "Invalid WrappedPlatformHandleHeader size");
 
 #if BUILDFLAG(IS_FUCHSIA)
 PlatformHandle MakeFDTransmissible(base::ScopedFD fd) {
@@ -169,7 +171,8 @@ scoped_refptr<WrappedPlatformHandle> WrappedPlatformHandle::Deserialize(
 
   const auto& header =
       *reinterpret_cast<const WrappedPlatformHandleHeader*>(data.data());
-  if (header.size < sizeof(header)) {
+  const size_t header_size = header.size;
+  if (header_size < sizeof(header) || header_size % 8 != 0) {
     return nullptr;
   }
 

third_party/ipcz/src/BUILD.gn

@@ -261,7 +261,6 @@ ipcz_source_set("impl") {
     "ipcz/driver_object.cc",
     "ipcz/driver_transport.cc",
     "ipcz/fragment.cc",
-    "ipcz/fragment_descriptor.cc",
     "ipcz/fragment_ref.cc",
     "ipcz/handle_type.h",
     "ipcz/link_side.cc",
@@ -291,7 +290,6 @@ ipcz_source_set("impl") {
     "ipcz/remote_router_link.cc",
     "ipcz/route_edge.cc",
     "ipcz/router.cc",
-    "ipcz/router_descriptor.cc",
     "ipcz/router_descriptor.h",
     "ipcz/router_link_state.cc",
     "ipcz/test_messages.cc",

third_party/ipcz/src/ipcz/fragment_descriptor.h

@@ -23,9 +23,6 @@ struct IPCZ_ALIGN(8) FragmentDescriptor {
   // fragments.
   constexpr FragmentDescriptor() = default;
 
-  FragmentDescriptor(const FragmentDescriptor&);
-  FragmentDescriptor& operator=(const FragmentDescriptor&);
-
   // Constructs a descriptor for a span of memory `size` bytes long, starting
   // at byte `offset` within the buffer identified by `buffer_id` within some
   // BufferPool.
@@ -53,6 +50,8 @@ struct IPCZ_ALIGN(8) FragmentDescriptor {
   // The size of this fragment in bytes.
   uint32_t size_ = 0;
 };
+static_assert(std::is_trivially_copyable_v<FragmentDescriptor>,
+              "FragmentDescriptor must be trivially copyable");
 
 }  // namespace ipcz
 

third_party/ipcz/src/ipcz/message.cc

@@ -13,6 +13,7 @@
 #include "ipcz/driver_object.h"
 #include "ipcz/driver_transport.h"
 #include "ipcz/ipcz.h"
+#include "third_party/abseil-cpp/absl/base/macros.h"
 #include "third_party/abseil-cpp/absl/container/inlined_vector.h"
 #include "third_party/abseil-cpp/absl/types/span.h"
 #include "util/safe_math.h"
@@ -133,6 +134,10 @@ DriverObject DeserializeDriverObject(
                       object_data.num_driver_handles));
 }
 
+bool IsAligned(size_t n) {
+  return n % 8 == 0;
+}
+
 }  // namespace
 
 Message::ReceivedDataBuffer::ReceivedDataBuffer() = default;
@@ -164,6 +169,8 @@ Message::Message(uint8_t message_id, size_t params_size)
   h.version = 0;
   h.message_id = message_id;
   h.driver_object_data_array = 0;
+
+  ABSL_ASSERT(IsAligned(inlined_data_->size()));
 }
 
 Message::~Message() = default;
@@ -335,8 +342,8 @@ bool Message::CopyDataAndValidateHeader(absl::Span<const uint8_t> data) {
   }
 
   // The header's stated size (and thus the start of the parameter payload)
-  // must not run over the edge of the message.
-  if (header.size > data_.size()) {
+  // must not run over the edge of the message and must be 8-byte-aligned.
+  if (header.size > data_.size() || !IsAligned(header.size)) {
     return false;
   }
 
@@ -366,6 +373,11 @@ bool Message::ValidateParameters(
     return false;
   }
 
+  // Parameter struct sizes must be 8-byte-aligned.
+  if (!IsAligned(params_header.size)) {
+    return false;
+  }
+
   // NOTE: In Chromium, a vast majority of IPC messages have 0, 1, or 2 OS
   // handles attached. Since these objects are small, we inline some storage on
   // the stack to avoid some heap allocation in the most common cases.

third_party/ipcz/src/ipcz/message_macros/message_declaration_macros.h

@@ -14,6 +14,7 @@
   class name : public MessageWithParams<name##_Params> {         \
    public:                                                       \
     using ParamsType = name##_Params;                            \
+    static_assert(sizeof(ParamsType) % 8 == 0, "Invalid size");  \
     id_decl;                                                     \
     version_decl;                                                \
     name();                                                      \

third_party/ipcz/src/ipcz/node.cc

@@ -451,6 +451,7 @@ bool Node::RelayMessage(const NodeName& from_node, msg::RelayMessage& relay) {
   msg::AcceptRelayedMessage accept;
   accept.params().source = from_node;
   accept.params().data = accept.AllocateArray<uint8_t>(data.size());
+  accept.params().padding = 0;
   memcpy(accept.GetArrayData(accept.params().data), data.data(), data.size());
   accept.params().driver_objects =
       accept.AppendDriverObjects(relay.driver_objects());

third_party/ipcz/src/ipcz/node_connector.cc

@@ -62,6 +62,7 @@ class NodeConnectorForBrokerToNonBroker : public NodeConnector {
         checked_cast<uint32_t>(num_portals());
     connect.params().buffer = connect.AppendDriverObject(
         link_memory_allocation_.memory.TakeDriverObject());
+    connect.params().padding = 0;
     return IPCZ_RESULT_OK == transport_->Transmit(connect);
   }
 
@@ -430,6 +431,7 @@ class NodeConnectorForBrokerToBroker : public NodeConnector {
         checked_cast<uint32_t>(num_portals());
     connect.params().buffer = connect.AppendDriverObject(
         link_memory_allocation_.memory.TakeDriverObject());
+    connect.params().padding = 0;
     return IPCZ_RESULT_OK == transport_->Transmit(connect);
   }
 

third_party/ipcz/src/ipcz/node_link.cc

@@ -42,7 +42,8 @@ namespace {
 template <typename T>
 FragmentRef<T> MaybeAdoptFragmentRef(NodeLinkMemory& memory,
                                      const FragmentDescriptor& descriptor) {
-  if (descriptor.is_null() || descriptor.size() < sizeof(T)) {
+  if (descriptor.is_null() || descriptor.size() < sizeof(T) ||
+      descriptor.offset() % 8 != 0) {
     return {};
   }
 
@@ -203,6 +204,7 @@ void NodeLink::AcceptIntroduction(const NodeName& name,
   accept.params().name = name;
   accept.params().link_side = side;
   accept.params().remote_node_type = remote_node_type;
+  accept.params().padding = 0;
   accept.params().remote_protocol_version = remote_protocol_version;
   accept.params().transport =
       accept.AppendDriverObject(transport->TakeDriverObject());
@@ -270,6 +272,7 @@ void NodeLink::RequestMemory(size_t size, RequestMemoryCallback callback) {
 
   msg::RequestMemory request;
   request.params().size = size32;
+  request.params().padding = 0;
   Transmit(request);
 }
 
@@ -280,6 +283,7 @@ void NodeLink::RelayMessage(const NodeName& to_node, Message& message) {
   relay.params().destination = to_node;
   relay.params().data =
       relay.AllocateArray<uint8_t>(message.data_view().size());
+  relay.params().padding = 0;
   memcpy(relay.GetArrayData(relay.params().data), message.data_view().data(),
          message.data_view().size());
   relay.params().driver_objects =

third_party/ipcz/src/ipcz/node_messages_generator.h

@@ -36,6 +36,9 @@ IPCZ_MSG_BEGIN(ConnectFromBrokerToNonBroker,
   // A driver memory object to serve as the new NodeLink's primary shared
   // buffer. That is, BufferId 0 within its NodeLinkMemory's BufferPool.
   IPCZ_MSG_PARAM_DRIVER_OBJECT(buffer)
+
+  // Explicit padding to preserve 8-byte size alignemnt.
+  IPCZ_MSG_PARAM(uint32_t, padding)
 IPCZ_MSG_END()
 
 // Initial greeting sent by a non-broker node when ConnectNode() is invoked with
@@ -204,6 +207,9 @@ IPCZ_MSG_BEGIN(ConnectFromBrokerToBroker, IPCZ_MSG_ID(7), IPCZ_MSG_VERSION(0))
   // determined by whichever broker's name is the lesser of the two when
   // compared numerically.
   IPCZ_MSG_PARAM_DRIVER_OBJECT(buffer)
+
+  // Explicit padding to preserve 8-byte size alignment.
+  IPCZ_MSG_PARAM(uint32_t, padding)
 IPCZ_MSG_END()
 
 // Sent to a broker to ask for an introduction to one of the non-broker nodes in
@@ -234,6 +240,9 @@ IPCZ_MSG_BEGIN(AcceptIntroduction, IPCZ_MSG_ID(11), IPCZ_MSG_VERSION(0))
   // Indicates the type of the remote node being introduced.
   IPCZ_MSG_PARAM(NodeType, remote_node_type)
 
+  // Explicit padding to preserve 4-byte alignment of the following field.
+  IPCZ_MSG_PARAM(uint16_t, padding)
+
   // Indicates the highest ipcz protocol version which the remote side of
   // `transport` able and willing to use according to the broker.
   IPCZ_MSG_PARAM(uint32_t, remote_protocol_version)
@@ -310,15 +319,15 @@ IPCZ_MSG_BEGIN(AcceptParcel, IPCZ_MSG_ID(20), IPCZ_MSG_VERSION(0))
   // its main containing Parcel.
   IPCZ_MSG_PARAM(uint32_t, num_subparcels)
 
-  // Free-form array of application-provided data bytes for this parcel. This
-  // field is only meaningful if `parcel_fragment` is null.
-  IPCZ_MSG_PARAM_ARRAY(uint8_t, parcel_data)
-
   // An optional shared memory fragment containing this parcel's data. If this
   // is null, the parcel data is instead inlined via the `parcel_data` array
   // above.
   IPCZ_MSG_PARAM(FragmentDescriptor, parcel_fragment)
 
+  // Free-form array of application-provided data bytes for this parcel. This
+  // field is only meaningful if `parcel_fragment` is null.
+  IPCZ_MSG_PARAM_ARRAY(uint8_t, parcel_data)
+
   // Array of handle types, with each corresponding to a single IpczHandle
   // attached to the parcel.
   IPCZ_MSG_PARAM_ARRAY(HandleType, handle_types)
@@ -328,6 +337,9 @@ IPCZ_MSG_BEGIN(AcceptParcel, IPCZ_MSG_ID(20), IPCZ_MSG_VERSION(0))
   // to extend the transmitted portal's route there.
   IPCZ_MSG_PARAM_ARRAY(RouterDescriptor, new_routers)
 
+  // Explicit padding to preserve 8-byte alignment.
+  IPCZ_MSG_PARAM(uint32_t, padding)
+
   // Every DriverObject boxed and attached to this parcel has an entry in this
   // array.
   IPCZ_MSG_PARAM_DRIVER_OBJECT_ARRAY(driver_objects)
@@ -570,6 +582,7 @@ IPCZ_MSG_END()
 // providing the IPCZ_CONNECT_NODE_TO_ALLOCATION_DELEGATE flag to ConnectNode().
 IPCZ_MSG_BEGIN(RequestMemory, IPCZ_MSG_ID(64), IPCZ_MSG_VERSION(0))
   IPCZ_MSG_PARAM(uint32_t, size)
+  IPCZ_MSG_PARAM(uint32_t, padding)
 IPCZ_MSG_END()
 
 // Provides a new shared buffer to the receiver, owned exclusively by the
@@ -590,6 +603,9 @@ IPCZ_MSG_BEGIN(RelayMessage, IPCZ_MSG_ID(66), IPCZ_MSG_VERSION(0))
   // The actual serialized message to be relayed, including its own header.
   IPCZ_MSG_PARAM_ARRAY(uint8_t, data)
 
+  // Padding to preserve 8-byte alignment of the `driver_objects` field below.
+  IPCZ_MSG_PARAM(uint32_t, padding)
+
   // The set of driver objects to be relayed along with `data`.
   IPCZ_MSG_PARAM_DRIVER_OBJECT_ARRAY(driver_objects)
 IPCZ_MSG_END()
@@ -603,6 +619,9 @@ IPCZ_MSG_BEGIN(AcceptRelayedMessage, IPCZ_MSG_ID(67), IPCZ_MSG_VERSION(0))
   // The full serialized data of the relayed message.
   IPCZ_MSG_PARAM_ARRAY(uint8_t, data)
 
+  // Padding to preserve 8-byte alignment of the `driver_objects` field below.
+  IPCZ_MSG_PARAM(uint32_t, padding)
+
   // The set of driver objects relayed along with `data`.
   IPCZ_MSG_PARAM_DRIVER_OBJECT_ARRAY(driver_objects)
 IPCZ_MSG_END()

third_party/ipcz/src/ipcz/parcel.cc

@@ -87,7 +87,8 @@ void Parcel::AllocateData(size_t num_bytes,
 
 bool Parcel::AdoptDataFragment(Ref<NodeLinkMemory> memory,
                                const Fragment& fragment) {
-  if (!fragment.is_addressable() || fragment.size() <= sizeof(FragmentHeader)) {
+  if (!fragment.is_addressable() || fragment.size() <= sizeof(FragmentHeader) ||
+      fragment.offset() % 8 != 0) {
     return false;
   }
 

third_party/ipcz/src/ipcz/portal.cc

@@ -99,7 +99,9 @@ IpczResult Portal::Put(absl::Span<const uint8_t> data,
     return allocate_result;
   }
 
-  memcpy(parcel.data_view().data(), data.data(), data.size());
+  if (!data.empty()) {
+    memcpy(parcel.data_view().data(), data.data(), data.size());
+  }
   parcel.CommitData(data.size());
   parcel.SetObjects(std::move(objects));
   const IpczResult result = router_->SendOutboundParcel(parcel);

third_party/ipcz/src/ipcz/remote_router_link.cc

@@ -146,6 +146,7 @@ void RemoteRouterLink::AcceptParcel(const OperationContext& context,
   msg::AcceptParcel accept;
   accept.params().sublink = sublink_;
   accept.params().sequence_number = parcel.sequence_number();
+  accept.params().padding = 0;
 
   size_t num_portals = 0;
   absl::InlinedVector<DriverObject, 2> driver_objects;
@@ -332,8 +333,10 @@ void RemoteRouterLink::AcceptParcel(const OperationContext& context,
 
   // Copy all the serialized router descriptors into the message. Our local
   // copy will supply inputs for BeginProxyingToNewRouter() calls below.
-  memcpy(new_routers.data(), descriptors.data(),
-         new_routers.size() * sizeof(new_routers[0]));
+  if (!descriptors.empty()) {
+    memcpy(new_routers.data(), descriptors.data(),
+           new_routers.size() * sizeof(new_routers[0]));
+  }
 
   if (must_split_parcel) {
     msg::AcceptParcelDriverObjects accept_objects;

third_party/ipcz/src/ipcz/router.cc

@@ -429,7 +429,9 @@ IpczResult Router::GetNextInboundParcel(IpczGetFlags flags,
       const bool ok = inbound_parcels_.Pop(consumed_parcel);
       ABSL_ASSERT(ok);
     } else {
-      memcpy(data, p.data_view().data(), data_size);
+      if (data_size > 0) {
+        memcpy(data, p.data_view().data(), data_size);
+      }
       if (consuming_whole_parcel) {
         const bool ok = inbound_parcels_.Pop(consumed_parcel);
         ABSL_ASSERT(ok);