diff --git a/cmds/stagefright/stagefright.cpp b/cmds/stagefright/stagefright.cpp index bf36be09e3..cf9efe03d7 100644 --- a/cmds/stagefright/stagefright.cpp +++ b/cmds/stagefright/stagefright.cpp @@ -989,7 +989,7 @@ int main(int argc, char **argv) { failed = false; printf("getFrameAtTime(%s) => OK\n", filename); - VideoFrame *frame = (VideoFrame *)mem->pointer(); + VideoFrame *frame = (VideoFrame *)mem->unsecurePointer(); CHECK_EQ(writeJpegFile("/sdcard/out.jpg", frame->getFlattenedData(), diff --git a/cmds/stagefright/stream.cpp b/cmds/stagefright/stream.cpp index 35bdbc0978..59b5f9febf 100644 --- a/cmds/stagefright/stream.cpp +++ b/cmds/stagefright/stream.cpp @@ -116,7 +116,7 @@ void MyStreamSource::onBufferAvailable(size_t index) { sp mem = mBuffers.itemAt(index); - ssize_t n = read(mFd, mem->pointer(), mem->size()); + ssize_t n = read(mFd, mem->unsecurePointer(), mem->size()); if (n <= 0) { mListener->issueCommand(IStreamListener::EOS, false /* synchronous */); } else { @@ -238,7 +238,7 @@ ssize_t MyConvertingStreamSource::writeData(const void *data, size_t size) { copy = mem->size() - mCurrentBufferOffset; } - memcpy((uint8_t *)mem->pointer() + mCurrentBufferOffset, data, copy); + memcpy((uint8_t *)mem->unsecurePointer() + mCurrentBufferOffset, data, copy); mCurrentBufferOffset += copy; if (mCurrentBufferOffset == mem->size()) { diff --git a/drm/libmediadrm/CryptoHal.cpp b/drm/libmediadrm/CryptoHal.cpp index 954608f725..58110d41ad 100644 --- a/drm/libmediadrm/CryptoHal.cpp +++ b/drm/libmediadrm/CryptoHal.cpp @@ -321,7 +321,11 @@ status_t CryptoHal::toSharedBuffer(const sp& memory, int32_t seqNum, :: } // memory must be within the address space of the heap - if (memory->pointer() != static_cast(heap->getBase()) + memory->offset() || + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + if (memory->unsecurePointer() != static_cast(heap->getBase()) + memory->offset() || heap->getSize() < memory->offset() + memory->size() || SIZE_MAX - memory->offset() < memory->size()) { android_errorWriteLog(0x534e4554, "76221123"); diff --git a/media/codec2/sfplugin/Codec2Buffer.cpp b/media/codec2/sfplugin/Codec2Buffer.cpp index 5c8ad56b11..b339a922df 100644 --- a/media/codec2/sfplugin/Codec2Buffer.cpp +++ b/media/codec2/sfplugin/Codec2Buffer.cpp @@ -764,7 +764,11 @@ EncryptedLinearBlockBuffer::EncryptedLinearBlockBuffer( const std::shared_ptr &block, const sp &memory, int32_t heapSeqNum) - : Codec2Buffer(format, new ABuffer(memory->pointer(), memory->size())), + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + : Codec2Buffer(format, new ABuffer(memory->unsecurePointer(), memory->size())), mBlock(block), mMemory(memory), mHeapSeqNum(heapSeqNum) { @@ -800,7 +804,7 @@ bool EncryptedLinearBlockBuffer::copyDecryptedContent( if (view.size() < length) { return false; } - memcpy(view.data(), decrypted->pointer(), length); + memcpy(view.data(), decrypted->unsecurePointer(), length); return true; } diff --git a/media/libaudioclient/AudioEffect.cpp b/media/libaudioclient/AudioEffect.cpp index cf11936400..1cc5fe6610 100644 --- a/media/libaudioclient/AudioEffect.cpp +++ b/media/libaudioclient/AudioEffect.cpp @@ -159,7 +159,11 @@ status_t AudioEffect::set(const effect_uuid_t *type, mIEffect = iEffect; mCblkMemory = cblk; - mCblk = static_cast(cblk->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + mCblk = static_cast(cblk->unsecurePointer()); int bufOffset = ((sizeof(effect_param_cblk_t) - 1) / sizeof(int) + 1) * sizeof(int); mCblk->buffer = (uint8_t *)mCblk + bufOffset; diff --git a/media/libaudioclient/AudioRecord.cpp b/media/libaudioclient/AudioRecord.cpp index a1b04caa92..0f2d48ebd7 100644 --- a/media/libaudioclient/AudioRecord.cpp +++ b/media/libaudioclient/AudioRecord.cpp @@ -759,7 +759,11 @@ status_t AudioRecord::createRecord_l(const Modulo &epoch, const String status = NO_INIT; goto exit; } - iMemPointer = output.cblk ->pointer(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + iMemPointer = output.cblk ->unsecurePointer(); if (iMemPointer == NULL) { ALOGE("%s(%d): Could not get control block pointer", __func__, mPortId); status = NO_INIT; @@ -774,7 +778,11 @@ status_t AudioRecord::createRecord_l(const Modulo &epoch, const String if (output.buffers == 0) { buffers = cblk + 1; } else { - buffers = output.buffers->pointer(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + buffers = output.buffers->unsecurePointer(); if (buffers == NULL) { ALOGE("%s(%d): Could not get buffer pointer", __func__, mPortId); status = NO_INIT; diff --git a/media/libaudioclient/AudioTrack.cpp b/media/libaudioclient/AudioTrack.cpp index 4a80cd3baa..e8d7b6023e 100644 --- a/media/libaudioclient/AudioTrack.cpp +++ b/media/libaudioclient/AudioTrack.cpp @@ -406,7 +406,7 @@ status_t AudioTrack::set( mDoNotReconnect = doNotReconnect; ALOGV_IF(sharedBuffer != 0, "%s(): sharedBuffer: %p, size: %zu", - __func__, sharedBuffer->pointer(), sharedBuffer->size()); + __func__, sharedBuffer->unsecurePointer(), sharedBuffer->size()); ALOGV("%s(): streamType %d frameCount %zu flags %04x", __func__, streamType, frameCount, flags); @@ -1508,7 +1508,11 @@ status_t AudioTrack::createTrack_l() status = NO_INIT; goto exit; } - void *iMemPointer = iMem->pointer(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + void *iMemPointer = iMem->unsecurePointer(); if (iMemPointer == NULL) { ALOGE("%s(%d): Could not get control block pointer", __func__, mPortId); status = NO_INIT; @@ -1563,7 +1567,11 @@ status_t AudioTrack::createTrack_l() if (mSharedBuffer == 0) { buffers = cblk + 1; } else { - buffers = mSharedBuffer->pointer(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + buffers = mSharedBuffer->unsecurePointer(); if (buffers == NULL) { ALOGE("%s(%d): Could not get buffer pointer", __func__, mPortId); status = NO_INIT; diff --git a/media/libaudioclient/IAudioTrack.cpp b/media/libaudioclient/IAudioTrack.cpp index 83a568a554..6219e7a4ba 100644 --- a/media/libaudioclient/IAudioTrack.cpp +++ b/media/libaudioclient/IAudioTrack.cpp @@ -62,7 +62,7 @@ public: status_t status = remote()->transact(GET_CBLK, data, &reply); if (status == NO_ERROR) { cblk = interface_cast(reply.readStrongBinder()); - if (cblk != 0 && cblk->pointer() == NULL) { + if (cblk != 0 && cblk->unsecurePointer() == NULL) { cblk.clear(); } } diff --git a/media/libaudioclient/IEffect.cpp b/media/libaudioclient/IEffect.cpp index ce72dae600..5d47dff808 100644 --- a/media/libaudioclient/IEffect.cpp +++ b/media/libaudioclient/IEffect.cpp @@ -122,7 +122,7 @@ public: status_t status = remote()->transact(GET_CBLK, data, &reply); if (status == NO_ERROR) { cblk = interface_cast(reply.readStrongBinder()); - if (cblk != 0 && cblk->pointer() == NULL) { + if (cblk != 0 && cblk->unsecurePointer() == NULL) { cblk.clear(); } } diff --git a/media/libaudioclient/include/media/IAudioFlinger.h b/media/libaudioclient/include/media/IAudioFlinger.h index db09ddf0ae..b580a88be5 100644 --- a/media/libaudioclient/include/media/IAudioFlinger.h +++ b/media/libaudioclient/include/media/IAudioFlinger.h @@ -70,8 +70,12 @@ public: return DEAD_OBJECT; } if (parcel->readInt32() != 0) { + // TODO: Using unsecurePointer() has some associated security + // pitfalls (see declaration for details). + // Either document why it is safe in this case or address + // the issue (e.g. by copying). sharedBuffer = interface_cast(parcel->readStrongBinder()); - if (sharedBuffer == 0 || sharedBuffer->pointer() == NULL) { + if (sharedBuffer == 0 || sharedBuffer->unsecurePointer() == NULL) { return BAD_VALUE; } } @@ -269,13 +273,21 @@ public: (void)parcel->read(&inputId, sizeof(audio_io_handle_t)); if (parcel->readInt32() != 0) { cblk = interface_cast(parcel->readStrongBinder()); - if (cblk == 0 || cblk->pointer() == NULL) { + // TODO: Using unsecurePointer() has some associated security + // pitfalls (see declaration for details). + // Either document why it is safe in this case or address + // the issue (e.g. by copying). + if (cblk == 0 || cblk->unsecurePointer() == NULL) { return BAD_VALUE; } } if (parcel->readInt32() != 0) { buffers = interface_cast(parcel->readStrongBinder()); - if (buffers == 0 || buffers->pointer() == NULL) { + // TODO: Using unsecurePointer() has some associated security + // pitfalls (see declaration for details). + // Either document why it is safe in this case or address + // the issue (e.g. by copying). + if (buffers == 0 || buffers->unsecurePointer() == NULL) { return BAD_VALUE; } } diff --git a/media/libheif/HeifDecoderImpl.cpp b/media/libheif/HeifDecoderImpl.cpp index bad42104d6..33ea1cad72 100644 --- a/media/libheif/HeifDecoderImpl.cpp +++ b/media/libheif/HeifDecoderImpl.cpp @@ -173,7 +173,7 @@ ssize_t HeifDataSource::readAt(off64_t offset, size_t size) { // copy from cache if the request falls entirely in cache if (offset + size <= mCachedOffset + mCachedSize) { - memcpy(mMemory->pointer(), mCache.get() + offset - mCachedOffset, size); + memcpy(mMemory->unsecurePointer(), mCache.get() + offset - mCachedOffset, size); return size; } @@ -271,7 +271,7 @@ ssize_t HeifDataSource::readAt(off64_t offset, size_t size) { if (bytesAvailable < (int64_t)size) { size = bytesAvailable; } - memcpy(mMemory->pointer(), mCache.get() + offset - mCachedOffset, size); + memcpy(mMemory->unsecurePointer(), mCache.get() + offset - mCachedOffset, size); return size; } @@ -360,12 +360,16 @@ bool HeifDecoderImpl::init(HeifStream* stream, HeifFrameInfo* frameInfo) { sp sharedMem = mRetriever->getImageAtIndex( -1, mOutputColor, true /*metaOnly*/); - if (sharedMem == nullptr || sharedMem->pointer() == nullptr) { + if (sharedMem == nullptr || sharedMem->unsecurePointer() == nullptr) { ALOGE("init: videoFrame is a nullptr"); return false; } - VideoFrame* videoFrame = static_cast(sharedMem->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoFrame* videoFrame = static_cast(sharedMem->unsecurePointer()); ALOGV("Image dimension %dx%d, display %dx%d, angle %d, iccSize %d", videoFrame->mWidth, @@ -391,12 +395,17 @@ bool HeifDecoderImpl::init(HeifStream* stream, HeifFrameInfo* frameInfo) { MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC, mOutputColor, true /*metaOnly*/); - if (sharedMem == nullptr || sharedMem->pointer() == nullptr) { + if (sharedMem == nullptr || sharedMem->unsecurePointer() == nullptr) { ALOGE("init: videoFrame is a nullptr"); return false; } - VideoFrame* videoFrame = static_cast(sharedMem->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoFrame* videoFrame = static_cast( + sharedMem->unsecurePointer()); ALOGV("Sequence dimension %dx%d, display %dx%d, angle %d, iccSize %d", videoFrame->mWidth, @@ -487,7 +496,7 @@ bool HeifDecoderImpl::decodeAsync() { { Mutex::Autolock autolock(mLock); - if (frameMemory == nullptr || frameMemory->pointer() == nullptr) { + if (frameMemory == nullptr || frameMemory->unsecurePointer() == nullptr) { mAsyncDecodeDone = true; mScanlineReady.signal(); break; @@ -529,12 +538,16 @@ bool HeifDecoderImpl::decode(HeifFrameInfo* frameInfo) { sp frameMemory = mRetriever->getImageRectAtIndex( -1, mOutputColor, 0, 0, mImageInfo.mWidth, mSliceHeight); - if (frameMemory == nullptr || frameMemory->pointer() == nullptr) { + if (frameMemory == nullptr || frameMemory->unsecurePointer() == nullptr) { ALOGE("decode: metadata is a nullptr"); return false; } - VideoFrame* videoFrame = static_cast(frameMemory->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoFrame* videoFrame = static_cast(frameMemory->unsecurePointer()); if (frameInfo != nullptr) { initFrameInfo(frameInfo, videoFrame); @@ -563,12 +576,16 @@ bool HeifDecoderImpl::decode(HeifFrameInfo* frameInfo) { MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC, mOutputColor); } - if (mFrameMemory == nullptr || mFrameMemory->pointer() == nullptr) { + if (mFrameMemory == nullptr || mFrameMemory->unsecurePointer() == nullptr) { ALOGE("decode: videoFrame is a nullptr"); return false; } - VideoFrame* videoFrame = static_cast(mFrameMemory->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoFrame* videoFrame = static_cast(mFrameMemory->unsecurePointer()); if (videoFrame->mSize == 0 || mFrameMemory->size() < videoFrame->getFlattenedSize()) { ALOGE("decode: videoFrame size is invalid"); @@ -613,12 +630,16 @@ bool HeifDecoderImpl::decodeSequence(int frameIndex, HeifFrameInfo* frameInfo) { mTotalScanline = mSequenceInfo.mHeight; mFrameMemory = mRetriever->getFrameAtIndex(frameIndex, mOutputColor); - if (mFrameMemory == nullptr || mFrameMemory->pointer() == nullptr) { + if (mFrameMemory == nullptr || mFrameMemory->unsecurePointer() == nullptr) { ALOGE("decode: videoFrame is a nullptr"); return false; } - VideoFrame* videoFrame = static_cast(mFrameMemory->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoFrame* videoFrame = static_cast(mFrameMemory->unsecurePointer()); if (videoFrame->mSize == 0 || mFrameMemory->size() < videoFrame->getFlattenedSize()) { ALOGE("decode: videoFrame size is invalid"); @@ -641,10 +662,14 @@ bool HeifDecoderImpl::decodeSequence(int frameIndex, HeifFrameInfo* frameInfo) { } bool HeifDecoderImpl::getScanlineInner(uint8_t* dst) { - if (mFrameMemory == nullptr || mFrameMemory->pointer() == nullptr) { + if (mFrameMemory == nullptr || mFrameMemory->unsecurePointer() == nullptr) { return false; } - VideoFrame* videoFrame = static_cast(mFrameMemory->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoFrame* videoFrame = static_cast(mFrameMemory->unsecurePointer()); uint8_t* src = videoFrame->getFlattenedData() + videoFrame->mRowBytes * mCurScanline++; memcpy(dst, src, videoFrame->mBytesPerPixel * videoFrame->mWidth); return true; diff --git a/media/libmedia/IMediaHTTPConnection.cpp b/media/libmedia/IMediaHTTPConnection.cpp index 1bb8d67c0f..8cbb4c2cec 100644 --- a/media/libmedia/IMediaHTTPConnection.cpp +++ b/media/libmedia/IMediaHTTPConnection.cpp @@ -128,12 +128,12 @@ struct BpMediaHTTPConnection : public BpInterface { ALOGE("readAt got a NULL buffer"); return UNKNOWN_ERROR; } - if (mMemory->pointer() == NULL) { - ALOGE("readAt got a NULL mMemory->pointer()"); + if (mMemory->unsecurePointer() == NULL) { + ALOGE("readAt got a NULL mMemory->unsecurePointer()"); return UNKNOWN_ERROR; } - memcpy(buffer, mMemory->pointer(), len); + memcpy(buffer, mMemory->unsecurePointer(), len); return len; } diff --git a/media/libmediaplayerservice/MetadataRetrieverClient.cpp b/media/libmediaplayerservice/MetadataRetrieverClient.cpp index 4a3c65e1a4..b44eb43196 100644 --- a/media/libmediaplayerservice/MetadataRetrieverClient.cpp +++ b/media/libmediaplayerservice/MetadataRetrieverClient.cpp @@ -292,7 +292,7 @@ sp MetadataRetrieverClient::extractAlbumArt() delete albumArt; return NULL; } - MediaAlbumArt::init((MediaAlbumArt *) mAlbumArt->pointer(), + MediaAlbumArt::init((MediaAlbumArt *) mAlbumArt->unsecurePointer(), albumArt->size(), albumArt->data()); delete albumArt; // We've taken our copy. return mAlbumArt; diff --git a/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp b/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp index ee70306a35..b5142edff1 100644 --- a/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp +++ b/media/libmediaplayerservice/nuplayer/NuPlayerStreamListener.cpp @@ -154,7 +154,7 @@ ssize_t NuPlayer::NuPlayerStreamListener::read( } memcpy(data, - (const uint8_t *)mem->pointer() + (const uint8_t *)mem->unsecurePointer() + entry->mOffset, copy); diff --git a/media/libnblog/Reader.cpp b/media/libnblog/Reader.cpp index f556e3787c..67d028d705 100644 --- a/media/libnblog/Reader.cpp +++ b/media/libnblog/Reader.cpp @@ -45,7 +45,12 @@ Reader::Reader(const void *shared, size_t size, const std::string &name) } Reader::Reader(const sp& iMemory, size_t size, const std::string &name) - : Reader(iMemory != 0 ? (Shared *) iMemory->pointer() : NULL, size, name) + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + : Reader(iMemory != 0 ? (Shared *) iMemory->unsecurePointer() : NULL, size, + name) { mIMemory = iMemory; } @@ -156,7 +161,8 @@ std::unique_ptr Reader::getSnapshot(bool flush) bool Reader::isIMemory(const sp& iMemory) const { - return iMemory != 0 && mIMemory != 0 && iMemory->pointer() == mIMemory->pointer(); + return iMemory != 0 && mIMemory != 0 && + iMemory->unsecurePointer() == mIMemory->unsecurePointer(); } // We make a set of the invalid types rather than the valid types when aligning diff --git a/media/libnblog/Writer.cpp b/media/libnblog/Writer.cpp index da3bd52a50..86d3b9830f 100644 --- a/media/libnblog/Writer.cpp +++ b/media/libnblog/Writer.cpp @@ -56,7 +56,11 @@ Writer::Writer(void *shared, size_t size) } Writer::Writer(const sp& iMemory, size_t size) - : Writer(iMemory != 0 ? (Shared *) iMemory->pointer() : NULL, size) + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + : Writer(iMemory != 0 ? (Shared *) iMemory->unsecurePointer() : NULL, size) { mIMemory = iMemory; } diff --git a/media/libstagefright/ACodecBufferChannel.cpp b/media/libstagefright/ACodecBufferChannel.cpp index 266a240c71..dd6f7b42bd 100644 --- a/media/libstagefright/ACodecBufferChannel.cpp +++ b/media/libstagefright/ACodecBufferChannel.cpp @@ -153,7 +153,8 @@ status_t ACodecBufferChannel::queueSecureInputBuffer( } if (destination.mType == ICrypto::kDestinationTypeSharedMemory) { - memcpy(it->mCodecBuffer->base(), destination.mSharedMemory->pointer(), result); + memcpy(it->mCodecBuffer->base(), + destination.mSharedMemory->unsecurePointer(), result); } } else { // Here we cast CryptoPlugin::SubSample to hardware::cas::native::V1_0::SubSample @@ -219,7 +220,8 @@ status_t ACodecBufferChannel::queueSecureInputBuffer( if (dstBuffer.type == BufferType::SHARED_MEMORY) { memcpy(it->mCodecBuffer->base(), - (uint8_t*)it->mSharedEncryptedBuffer->pointer(), result); + (uint8_t*)it->mSharedEncryptedBuffer->unsecurePointer(), + result); } } diff --git a/media/libstagefright/BufferImpl.cpp b/media/libstagefright/BufferImpl.cpp index b760273455..68440e6abb 100644 --- a/media/libstagefright/BufferImpl.cpp +++ b/media/libstagefright/BufferImpl.cpp @@ -32,7 +32,11 @@ namespace android { // SharedMemoryBuffer SharedMemoryBuffer::SharedMemoryBuffer(const sp &format, const sp &mem) - : MediaCodecBuffer(format, new ABuffer(mem->pointer(), mem->size())), + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + : MediaCodecBuffer(format, new ABuffer(mem->unsecurePointer(), mem->size())), mMemory(mem) { } diff --git a/media/libstagefright/CallbackDataSource.cpp b/media/libstagefright/CallbackDataSource.cpp index 92e6eb9d5d..2f8e6af006 100644 --- a/media/libstagefright/CallbackDataSource.cpp +++ b/media/libstagefright/CallbackDataSource.cpp @@ -81,7 +81,8 @@ ssize_t CallbackDataSource::readAt(off64_t offset, void* data, size_t size) { return ERROR_OUT_OF_RANGE; } CHECK(numRead >= 0 && (size_t)numRead <= bufferSize); - memcpy(((uint8_t*)data) + totalNumRead, mMemory->pointer(), numRead); + memcpy(((uint8_t*)data) + totalNumRead, mMemory->unsecurePointer(), + numRead); numLeft -= numRead; totalNumRead += numRead; } diff --git a/media/libstagefright/CameraSource.cpp b/media/libstagefright/CameraSource.cpp index 41f5db04d6..9b3f4200d5 100644 --- a/media/libstagefright/CameraSource.cpp +++ b/media/libstagefright/CameraSource.cpp @@ -89,7 +89,7 @@ void CameraSourceListener::notify(int32_t msgType, int32_t ext1, int32_t ext2) { void CameraSourceListener::postData(int32_t msgType, const sp &dataPtr, camera_frame_metadata_t * /* metadata */) { ALOGV("postData(%d, ptr:%p, size:%zu)", - msgType, dataPtr->pointer(), dataPtr->size()); + msgType, dataPtr->unsecurePointer(), dataPtr->size()); sp source = mSource.promote(); if (source.get() != NULL) { @@ -966,8 +966,12 @@ void CameraSource::releaseRecordingFrame(const sp& frame) { // Check if frame contains a VideoNativeHandleMetadata. if (frame->size() == sizeof(VideoNativeHandleMetadata)) { - VideoNativeHandleMetadata *metadata = - (VideoNativeHandleMetadata*)(frame->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoNativeHandleMetadata *metadata = + (VideoNativeHandleMetadata*)(frame->unsecurePointer()); if (metadata->eType == kMetadataBufferTypeNativeHandleSource) { handle = metadata->pHandle; } @@ -1047,7 +1051,7 @@ void CameraSource::signalBufferReturned(MediaBufferBase *buffer) { Mutex::Autolock autoLock(mLock); for (List >::iterator it = mFramesBeingEncoded.begin(); it != mFramesBeingEncoded.end(); ++it) { - if ((*it)->pointer() == buffer->data()) { + if ((*it)->unsecurePointer() == buffer->data()) { releaseOneRecordingFrame((*it)); mFramesBeingEncoded.erase(it); ++mNumFramesEncoded; @@ -1102,7 +1106,11 @@ status_t CameraSource::read( frameTime = *mFrameTimes.begin(); mFrameTimes.erase(mFrameTimes.begin()); mFramesBeingEncoded.push_back(frame); - *buffer = new MediaBuffer(frame->pointer(), frame->size()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + *buffer = new MediaBuffer(frame->unsecurePointer(), frame->size()); (*buffer)->setObserver(this); (*buffer)->add_ref(); (*buffer)->meta_data().setInt64(kKeyTime, frameTime); @@ -1248,7 +1256,7 @@ void CameraSource::recordingFrameHandleCallbackTimestamp(int64_t timestampUs, mMemoryBases.erase(mMemoryBases.begin()); // Wrap native handle in sp so it can be pushed to mFramesReceived. - VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->pointer()); + VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->unsecurePointer()); metadata->eType = kMetadataBufferTypeNativeHandleSource; metadata->pHandle = handle; @@ -1296,7 +1304,11 @@ void CameraSource::recordingFrameHandleCallbackTimestampBatch( mMemoryBases.erase(mMemoryBases.begin()); // Wrap native handle in sp so it can be pushed to mFramesReceived. - VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(data->unsecurePointer()); metadata->eType = kMetadataBufferTypeNativeHandleSource; metadata->pHandle = handle; diff --git a/media/libstagefright/CameraSourceTimeLapse.cpp b/media/libstagefright/CameraSourceTimeLapse.cpp index 2a819ad0ae..e0a6eb3a57 100644 --- a/media/libstagefright/CameraSourceTimeLapse.cpp +++ b/media/libstagefright/CameraSourceTimeLapse.cpp @@ -245,11 +245,11 @@ sp CameraSourceTimeLapse::createIMemoryCopy( ALOGV("createIMemoryCopy"); size_t source_size = source_data->size(); - void* source_pointer = source_data->pointer(); + void* source_pointer = source_data->unsecurePointer(); sp newMemoryHeap = new MemoryHeapBase(source_size); sp newMemory = new MemoryBase(newMemoryHeap, 0, source_size); - memcpy(newMemory->pointer(), source_pointer, source_size); + memcpy(newMemory->unsecurePointer(), source_pointer, source_size); return newMemory; } diff --git a/media/libstagefright/FrameDecoder.cpp b/media/libstagefright/FrameDecoder.cpp index f99dd1c7ff..a5a07d6a8b 100644 --- a/media/libstagefright/FrameDecoder.cpp +++ b/media/libstagefright/FrameDecoder.cpp @@ -99,7 +99,7 @@ sp allocVideoFrame(const sp& trackMeta, ALOGE("not enough memory for VideoFrame size=%zu", size); return NULL; } - VideoFrame* frameCopy = static_cast(frameMem->pointer()); + VideoFrame* frameCopy = static_cast(frameMem->unsecurePointer()); frameCopy->init(frame, iccData, iccSize); return frameMem; @@ -206,7 +206,7 @@ sp FrameDecoder::getMetadataOnly( // try to fill sequence meta's duration based on average frame rate, // default to 33ms if frame rate is unavailable. int32_t frameRate; - VideoFrame* meta = static_cast(metaMem->pointer()); + VideoFrame* meta = static_cast(metaMem->unsecurePointer()); if (trackMeta->findInt32(kKeyFrameRate, &frameRate) && frameRate > 0) { meta->mDurationUs = 1000000LL / frameRate; } else { @@ -614,7 +614,7 @@ status_t VideoFrameDecoder::onOutputReceived( 0, dstBpp(), mSurfaceControl != nullptr /*allocRotated*/); - mFrame = static_cast(frameMem->pointer()); + mFrame = static_cast(frameMem->unsecurePointer()); setFrame(frameMem); } @@ -895,7 +895,7 @@ status_t ImageDecoder::onOutputReceived( if (mFrame == NULL) { sp frameMem = allocVideoFrame( trackMeta(), mWidth, mHeight, mTileWidth, mTileHeight, dstBpp()); - mFrame = static_cast(frameMem->pointer()); + mFrame = static_cast(frameMem->unsecurePointer()); setFrame(frameMem); } diff --git a/media/libstagefright/StagefrightMediaScanner.cpp b/media/libstagefright/StagefrightMediaScanner.cpp index cf4edaebff..ce73676580 100644 --- a/media/libstagefright/StagefrightMediaScanner.cpp +++ b/media/libstagefright/StagefrightMediaScanner.cpp @@ -158,7 +158,11 @@ MediaAlbumArt *StagefrightMediaScanner::extractAlbumArt(int fd) { if (mRetriever->setDataSource(fd, 0, size) == OK) { sp mem = mRetriever->extractAlbumArt(); if (mem != NULL) { - MediaAlbumArt *art = static_cast(mem->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + MediaAlbumArt *art = static_cast(mem->unsecurePointer()); return art->clone(); } } diff --git a/media/libstagefright/foundation/MediaBuffer.cpp b/media/libstagefright/foundation/MediaBuffer.cpp index 9beac05f47..8e245dc541 100644 --- a/media/libstagefright/foundation/MediaBuffer.cpp +++ b/media/libstagefright/foundation/MediaBuffer.cpp @@ -72,7 +72,7 @@ MediaBuffer::MediaBuffer(size_t size) } } else { getSharedControl()->clear(); - mData = (uint8_t *)mMemory->pointer() + sizeof(SharedControl); + mData = (uint8_t *)mMemory->unsecurePointer() + sizeof(SharedControl); ALOGV("Allocated shared mem buffer of size %zu @ %p", size, mData); } } diff --git a/media/libstagefright/foundation/MediaBufferGroup.cpp b/media/libstagefright/foundation/MediaBufferGroup.cpp index 84ff9a602d..3c250475f9 100644 --- a/media/libstagefright/foundation/MediaBufferGroup.cpp +++ b/media/libstagefright/foundation/MediaBufferGroup.cpp @@ -75,7 +75,7 @@ void MediaBufferGroup::init(size_t buffers, size_t buffer_size, size_t growthLim for (size_t i = 0; i < buffers; ++i) { sp mem = memoryDealer->allocate(augmented_size); - if (mem.get() == nullptr || mem->pointer() == nullptr) { + if (mem.get() == nullptr || mem->unsecurePointer() == nullptr) { ALOGW("Only allocated %zu shared buffers of size %zu", i, buffer_size); break; } diff --git a/media/libstagefright/include/media/stagefright/MediaBuffer.h b/media/libstagefright/include/media/stagefright/MediaBuffer.h index ace63ae9f8..9145b63136 100644 --- a/media/libstagefright/include/media/stagefright/MediaBuffer.h +++ b/media/libstagefright/include/media/stagefright/MediaBuffer.h @@ -48,7 +48,11 @@ public: explicit MediaBuffer(const sp &buffer); #ifndef NO_IMEMORY MediaBuffer(const sp &mem) : - MediaBuffer((uint8_t *)mem->pointer() + sizeof(SharedControl), mem->size()) { + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + MediaBuffer((uint8_t *)mem->unsecurePointer() + sizeof(SharedControl), mem->size()) { // delegate and override mMemory mMemory = mem; } @@ -94,9 +98,13 @@ public: virtual int remoteRefcount() const { #ifndef NO_IMEMORY - if (mMemory.get() == nullptr || mMemory->pointer() == nullptr) return 0; + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + if (mMemory.get() == nullptr || mMemory->unsecurePointer() == nullptr) return 0; int32_t remoteRefcount = - reinterpret_cast(mMemory->pointer())->getRemoteRefcount(); + reinterpret_cast(mMemory->unsecurePointer())->getRemoteRefcount(); // Sanity check so that remoteRefCount() is non-negative. return remoteRefcount >= 0 ? remoteRefcount : 0; // do not allow corrupted data. #else @@ -107,8 +115,12 @@ public: // returns old value int addRemoteRefcount(int32_t value) { #ifndef NO_IMEMORY - if (mMemory.get() == nullptr || mMemory->pointer() == nullptr) return 0; - return reinterpret_cast(mMemory->pointer())->addRemoteRefcount(value); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + if (mMemory.get() == nullptr || mMemory->unsecurePointer() == nullptr) return 0; + return reinterpret_cast(mMemory->unsecurePointer())->addRemoteRefcount(value); #else (void) value; return 0; @@ -121,8 +133,12 @@ public: static bool isDeadObject(const sp &memory) { #ifndef NO_IMEMORY - if (memory.get() == nullptr || memory->pointer() == nullptr) return false; - return reinterpret_cast(memory->pointer())->isDeadObject(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + if (memory.get() == nullptr || memory->unsecurePointer() == nullptr) return false; + return reinterpret_cast(memory->unsecurePointer())->isDeadObject(); #else (void) memory; return false; @@ -220,7 +236,11 @@ private: inline SharedControl *getSharedControl() const { #ifndef NO_IMEMORY - return reinterpret_cast(mMemory->pointer()); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + return reinterpret_cast(mMemory->unsecurePointer()); #else return nullptr; #endif diff --git a/media/libstagefright/include/media/stagefright/RemoteDataSource.h b/media/libstagefright/include/media/stagefright/RemoteDataSource.h index e191e6abbc..b63450553e 100644 --- a/media/libstagefright/include/media/stagefright/RemoteDataSource.h +++ b/media/libstagefright/include/media/stagefright/RemoteDataSource.h @@ -48,7 +48,7 @@ public: if (size > kBufferSize) { size = kBufferSize; } - return mSource->readAt(offset, mMemory->pointer(), size); + return mSource->readAt(offset, mMemory->unsecurePointer(), size); } virtual status_t getSize(off64_t *size) { return mSource->getSize(size); diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp index ddb4ba0bc3..e1c391661c 100644 --- a/media/libstagefright/omx/OMXNodeInstance.cpp +++ b/media/libstagefright/omx/OMXNodeInstance.cpp @@ -128,7 +128,7 @@ struct BufferMeta { } OMX_U8 *getPointer() { - return mMem.get() ? static_cast(mMem->pointer()) : + return mMem.get() ? static_cast(mMem->unsecurePointer()) : mHidlMemory.get() ? static_cast( static_cast(mHidlMemory->getPointer())) : nullptr; } @@ -1173,7 +1173,11 @@ status_t OMXNodeInstance::useBuffer_l( return BAD_VALUE; } if (params != NULL) { - paramsPointer = params->pointer(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + paramsPointer = params->unsecurePointer(); paramsSize = params->size(); } else if (hParams != NULL) { paramsPointer = hParams->getPointer(); diff --git a/media/utils/ServiceUtilities.cpp b/media/utils/ServiceUtilities.cpp index db13903560..7fe871c65b 100644 --- a/media/utils/ServiceUtilities.cpp +++ b/media/utils/ServiceUtilities.cpp @@ -222,9 +222,9 @@ status_t checkIMemory(const sp& iMemory) off_t size = lseek(heap->getHeapID(), 0, SEEK_END); lseek(heap->getHeapID(), 0, SEEK_SET); - if (iMemory->pointer() == NULL || size < (off_t)iMemory->size()) { + if (iMemory->unsecurePointer() == NULL || size < (off_t)iMemory->size()) { ALOGE("%s check failed: pointer %p size %zu fd size %u", - __FUNCTION__, iMemory->pointer(), iMemory->size(), (uint32_t)size); + __FUNCTION__, iMemory->unsecurePointer(), iMemory->size(), (uint32_t)size); return BAD_VALUE; } diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp index aef0ade992..27d785600b 100644 --- a/services/audioflinger/AudioFlinger.cpp +++ b/services/audioflinger/AudioFlinger.cpp @@ -652,7 +652,7 @@ sp AudioFlinger::newWriter_l(size_t size, const char *name) return new NBLog::Writer(); } success: - NBLog::Shared *sharedRawPtr = (NBLog::Shared *) shared->pointer(); + NBLog::Shared *sharedRawPtr = (NBLog::Shared *) shared->unsecurePointer(); new((void *) sharedRawPtr) NBLog::Shared(); // placement new here, but the corresponding // explicit destructor not needed since it is POD sMediaLogService->registerWriter(shared, size, name); diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp index 13152d0b84..d54ab4260f 100644 --- a/services/audioflinger/Effects.cpp +++ b/services/audioflinger/Effects.cpp @@ -1588,7 +1588,7 @@ AudioFlinger::EffectHandle::EffectHandle(const sp& effect, int bufOffset = ((sizeof(effect_param_cblk_t) - 1) / sizeof(int) + 1) * sizeof(int); mCblkMemory = client->heap()->allocate(EFFECT_PARAM_BUFFER_SIZE + bufOffset); if (mCblkMemory == 0 || - (mCblk = static_cast(mCblkMemory->pointer())) == NULL) { + (mCblk = static_cast(mCblkMemory->unsecurePointer())) == NULL) { ALOGE("not enough memory for Effect size=%zu", EFFECT_PARAM_BUFFER_SIZE + sizeof(effect_param_cblk_t)); mCblkMemory.clear(); diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index 6ca50a78b9..8fddc13509 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -2079,9 +2079,9 @@ sp AudioFlinger::PlaybackThread::createTrac // More than 2 channels does not require stronger alignment than stereo alignment <<= 1; } - if (((uintptr_t)sharedBuffer->pointer() & (alignment - 1)) != 0) { + if (((uintptr_t)sharedBuffer->unsecurePointer() & (alignment - 1)) != 0) { ALOGE("Invalid buffer alignment: address %p, channel count %u", - sharedBuffer->pointer(), channelCount); + sharedBuffer->unsecurePointer(), channelCount); lStatus = BAD_VALUE; goto Exit; } @@ -6756,7 +6756,7 @@ AudioFlinger::RecordThread::RecordThread(const sp& audioFlinger, sp pipeMemory; if ((roHeap == 0) || (pipeMemory = roHeap->allocate(pipeSize)) == 0 || - (pipeBuffer = pipeMemory->pointer()) == nullptr) { + (pipeBuffer = pipeMemory->unsecurePointer()) == nullptr) { ALOGE("not enough memory for pipe buffer size=%zu; " "roHeap=%p, pipeMemory=%p, pipeBuffer=%p; roHeapSize: %lld", pipeSize, roHeap.get(), pipeMemory.get(), pipeBuffer, diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp index fa35e5d2c1..6a999fce71 100644 --- a/services/audioflinger/Tracks.cpp +++ b/services/audioflinger/Tracks.cpp @@ -150,7 +150,7 @@ AudioFlinger::ThreadBase::TrackBase::TrackBase( if (client != 0) { mCblkMemory = client->heap()->allocate(size); if (mCblkMemory == 0 || - (mCblk = static_cast(mCblkMemory->pointer())) == NULL) { + (mCblk = static_cast(mCblkMemory->unsecurePointer())) == NULL) { ALOGE("%s(%d): not enough memory for AudioTrack size=%zu", __func__, mId, size); client->heap()->dump("AudioTrack"); mCblkMemory.clear(); @@ -172,7 +172,7 @@ AudioFlinger::ThreadBase::TrackBase::TrackBase( const sp roHeap(thread->readOnlyHeap()); if (roHeap == 0 || (mBufferMemory = roHeap->allocate(bufferSize)) == 0 || - (mBuffer = mBufferMemory->pointer()) == NULL) { + (mBuffer = mBufferMemory->unsecurePointer()) == NULL) { ALOGE("%s(%d): not enough memory for read-only buffer size=%zu", __func__, mId, bufferSize); if (roHeap != 0) { @@ -187,7 +187,7 @@ AudioFlinger::ThreadBase::TrackBase::TrackBase( case ALLOC_PIPE: mBufferMemory = thread->pipeMemory(); // mBuffer is the virtual address as seen from current process (mediaserver), - // and should normally be coming from mBufferMemory->pointer(). + // and should normally be coming from mBufferMemory->unsecurePointer(). // However in this case the TrackBase does not reference the buffer directly. // It should references the buffer via the pipe. // Therefore, to detect incorrect usage of the buffer, we set mBuffer to NULL. @@ -510,7 +510,11 @@ AudioFlinger::PlaybackThread::Track::Track( track_type type, audio_port_handle_t portId) : TrackBase(thread, client, attr, sampleRate, format, channelMask, frameCount, - (sharedBuffer != 0) ? sharedBuffer->pointer() : buffer, + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + (sharedBuffer != 0) ? sharedBuffer->unsecurePointer() : buffer, (sharedBuffer != 0) ? sharedBuffer->size() : bufferSize, sessionId, creatorPid, uid, true /*isOut*/, (type == TYPE_PATCH) ? ( buffer == NULL ? ALLOC_LOCAL : ALLOC_NONE) : ALLOC_CBLK, @@ -540,7 +544,7 @@ AudioFlinger::PlaybackThread::Track::Track( ALOG_ASSERT(!(client == 0 && sharedBuffer != 0)); ALOGV_IF(sharedBuffer != 0, "%s(%d): sharedBuffer: %p, size: %zu", - __func__, mId, sharedBuffer->pointer(), sharedBuffer->size()); + __func__, mId, sharedBuffer->unsecurePointer(), sharedBuffer->size()); if (mCblk == NULL) { return; diff --git a/services/camera/libcameraservice/api1/CameraClient.cpp b/services/camera/libcameraservice/api1/CameraClient.cpp index 764b3a94ff..388a5dc849 100644 --- a/services/camera/libcameraservice/api1/CameraClient.cpp +++ b/services/camera/libcameraservice/api1/CameraClient.cpp @@ -534,7 +534,7 @@ void CameraClient::releaseRecordingFrameHandle(native_handle_t *handle) { } if (mHardware != nullptr) { - VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(dataPtr->pointer()); + VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(dataPtr->unsecurePointer()); metadata->eType = kMetadataBufferTypeNativeHandleSource; metadata->pHandle = handle; mHardware->releaseRecordingFrame(dataPtr); @@ -573,7 +573,7 @@ void CameraClient::releaseRecordingFrameHandleBatch(const std::vectorpointer()); + VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(dataPtr->unsecurePointer()); metadata->eType = kMetadataBufferTypeNativeHandleSource; metadata->pHandle = handle; frames.push_back(dataPtr); @@ -916,8 +916,12 @@ void CameraClient::handleCallbackTimestampBatch( ALOGE("%s: dataPtr does not contain VideoNativeHandleMetadata!", __FUNCTION__); return; } + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). VideoNativeHandleMetadata *metadata = - (VideoNativeHandleMetadata*)(msg.dataPtr->pointer()); + (VideoNativeHandleMetadata*)(msg.dataPtr->unsecurePointer()); if (metadata->eType == kMetadataBufferTypeNativeHandleSource) { handle = metadata->pHandle; } @@ -1073,8 +1077,12 @@ void CameraClient::handleGenericDataTimestamp(nsecs_t timestamp, // Check if dataPtr contains a VideoNativeHandleMetadata. if (dataPtr->size() == sizeof(VideoNativeHandleMetadata)) { + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). VideoNativeHandleMetadata *metadata = - (VideoNativeHandleMetadata*)(dataPtr->pointer()); + (VideoNativeHandleMetadata*)(dataPtr->unsecurePointer()); if (metadata->eType == kMetadataBufferTypeNativeHandleSource) { handle = metadata->pHandle; } diff --git a/services/camera/libcameraservice/device1/CameraHardwareInterface.cpp b/services/camera/libcameraservice/device1/CameraHardwareInterface.cpp index 522d521b5b..62ef681668 100644 --- a/services/camera/libcameraservice/device1/CameraHardwareInterface.cpp +++ b/services/camera/libcameraservice/device1/CameraHardwareInterface.cpp @@ -165,8 +165,12 @@ hardware::Return CameraHardwareInterface::handleCallbackTimestamp( mem = mHidlMemPoolMap.at(data); } sp heapMem(static_cast(mem->handle)); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). VideoNativeHandleMetadata* md = (VideoNativeHandleMetadata*) - heapMem->mBuffers[bufferIndex]->pointer(); + heapMem->mBuffers[bufferIndex]->unsecurePointer(); md->pHandle = const_cast(frameData.getNativeHandle()); sDataCbTimestamp(timestamp, (int32_t) msgType, mem, bufferIndex, this); return hardware::Void(); @@ -192,8 +196,12 @@ hardware::Return CameraHardwareInterface::handleCallbackTimestampBatch( hidl_msg.bufferIndex, mem->mNumBufs); return hardware::Void(); } + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). VideoNativeHandleMetadata* md = (VideoNativeHandleMetadata*) - mem->mBuffers[hidl_msg.bufferIndex]->pointer(); + mem->mBuffers[hidl_msg.bufferIndex]->unsecurePointer(); md->pHandle = const_cast(hidl_msg.frameData.getNativeHandle()); msgs.push_back({hidl_msg.timestamp, mem->mBuffers[hidl_msg.bufferIndex]}); @@ -578,7 +586,11 @@ void CameraHardwareInterface::releaseRecordingFrame(const sp& mem) int bufferIndex = offset / size; if (CC_LIKELY(mHidlDevice != nullptr)) { if (size == sizeof(VideoNativeHandleMetadata)) { - VideoNativeHandleMetadata* md = (VideoNativeHandleMetadata*) mem->pointer(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoNativeHandleMetadata* md = (VideoNativeHandleMetadata*) mem->unsecurePointer(); // Caching the handle here because md->pHandle will be subject to HAL's edit native_handle_t* nh = md->pHandle; hidl_handle frame = nh; @@ -605,7 +617,11 @@ void CameraHardwareInterface::releaseRecordingFrameBatch(const std::vectorgetHeapID(); uint32_t bufferIndex = offset / size; - VideoNativeHandleMetadata* md = (VideoNativeHandleMetadata*) mem->pointer(); + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). + VideoNativeHandleMetadata* md = (VideoNativeHandleMetadata*) mem->unsecurePointer(); // Caching the handle here because md->pHandle will be subject to HAL's edit native_handle_t* nh = md->pHandle; VideoFrameMessage msg; diff --git a/services/soundtrigger/SoundTriggerHwService.cpp b/services/soundtrigger/SoundTriggerHwService.cpp index 377d30bcde..69e5f500cf 100644 --- a/services/soundtrigger/SoundTriggerHwService.cpp +++ b/services/soundtrigger/SoundTriggerHwService.cpp @@ -234,11 +234,11 @@ sp SoundTriggerHwService::prepareRecognitionEvent( size_t size = event->data_offset + event->data_size; eventMemory = mMemoryDealer->allocate(size); - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { eventMemory.clear(); return eventMemory; } - memcpy(eventMemory->pointer(), event, size); + memcpy(eventMemory->unsecurePointer(), event, size); return eventMemory; } @@ -283,11 +283,11 @@ sp SoundTriggerHwService::prepareSoundModelEvent(struct sound_trigger_m size_t size = event->data_offset + event->data_size; eventMemory = mMemoryDealer->allocate(size); - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { eventMemory.clear(); return eventMemory; } - memcpy(eventMemory->pointer(), event, size); + memcpy(eventMemory->unsecurePointer(), event, size); return eventMemory; } @@ -313,11 +313,11 @@ sp SoundTriggerHwService::prepareServiceStateEvent(sound_trigger_servic size_t size = sizeof(sound_trigger_service_state_t); eventMemory = mMemoryDealer->allocate(size); - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { eventMemory.clear(); return eventMemory; } - *((sound_trigger_service_state_t *)eventMemory->pointer()) = state; + *((sound_trigger_service_state_t *)eventMemory->unsecurePointer()) = state; return eventMemory; } @@ -557,8 +557,12 @@ status_t SoundTriggerHwService::Module::loadSoundModel(const sp& modelM return NO_INIT; } + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). struct sound_trigger_sound_model *sound_model = - (struct sound_trigger_sound_model *)modelMemory->pointer(); + (struct sound_trigger_sound_model *)modelMemory->unsecurePointer(); size_t structSize; if (sound_model->type == SOUND_MODEL_TYPE_KEYPHRASE) { @@ -651,8 +655,12 @@ status_t SoundTriggerHwService::Module::startRecognition(sound_model_handle_t ha return NO_INIT; } + // TODO: Using unsecurePointer() has some associated security pitfalls + // (see declaration for details). + // Either document why it is safe in this case or address the + // issue (e.g. by copying). struct sound_trigger_recognition_config *config = - (struct sound_trigger_recognition_config *)dataMemory->pointer(); + (struct sound_trigger_recognition_config *)dataMemory->unsecurePointer(); if (config->data_offset < sizeof(struct sound_trigger_recognition_config) || config->data_size > (UINT_MAX - config->data_offset) || @@ -734,9 +742,10 @@ void SoundTriggerHwService::Module::onCallbackEvent(const sp& eve { ALOGV("onCallbackEvent type %d", event->mType); + // Memory is coming from a trusted process. sp eventMemory = event->mMemory; - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { return; } if (mModuleClients.isEmpty()) { @@ -749,7 +758,7 @@ void SoundTriggerHwService::Module::onCallbackEvent(const sp& eve switch (event->mType) { case CallbackEvent::TYPE_RECOGNITION: { struct sound_trigger_recognition_event *recognitionEvent = - (struct sound_trigger_recognition_event *)eventMemory->pointer(); + (struct sound_trigger_recognition_event *)eventMemory->unsecurePointer(); { AutoMutex lock(mLock); sp model = getModel(recognitionEvent->model); @@ -769,7 +778,7 @@ void SoundTriggerHwService::Module::onCallbackEvent(const sp& eve } break; case CallbackEvent::TYPE_SOUNDMODEL: { struct sound_trigger_model_event *soundmodelEvent = - (struct sound_trigger_model_event *)eventMemory->pointer(); + (struct sound_trigger_model_event *)eventMemory->unsecurePointer(); { AutoMutex lock(mLock); sp model = getModel(soundmodelEvent->model); @@ -1082,7 +1091,8 @@ void SoundTriggerHwService::ModuleClient::onCallbackEvent(const sp eventMemory = event->mMemory; - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + // Memory is coming from a trusted process. + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { return; } diff --git a/soundtrigger/SoundTrigger.cpp b/soundtrigger/SoundTrigger.cpp index 9708ea726f..e297ee7c3e 100644 --- a/soundtrigger/SoundTrigger.cpp +++ b/soundtrigger/SoundTrigger.cpp @@ -204,39 +204,42 @@ status_t SoundTrigger::getModelState(sound_model_handle_t handle) void SoundTrigger::onRecognitionEvent(const sp& eventMemory) { Mutex::Autolock _l(mLock); - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { return; } if (mCallback != 0) { + // Memory is coming from a trusted process. mCallback->onRecognitionEvent( - (struct sound_trigger_recognition_event *)eventMemory->pointer()); + (struct sound_trigger_recognition_event *)eventMemory->unsecurePointer()); } } void SoundTrigger::onSoundModelEvent(const sp& eventMemory) { Mutex::Autolock _l(mLock); - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { return; } if (mCallback != 0) { + // Memory is coming from a trusted process. mCallback->onSoundModelEvent( - (struct sound_trigger_model_event *)eventMemory->pointer()); + (struct sound_trigger_model_event *)eventMemory->unsecurePointer()); } } void SoundTrigger::onServiceStateChange(const sp& eventMemory) { Mutex::Autolock _l(mLock); - if (eventMemory == 0 || eventMemory->pointer() == NULL) { + if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) { return; } if (mCallback != 0) { + // Memory is coming from a trusted process. mCallback->onServiceStateChange( - *((sound_trigger_service_state_t *)eventMemory->pointer())); + *((sound_trigger_service_state_t *)eventMemory->unsecurePointer())); } }