Snap for 10453563 from 90f8ec85746d01057064e7c670e23c1cf66e73fe to mainline-documentsui-release Change-Id: Ic3eb47095caacffc5b5c53ec0c5f88bec6cdba38
diff --git a/Android.bp b/Android.bp index f37f1c6..c42772b 100644 --- a/Android.bp +++ b/Android.bp
@@ -30,7 +30,7 @@ ], apex_available: [ "//apex_available:platform", - "com.android.bluetooth", + "com.android.btservices", "com.android.media.swcodec", "com.android.neuralnetworks", "test_com.android.media.swcodec",
diff --git a/OWNERS b/OWNERS index cc4d814..8b7f0e5 100644 --- a/OWNERS +++ b/OWNERS
@@ -1,3 +1,5 @@ +# Bug component: 655781 + smoreland@google.com elsk@google.com malchev@google.com
diff --git a/TEST_MAPPING b/TEST_MAPPING index 9f1aa6b..51c6c97 100644 --- a/TEST_MAPPING +++ b/TEST_MAPPING
@@ -7,7 +7,7 @@ "name": "fmq_test" } ], - "hwasan-postsubmit": [ + "hwasan-presubmit": [ { "name": "fmq_unit_tests" },
diff --git a/fuzzer/Android.bp b/fuzzer/Android.bp index 0926c09..72d99d4 100644 --- a/fuzzer/Android.bp +++ b/fuzzer/Android.bp
@@ -54,15 +54,4 @@ }, host_supported: true, - - sanitize: { - scs: true, - cfi: true, - address: true, - memtag_heap: true, - // undefined behavior is expected - all_undefined: false, - // integer overflow is expected - integer_overflow: false, - }, }
diff --git a/fuzzer/fmq_fuzzer.cpp b/fuzzer/fmq_fuzzer.cpp index 8c8a78e..1c92814 100644 --- a/fuzzer/fmq_fuzzer.cpp +++ b/fuzzer/fmq_fuzzer.cpp
@@ -36,8 +36,8 @@ typedef int32_t payload_t; -// The reader will wait for 10 ms -static constexpr int kBlockingTimeoutNs = 10000000; +// The reader/writers will wait during blocking calls +static constexpr int kBlockingTimeoutNs = 100000; /* * MessageQueueBase.h contains asserts when memory allocation fails. So we need @@ -45,6 +45,13 @@ */ static constexpr size_t kAlignment = 8; static constexpr size_t kMaxNumElements = PAGE_SIZE * 10 / sizeof(payload_t) - kAlignment + 1; +/* + * limit the custom grantor case to one page of memory. + * If we want to increase this, we need to make sure that all of grantors offset + * plus extent are less than the size of the page aligned ashmem region that is + * created + */ +static constexpr size_t kMaxCustomGrantorMemoryBytes = PAGE_SIZE; /* * The read counter can be found in the shared memory 16 bytes before the start @@ -72,8 +79,40 @@ typedef android::hardware::MQDescriptorSync<payload_t> MQDescSync; typedef android::hardware::MQDescriptorUnsync<payload_t> MQDescUnsync; -static inline uint64_t* getCounterPtr(payload_t* start, int byteOffset) { - return reinterpret_cast<uint64_t*>(reinterpret_cast<uint8_t*>(start) - byteOffset); +// AIDL and HIDL have different ways of accessing the grantors +template <typename Desc> +uint64_t* getCounterPtr(payload_t* start, const Desc& desc, int grantorIndx); + +uint64_t* createCounterPtr(payload_t* start, uint32_t offset, uint32_t data_offset) { + // start is the address of the beginning of the FMQ data section in memory + // offset is overall offset of the counter in the FMQ memory + // data_offset is the overall offset of the data section in the FMQ memory + // start - (data_offset) = beginning address of the FMQ memory + return reinterpret_cast<uint64_t*>(reinterpret_cast<uint8_t*>(start) - data_offset + offset); +} + +uint64_t* getCounterPtr(payload_t* start, const MQDescSync& desc, int grantorIndx) { + uint32_t offset = desc.grantors()[grantorIndx].offset; + uint32_t data_offset = desc.grantors()[android::hardware::details::DATAPTRPOS].offset; + return createCounterPtr(start, offset, data_offset); +} + +uint64_t* getCounterPtr(payload_t* start, const MQDescUnsync& desc, int grantorIndx) { + uint32_t offset = desc.grantors()[grantorIndx].offset; + uint32_t data_offset = desc.grantors()[android::hardware::details::DATAPTRPOS].offset; + return createCounterPtr(start, offset, data_offset); +} + +uint64_t* getCounterPtr(payload_t* start, const AidlMQDescSync& desc, int grantorIndx) { + uint32_t offset = desc.grantors[grantorIndx].offset; + uint32_t data_offset = desc.grantors[android::hardware::details::DATAPTRPOS].offset; + return createCounterPtr(start, offset, data_offset); +} + +uint64_t* getCounterPtr(payload_t* start, const AidlMQDescUnsync& desc, int grantorIndx) { + uint32_t offset = desc.grantors[grantorIndx].offset; + uint32_t data_offset = desc.grantors[android::hardware::details::DATAPTRPOS].offset; + return createCounterPtr(start, offset, data_offset); } template <typename Queue, typename Desc> @@ -84,7 +123,7 @@ return; } FuzzedDataProvider fdp(&readerData[0], readerData.size()); - payload_t* ring = nullptr; + payload_t* ring = reinterpret_cast<payload_t*>(readMq.getRingBufferPtr()); while (fdp.remaining_bytes()) { typename Queue::MemTransaction tx; size_t numElements = fdp.ConsumeIntegralInRange<size_t>(0, kMaxNumElements); @@ -97,11 +136,9 @@ // the ring buffer is only next to the read/write counters when there is // no user supplied fd if (!userFd) { - if (ring == nullptr) { - ring = firstStart; - } if (fdp.ConsumeIntegral<uint8_t>() == 1) { - uint64_t* writeCounter = getCounterPtr(ring, kWriteCounterOffsetBytes); + uint64_t* writeCounter = + getCounterPtr(ring, desc, android::hardware::details::WRITEPTRPOS); *writeCounter = fdp.ConsumeIntegral<uint64_t>(); } } @@ -124,7 +161,7 @@ FuzzedDataProvider fdp(&readerData[0], readerData.size()); do { size_t count = fdp.remaining_bytes() - ? fdp.ConsumeIntegralInRange<size_t>(1, readMq.getQuantumCount()) + ? fdp.ConsumeIntegralInRange<size_t>(0, readMq.getQuantumCount() + 1) : 1; std::vector<payload_t> data; data.resize(count); @@ -142,9 +179,9 @@ void readerBlocking<MessageQueueUnsync, MQDescUnsync>(const MQDescUnsync&, std::vector<uint8_t>&, std::atomic<size_t>&, std::atomic<size_t>&) {} -template <typename Queue> -void writer(Queue& writeMq, FuzzedDataProvider& fdp, bool userFd) { - payload_t* ring = nullptr; +template <typename Queue, typename Desc> +void writer(const Desc& desc, Queue& writeMq, FuzzedDataProvider& fdp, bool userFd) { + payload_t* ring = reinterpret_cast<payload_t*>(writeMq.getRingBufferPtr()); while (fdp.remaining_bytes()) { typename Queue::MemTransaction tx; size_t numElements = 1; @@ -159,11 +196,9 @@ // the ring buffer is only next to the read/write counters when there is // no user supplied fd if (!userFd) { - if (ring == nullptr) { - ring = firstStart; - } if (fdp.ConsumeIntegral<uint8_t>() == 1) { - uint64_t* readCounter = getCounterPtr(ring, kReadCounterOffsetBytes); + uint64_t* readCounter = + getCounterPtr(ring, desc, android::hardware::details::READPTRPOS); *readCounter = fdp.ConsumeIntegral<uint64_t>(); } } @@ -179,7 +214,7 @@ std::atomic<size_t>& readersNotFinished) { android::base::ScopeGuard guard([&writersNotFinished]() { writersNotFinished--; }); while (fdp.remaining_bytes() > sizeof(size_t) && readersNotFinished > 0) { - size_t count = fdp.ConsumeIntegralInRange<size_t>(1, writeMq.getQuantumCount()); + size_t count = fdp.ConsumeIntegralInRange<size_t>(0, writeMq.getQuantumCount() + 1); std::vector<payload_t> data; for (int i = 0; i < count; i++) { data.push_back(fdp.ConsumeIntegral<payload_t>()); @@ -197,74 +232,132 @@ std::atomic<size_t>&, std::atomic<size_t>&) {} template <typename Queue, typename Desc> -void fuzzAidlWithReaders(std::vector<uint8_t>& writerData, - std::vector<std::vector<uint8_t>>& readerData, bool blocking) { - FuzzedDataProvider fdp(&writerData[0], writerData.size()); - bool evFlag = blocking || fdp.ConsumeBool(); - android::base::unique_fd dataFd; - size_t bufferSize = 0; - size_t numElements = fdp.ConsumeIntegralInRange<size_t>(1, kMaxNumElements); - bool userFd = fdp.ConsumeBool(); - if (userFd) { - // run test with our own data region - bufferSize = numElements * sizeof(payload_t); - dataFd.reset(::ashmem_create_region("SyncReadWrite", bufferSize)); - } - Queue writeMq(numElements, evFlag, std::move(dataFd), bufferSize); - if (!writeMq.isValid()) { - LOG(ERROR) << "AIDL write mq invalid"; - return; - } - const auto desc = writeMq.dupeDesc(); - CHECK(desc.handle.fds[0].get() != -1); +inline std::optional<Desc> getDesc(std::unique_ptr<Queue>& queue, FuzzedDataProvider& fdp); - std::atomic<size_t> readersNotFinished = readerData.size(); - std::atomic<size_t> writersNotFinished = 1; - std::vector<std::thread> readers; - for (int i = 0; i < readerData.size(); i++) { - if (blocking) { - readers.emplace_back(readerBlocking<Queue, Desc>, std::ref(desc), - std::ref(readerData[i]), std::ref(readersNotFinished), - std::ref(writersNotFinished)); - +template <typename Queue, typename Desc> +inline std::optional<Desc> getAidlDesc(std::unique_ptr<Queue>& queue, FuzzedDataProvider& fdp) { + if (queue) { + // get the existing descriptor from the queue + Desc desc = queue->dupeDesc(); + if (desc.handle.fds[0].get() == -1) { + return std::nullopt; } else { - readers.emplace_back(reader<Queue, Desc>, std::ref(desc), std::ref(readerData[i]), - userFd); + return std::make_optional(std::move(desc)); } - } - - if (blocking) { - writerBlocking<Queue>(writeMq, fdp, writersNotFinished, readersNotFinished); } else { - writer<Queue>(writeMq, fdp, userFd); - } + // create a custom descriptor + std::vector<aidl::android::hardware::common::fmq::GrantorDescriptor> grantors; + size_t numGrantors = fdp.ConsumeIntegralInRange<size_t>(0, 4); + for (int i = 0; i < numGrantors; i++) { + grantors.push_back({fdp.ConsumeIntegralInRange<int32_t>(-2, 2) /* fdIndex */, + fdp.ConsumeIntegralInRange<int32_t>( + 0, kMaxCustomGrantorMemoryBytes) /* offset */, + fdp.ConsumeIntegralInRange<int64_t>( + 0, kMaxCustomGrantorMemoryBytes) /* extent */}); + // ashmem region is PAGE_SIZE and we need to make sure all of the + // pointers and data region fit inside + if (grantors.back().offset + grantors.back().extent > PAGE_SIZE) return std::nullopt; + } - for (auto& reader : readers) { - reader.join(); + android::base::unique_fd fd( + ashmem_create_region("AidlCustomGrantors", kMaxCustomGrantorMemoryBytes)); + ashmem_set_prot_region(fd, PROT_READ | PROT_WRITE); + aidl::android::hardware::common::NativeHandle handle; + handle.fds.emplace_back(fd.get()); + + return std::make_optional<Desc>( + {grantors, std::move(handle), sizeof(payload_t), fdp.ConsumeBool()}); } } +template <> +inline std::optional<AidlMQDescSync> getDesc(std::unique_ptr<AidlMessageQueueSync>& queue, + FuzzedDataProvider& fdp) { + return getAidlDesc<AidlMessageQueueSync, AidlMQDescSync>(queue, fdp); +} + +template <> +inline std::optional<AidlMQDescUnsync> getDesc(std::unique_ptr<AidlMessageQueueUnsync>& queue, + FuzzedDataProvider& fdp) { + return getAidlDesc<AidlMessageQueueUnsync, AidlMQDescUnsync>(queue, fdp); +} + template <typename Queue, typename Desc> -void fuzzHidlWithReaders(std::vector<uint8_t>& writerData, - std::vector<std::vector<uint8_t>>& readerData, bool blocking) { +inline std::optional<Desc> getHidlDesc(std::unique_ptr<Queue>& queue, FuzzedDataProvider& fdp) { + if (queue) { + auto desc = queue->getDesc(); + if (!desc->isHandleValid()) { + return std::nullopt; + } else { + return std::make_optional(std::move(*desc)); + } + } else { + // create a custom descriptor + std::vector<android::hardware::GrantorDescriptor> grantors; + size_t numGrantors = fdp.ConsumeIntegralInRange<size_t>(0, 4); + for (int i = 0; i < numGrantors; i++) { + grantors.push_back({fdp.ConsumeIntegral<uint32_t>() /* flags */, + fdp.ConsumeIntegralInRange<uint32_t>(0, 2) /* fdIndex */, + fdp.ConsumeIntegralInRange<uint32_t>( + 0, kMaxCustomGrantorMemoryBytes) /* offset */, + fdp.ConsumeIntegralInRange<uint64_t>( + 0, kMaxCustomGrantorMemoryBytes) /* extent */}); + // ashmem region is PAGE_SIZE and we need to make sure all of the + // pointers and data region fit inside + if (grantors.back().offset + grantors.back().extent > PAGE_SIZE) return std::nullopt; + } + + native_handle_t* handle = native_handle_create(1, 0); + int ashmemFd = ashmem_create_region("HidlCustomGrantors", kMaxCustomGrantorMemoryBytes); + ashmem_set_prot_region(ashmemFd, PROT_READ | PROT_WRITE); + handle->data[0] = ashmemFd; + + return std::make_optional<Desc>(grantors, handle, sizeof(payload_t)); + } +} + +template <> +inline std::optional<MQDescSync> getDesc(std::unique_ptr<MessageQueueSync>& queue, + FuzzedDataProvider& fdp) { + return getHidlDesc<MessageQueueSync, MQDescSync>(queue, fdp); +} + +template <> +inline std::optional<MQDescUnsync> getDesc(std::unique_ptr<MessageQueueUnsync>& queue, + FuzzedDataProvider& fdp) { + return getHidlDesc<MessageQueueUnsync, MQDescUnsync>(queue, fdp); +} + +template <typename Queue, typename Desc> +void fuzzWithReaders(std::vector<uint8_t>& writerData, + std::vector<std::vector<uint8_t>>& readerData, bool blocking) { FuzzedDataProvider fdp(&writerData[0], writerData.size()); bool evFlag = blocking || fdp.ConsumeBool(); - android::base::unique_fd dataFd; - size_t bufferSize = 0; size_t numElements = fdp.ConsumeIntegralInRange<size_t>(1, kMaxNumElements); + size_t bufferSize = numElements * sizeof(payload_t); bool userFd = fdp.ConsumeBool(); - if (userFd) { - // run test with our own data region - bufferSize = numElements * sizeof(payload_t); - dataFd.reset(::ashmem_create_region("SyncReadWrite", bufferSize)); + bool manualGrantors = fdp.ConsumeBool(); + std::unique_ptr<Queue> writeMq = nullptr; + if (manualGrantors) { + std::optional<Desc> customDesc(getDesc<Queue, Desc>(writeMq, fdp)); + if (customDesc) { + writeMq = std::make_unique<Queue>(*customDesc); + } + } else { + android::base::unique_fd dataFd; + if (userFd) { + // run test with our own data region + dataFd.reset(::ashmem_create_region("CustomData", bufferSize)); + } + writeMq = std::make_unique<Queue>(numElements, evFlag, std::move(dataFd), bufferSize); } - Queue writeMq(numElements, evFlag, std::move(dataFd), bufferSize); - if (!writeMq.isValid()) { - LOG(ERROR) << "HIDL write mq invalid"; + + if (writeMq == nullptr || !writeMq->isValid()) { return; } - const auto desc = writeMq.getDesc(); - CHECK(desc->isHandleValid()); + // get optional desc + const std::optional<Desc> desc(std::move(getDesc<Queue, Desc>(writeMq, fdp))); + CHECK(desc != std::nullopt); std::atomic<size_t> readersNotFinished = readerData.size(); std::atomic<size_t> writersNotFinished = 1; @@ -281,9 +374,9 @@ } if (blocking) { - writerBlocking<Queue>(writeMq, fdp, writersNotFinished, readersNotFinished); + writerBlocking<Queue>(*writeMq, fdp, writersNotFinished, readersNotFinished); } else { - writer<Queue>(writeMq, fdp, userFd); + writer<Queue>(*desc, *writeMq, fdp, userFd); } for (auto& reader : readers) { @@ -307,13 +400,11 @@ bool fuzzBlocking = fdp.ConsumeBool(); std::vector<uint8_t> writerData = fdp.ConsumeRemainingBytes<uint8_t>(); if (fuzzSync) { - fuzzHidlWithReaders<MessageQueueSync, MQDescSync>(writerData, readerData, fuzzBlocking); - fuzzAidlWithReaders<AidlMessageQueueSync, AidlMQDescSync>(writerData, readerData, - fuzzBlocking); + fuzzWithReaders<MessageQueueSync, MQDescSync>(writerData, readerData, fuzzBlocking); + fuzzWithReaders<AidlMessageQueueSync, AidlMQDescSync>(writerData, readerData, fuzzBlocking); } else { - fuzzHidlWithReaders<MessageQueueUnsync, MQDescUnsync>(writerData, readerData, false); - fuzzAidlWithReaders<AidlMessageQueueUnsync, AidlMQDescUnsync>(writerData, readerData, - false); + fuzzWithReaders<MessageQueueUnsync, MQDescUnsync>(writerData, readerData, false); + fuzzWithReaders<AidlMessageQueueUnsync, AidlMQDescUnsync>(writerData, readerData, false); } return 0;
diff --git a/include/fmq/AidlMQDescriptorShim.h b/include/fmq/AidlMQDescriptorShim.h index e3d3cb9..de175da 100644 --- a/include/fmq/AidlMQDescriptorShim.h +++ b/include/fmq/AidlMQDescriptorShim.h
@@ -230,7 +230,11 @@ template <typename T, MQFlavor flavor> size_t AidlMQDescriptorShim<T, flavor>::getSize() const { - return mGrantors[hardware::details::DATAPTRPOS].extent; + if (mGrantors.size() > hardware::details::DATAPTRPOS) { + return mGrantors[hardware::details::DATAPTRPOS].extent; + } else { + return 0; + } } template <typename T, MQFlavor flavor>
diff --git a/include/fmq/MessageQueueBase.h b/include/fmq/MessageQueueBase.h index c34a4ff..f99e335 100644 --- a/include/fmq/MessageQueueBase.h +++ b/include/fmq/MessageQueueBase.h
@@ -421,6 +421,11 @@ */ bool commitRead(size_t nMessages); + /** + * Get the pointer to the ring buffer. Useful for debugging and fuzzing. + */ + uint8_t* getRingBufferPtr() const { return mRing; } + private: size_t availableToWriteBytes() const; size_t availableToReadBytes() const; @@ -586,12 +591,6 @@ return; } - const auto& grantors = mDesc->grantors(); - for (const auto& grantor : grantors) { - hardware::details::check(hardware::details::isAlignedToWordBoundary(grantor.offset) == true, - "Grantor offsets need to be aligned"); - } - if (flavor == kSynchronizedReadWrite) { mReadPtr = reinterpret_cast<std::atomic<uint64_t>*>( mapGrantorDescr(hardware::details::READPTRPOS)); @@ -602,11 +601,11 @@ */ mReadPtr = new (std::nothrow) std::atomic<uint64_t>; } - hardware::details::check(mReadPtr != nullptr, "mReadPtr is null"); + if (mReadPtr == nullptr) goto error; mWritePtr = reinterpret_cast<std::atomic<uint64_t>*>( mapGrantorDescr(hardware::details::WRITEPTRPOS)); - hardware::details::check(mWritePtr != nullptr, "mWritePtr is null"); + if (mWritePtr == nullptr) goto error; if (resetPointers) { mReadPtr->store(0, std::memory_order_release); @@ -617,21 +616,40 @@ } mRing = reinterpret_cast<uint8_t*>(mapGrantorDescr(hardware::details::DATAPTRPOS)); - hardware::details::check(mRing != nullptr, "mRing is null"); + if (mRing == nullptr) goto error; if (mDesc->countGrantors() > hardware::details::EVFLAGWORDPOS) { mEvFlagWord = static_cast<std::atomic<uint32_t>*>( mapGrantorDescr(hardware::details::EVFLAGWORDPOS)); - hardware::details::check(mEvFlagWord != nullptr, "mEvFlagWord is null"); + if (mEvFlagWord == nullptr) goto error; android::hardware::EventFlag::createEventFlag(mEvFlagWord, &mEventFlag); } + return; +error: + if (mReadPtr) { + if (flavor == kSynchronizedReadWrite) { + unmapGrantorDescr(mReadPtr, hardware::details::READPTRPOS); + } else { + delete mReadPtr; + } + mReadPtr = nullptr; + } + if (mWritePtr) { + unmapGrantorDescr(mWritePtr, hardware::details::WRITEPTRPOS); + mWritePtr = nullptr; + } + if (mRing) { + unmapGrantorDescr(mRing, hardware::details::EVFLAGWORDPOS); + mRing = nullptr; + } } template <template <typename, MQFlavor> typename MQDescriptorType, typename T, MQFlavor flavor> MessageQueueBase<MQDescriptorType, T, flavor>::MessageQueueBase(const Descriptor& Desc, bool resetPointers) { mDesc = std::unique_ptr<Descriptor>(new (std::nothrow) Descriptor(Desc)); - if (mDesc == nullptr) { + if (mDesc == nullptr || mDesc->getSize() == 0) { + hardware::details::logError("MQDescriptor is invalid or queue size is 0."); return; } @@ -650,6 +668,10 @@ ". Number of elements: " + std::to_string(numElementsInQueue)); return; } + if (numElementsInQueue == 0) { + hardware::details::logError("Requested queue size of 0."); + return; + } if (bufferFd != -1 && numElementsInQueue * sizeof(T) > bufferSize) { hardware::details::logError("The supplied buffer size(" + std::to_string(bufferSize) + ") is smaller than the required size(" + @@ -755,10 +777,10 @@ template <template <typename, MQFlavor> typename MQDescriptorType, typename T, MQFlavor flavor> MessageQueueBase<MQDescriptorType, T, flavor>::~MessageQueueBase() { - if (flavor == kUnsynchronizedWrite && mReadPtr != nullptr) { - delete mReadPtr; - } else if (mReadPtr != nullptr) { + if (flavor == kSynchronizedReadWrite && mReadPtr != nullptr) { unmapGrantorDescr(mReadPtr, hardware::details::READPTRPOS); + } else if (mReadPtr != nullptr) { + delete mReadPtr; } if (mWritePtr != nullptr) { unmapGrantorDescr(mWritePtr, hardware::details::WRITEPTRPOS); @@ -1234,7 +1256,7 @@ template <template <typename, MQFlavor> typename MQDescriptorType, typename T, MQFlavor flavor> void* MessageQueueBase<MQDescriptorType, T, flavor>::mapGrantorDescr(uint32_t grantorIdx) { const native_handle_t* handle = mDesc->handle(); - auto grantors = mDesc->grantors(); + const std::vector<android::hardware::GrantorDescriptor> grantors = mDesc->grantors(); if (handle == nullptr) { hardware::details::logError("mDesc->handle is null"); return nullptr; @@ -1247,10 +1269,55 @@ } int fdIndex = grantors[grantorIdx].fdIndex; + if (fdIndex < 0 || fdIndex >= handle->numFds) { + hardware::details::logError( + std::string("fdIndex (" + std::to_string(fdIndex) + ") from grantor (index " + + std::to_string(grantorIdx) + + ") must be smaller than the number of fds in the handle: " + + std::to_string(handle->numFds))); + return nullptr; + } + /* * Offset for mmap must be a multiple of PAGE_SIZE. */ + if (!hardware::details::isAlignedToWordBoundary(grantors[grantorIdx].offset)) { + hardware::details::logError("Grantor (index " + std::to_string(grantorIdx) + + ") offset needs to be aligned to word boundary but is: " + + std::to_string(grantors[grantorIdx].offset)); + return nullptr; + } + + /* + * Expect some grantors to be at least a min size + */ + for (uint32_t i = 0; i < grantors.size(); i++) { + switch (i) { + case hardware::details::READPTRPOS: + if (grantors[i].extent < sizeof(uint64_t)) return nullptr; + break; + case hardware::details::WRITEPTRPOS: + if (grantors[i].extent < sizeof(uint64_t)) return nullptr; + break; + case hardware::details::DATAPTRPOS: + // We don't expect specific data size + break; + case hardware::details::EVFLAGWORDPOS: + if (grantors[i].extent < sizeof(uint32_t)) return nullptr; + break; + default: + // We don't care about unknown grantors + break; + } + } + int mapOffset = (grantors[grantorIdx].offset / PAGE_SIZE) * PAGE_SIZE; + if (grantors[grantorIdx].extent < 0 || grantors[grantorIdx].extent > INT_MAX - PAGE_SIZE) { + hardware::details::logError(std::string("Grantor (index " + std::to_string(grantorIdx) + + ") extent value is too large or negative: " + + std::to_string(grantors[grantorIdx].extent))); + return nullptr; + } int mapLength = grantors[grantorIdx].offset - mapOffset + grantors[grantorIdx].extent; void* address = mmap(0, mapLength, PROT_READ | PROT_WRITE, MAP_SHARED, handle->data[fdIndex],
diff --git a/tests/Android.bp b/tests/Android.bp index 40148b1..55eb1c7 100644 --- a/tests/Android.bp +++ b/tests/Android.bp
@@ -37,7 +37,8 @@ name: "fmq_test_client", tidy_timeout_srcs: ["msgq_test_client.cpp"], srcs: ["msgq_test_client.cpp"], - + // This cc_test is used through the python test and won't support isolated + isolated: false, cflags: [ "-Wall", "-Werror",
diff --git a/tests/fmq_unit_tests.cpp b/tests/fmq_unit_tests.cpp index d3fdfbc..03db6ec 100644 --- a/tests/fmq_unit_tests.cpp +++ b/tests/fmq_unit_tests.cpp
@@ -230,6 +230,7 @@ class BadQueueConfig : public TestBase<T> {}; class AidlOnlyBadQueueConfig : public ::testing::Test {}; +class HidlOnlyBadQueueConfig : public ::testing::Test {}; class Hidl2AidlOperation : public ::testing::Test {}; class DoubleFdFailures : public ::testing::Test {}; @@ -301,13 +302,76 @@ TYPED_TEST(BadQueueConfig, QueueSizeTooLarge) { size_t numElementsInQueue = SIZE_MAX / sizeof(uint16_t) + 1; - typename TypeParam::MQType* fmq = - new (std::nothrow) typename TypeParam::MQType(numElementsInQueue); - ASSERT_NE(nullptr, fmq); + typename TypeParam::MQType fmq(numElementsInQueue); /* * Should fail due to size being too large to fit into size_t. */ - ASSERT_FALSE(fmq->isValid()); + ASSERT_FALSE(fmq.isValid()); +} + +// {flags, fdIndex, offset, extent} +static const std::vector<android::hardware::GrantorDescriptor> kGrantors = { + {0, 0, 0, 4096}, + {0, 0, 0, 4096}, + {0, 0, 0, 4096}, +}; + +// Make sure this passes without invalid index/extent for the next two test +// cases +TEST_F(HidlOnlyBadQueueConfig, SanityCheck) { + std::vector<android::hardware::GrantorDescriptor> grantors = kGrantors; + + native_handle_t* handle = native_handle_create(1, 0); + int ashmemFd = ashmem_create_region("QueueHidlOnlyBad", 4096); + ashmem_set_prot_region(ashmemFd, PROT_READ | PROT_WRITE); + handle->data[0] = ashmemFd; + + android::hardware::MQDescriptor<uint16_t, kSynchronizedReadWrite> desc(grantors, handle, + sizeof(uint16_t)); + android::hardware::MessageQueue<uint16_t, kSynchronizedReadWrite> fmq(desc); + EXPECT_TRUE(fmq.isValid()); + + close(ashmemFd); +} + +TEST_F(HidlOnlyBadQueueConfig, BadFdIndex) { + std::vector<android::hardware::GrantorDescriptor> grantors = kGrantors; + grantors[0].fdIndex = 5; + + native_handle_t* handle = native_handle_create(1, 0); + int ashmemFd = ashmem_create_region("QueueHidlOnlyBad", 4096); + ashmem_set_prot_region(ashmemFd, PROT_READ | PROT_WRITE); + handle->data[0] = ashmemFd; + + android::hardware::MQDescriptor<uint16_t, kSynchronizedReadWrite> desc(grantors, handle, + sizeof(uint16_t)); + android::hardware::MessageQueue<uint16_t, kSynchronizedReadWrite> fmq(desc); + /* + * Should fail due fdIndex being out of range of the native_handle. + */ + EXPECT_FALSE(fmq.isValid()); + + close(ashmemFd); +} + +TEST_F(HidlOnlyBadQueueConfig, ExtentTooLarge) { + std::vector<android::hardware::GrantorDescriptor> grantors = kGrantors; + grantors[0].extent = 0xfffff041; + + native_handle_t* handle = native_handle_create(1, 0); + int ashmemFd = ashmem_create_region("QueueHidlOnlyBad", 4096); + ashmem_set_prot_region(ashmemFd, PROT_READ | PROT_WRITE); + handle->data[0] = ashmemFd; + + android::hardware::MQDescriptor<uint16_t, kSynchronizedReadWrite> desc(grantors, handle, + sizeof(uint16_t)); + android::hardware::MessageQueue<uint16_t, kSynchronizedReadWrite> fmq(desc); + /* + * Should fail due to extent being too large. + */ + EXPECT_FALSE(fmq.isValid()); + + close(ashmemFd); } // If this test fails and we do leak FDs, the next test will cause a crash @@ -329,10 +393,9 @@ size_t numElementsInQueue = 64; // Create HIDL side and get MQDescriptor - MessageQueueSync8* fmq = new (std::nothrow) MessageQueueSync8(numElementsInQueue); - ASSERT_NE(nullptr, fmq); - ASSERT_TRUE(fmq->isValid()); - const HidlMQDescSync8* hidlDesc = fmq->getDesc(); + MessageQueueSync8 fmq(numElementsInQueue); + ASSERT_TRUE(fmq.isValid()); + const HidlMQDescSync8* hidlDesc = fmq.getDesc(); ASSERT_NE(nullptr, hidlDesc); // Create AIDL MQDescriptor to send to another process based off the HIDL MQDescriptor @@ -341,17 +404,16 @@ &aidlDesc); // Other process will create the other side of the queue using the AIDL MQDescriptor - AidlMessageQueueSync8* aidlFmq = new (std::nothrow) AidlMessageQueueSync8(aidlDesc); - ASSERT_NE(nullptr, aidlFmq); - ASSERT_TRUE(aidlFmq->isValid()); + AidlMessageQueueSync8 aidlFmq(aidlDesc); + ASSERT_TRUE(aidlFmq.isValid()); // Make sure a write to the HIDL side, will show up for the AIDL side constexpr size_t dataLen = 4; uint8_t data[dataLen] = {12, 11, 10, 9}; - fmq->write(data, dataLen); + fmq.write(data, dataLen); int8_t readData[dataLen]; - ASSERT_TRUE(aidlFmq->read(readData, dataLen)); + ASSERT_TRUE(aidlFmq.read(readData, dataLen)); ASSERT_EQ(data[0], readData[0]); ASSERT_EQ(data[1], readData[1]); @@ -363,10 +425,9 @@ size_t numElementsInQueue = 64; // Create HIDL side and get MQDescriptor - MessageQueueUnsync8* fmq = new (std::nothrow) MessageQueueUnsync8(numElementsInQueue); - ASSERT_NE(nullptr, fmq); - ASSERT_TRUE(fmq->isValid()); - const HidlMQDescUnsync8* hidlDesc = fmq->getDesc(); + MessageQueueUnsync8 fmq(numElementsInQueue); + ASSERT_TRUE(fmq.isValid()); + const HidlMQDescUnsync8* hidlDesc = fmq.getDesc(); ASSERT_NE(nullptr, hidlDesc); // Create AIDL MQDescriptor to send to another process based off the HIDL MQDescriptor @@ -375,24 +436,22 @@ &aidlDesc); // Other process will create the other side of the queue using the AIDL MQDescriptor - AidlMessageQueueUnsync8* aidlFmq = new (std::nothrow) AidlMessageQueueUnsync8(aidlDesc); - ASSERT_NE(nullptr, aidlFmq); - ASSERT_TRUE(aidlFmq->isValid()); + AidlMessageQueueUnsync8 aidlFmq(aidlDesc); + ASSERT_TRUE(aidlFmq.isValid()); // Can have multiple readers with unsync flavor - AidlMessageQueueUnsync8* aidlFmq2 = new (std::nothrow) AidlMessageQueueUnsync8(aidlDesc); - ASSERT_NE(nullptr, aidlFmq2); - ASSERT_TRUE(aidlFmq2->isValid()); + AidlMessageQueueUnsync8 aidlFmq2(aidlDesc); + ASSERT_TRUE(aidlFmq2.isValid()); // Make sure a write to the HIDL side, will show up for the AIDL side constexpr size_t dataLen = 4; uint8_t data[dataLen] = {12, 11, 10, 9}; - fmq->write(data, dataLen); + fmq.write(data, dataLen); int8_t readData[dataLen]; - ASSERT_TRUE(aidlFmq->read(readData, dataLen)); + ASSERT_TRUE(aidlFmq.read(readData, dataLen)); int8_t readData2[dataLen]; - ASSERT_TRUE(aidlFmq2->read(readData2, dataLen)); + ASSERT_TRUE(aidlFmq2.read(readData2, dataLen)); ASSERT_EQ(data[0], readData[0]); ASSERT_EQ(data[1], readData[1]); @@ -417,12 +476,12 @@ grantors[1] = {0, 1 /* fdIndex */, 16, 16}; grantors[2] = {0, 1 /* fdIndex */, 16, 16}; - HidlMQDescUnsync8* hidlDesc = new (std::nothrow) HidlMQDescUnsync8(grantors, mqHandle, 10); - ASSERT_TRUE(hidlDesc->isHandleValid()); + HidlMQDescUnsync8 hidlDesc(grantors, mqHandle, 10); + ASSERT_TRUE(hidlDesc.isHandleValid()); AidlMQDescUnsync8 aidlDesc; bool ret = android::unsafeHidlToAidlMQDescriptor<uint8_t, int8_t, UnsynchronizedWrite>( - *hidlDesc, &aidlDesc); + hidlDesc, &aidlDesc); ASSERT_TRUE(ret); } @@ -439,16 +498,14 @@ grantors[1] = {0, 1 /* fdIndex */, 16, 16}; grantors[2] = {0, 0 /* fdIndex */, 16, 16}; - HidlMQDescUnsync8* hidlDesc = new (std::nothrow) HidlMQDescUnsync8(grantors, mqHandle, 10); - ASSERT_TRUE(hidlDesc->isHandleValid()); + HidlMQDescUnsync8 hidlDesc(grantors, mqHandle, 10); + ASSERT_TRUE(hidlDesc.isHandleValid()); AidlMQDescUnsync8 aidlDesc; bool ret = android::unsafeHidlToAidlMQDescriptor<uint8_t, int8_t, UnsynchronizedWrite>( - *hidlDesc, &aidlDesc); + hidlDesc, &aidlDesc); ASSERT_TRUE(ret); EXPECT_EQ(aidlDesc.handle.fds.size(), 2); - native_handle_close(mqHandle); - native_handle_delete(mqHandle); } // TODO(b/165674950) Since AIDL does not support unsigned integers, it can only support @@ -456,23 +513,21 @@ // lifted. Until then, check against SSIZE_MAX instead of SIZE_MAX. TEST_F(AidlOnlyBadQueueConfig, QueueSizeTooLargeForAidl) { size_t numElementsInQueue = SSIZE_MAX / sizeof(uint16_t) + 1; - AidlMessageQueueSync16* fmq = new (std::nothrow) AidlMessageQueueSync16(numElementsInQueue); - ASSERT_NE(nullptr, fmq); + AidlMessageQueueSync16 fmq(numElementsInQueue); /* * Should fail due to size being too large to fit into size_t. */ - ASSERT_FALSE(fmq->isValid()); + ASSERT_FALSE(fmq.isValid()); } TEST_F(AidlOnlyBadQueueConfig, NegativeAidlDescriptor) { aidl::android::hardware::common::fmq::MQDescriptor<uint16_t, SynchronizedReadWrite> desc; desc.quantum = -10; - AidlMessageQueueSync16* fmq = new (std::nothrow) AidlMessageQueueSync16(desc); - ASSERT_NE(nullptr, fmq); + AidlMessageQueueSync16 fmq(desc); /* * Should fail due to quantum being negative. */ - ASSERT_FALSE(fmq->isValid()); + ASSERT_FALSE(fmq.isValid()); } TEST_F(AidlOnlyBadQueueConfig, NegativeAidlDescriptorGrantor) { @@ -481,12 +536,11 @@ desc.flags = 0; desc.grantors.push_back( aidl::android::hardware::common::fmq::GrantorDescriptor{.offset = 12, .extent = -10}); - AidlMessageQueueSync16* fmq = new (std::nothrow) AidlMessageQueueSync16(desc); - ASSERT_NE(nullptr, fmq); + AidlMessageQueueSync16 fmq(desc); /* * Should fail due to grantor having negative extent. */ - ASSERT_FALSE(fmq->isValid()); + ASSERT_FALSE(fmq.isValid()); } /* @@ -516,8 +570,8 @@ */ TEST_F(DoubleFdFailures, InvalidFd) { android::base::SetLogger(android::base::StdioLogger); - EXPECT_DEATH_IF_SUPPORTED(AidlMessageQueueSync(64, false, android::base::unique_fd(3000), 64), - "Check failed: exp mRing is null"); + auto queue = AidlMessageQueueSync(64, false, android::base::unique_fd(3000), 64); + EXPECT_FALSE(queue.isValid()); } /*