From 7dd39723b34c8378fc5eabb4d4ccacf32f55ffe2 Mon Sep 17 00:00:00 2001 From: Ytai Ben-Tsvi Date: Thu, 5 Sep 2019 15:14:30 -0700 Subject: [PATCH] Improve visibility of IMemory security risks This change renames the IMemory raw pointer accessors to unsecure*() to make it apparent to coders and code reviewers that the returned buffer may potentially be shared with untrusted processes, who may, after the fact, attempt to read and/or modify the contents. This may lead to hard to find security bugs and hopefully the rename makes it harder to forget. The change also attempts to fix all the callsites to make everything build correctly, but in the processes, wherever the callsite code was not obviously secure, I added a TODO requesting the owners to either document why it's secure or to change the code. Apologies in advance to the owners if there are some false positives here - I don't have enough context to reason about all the different callsites. Test: Completely syntactic change. Made sure code still builds. Change-Id: I5fb99aa797c488406083178a6b05355d98710d3b --- cmds/stagefright/stagefright.cpp | 2 +- cmds/stagefright/stream.cpp | 4 +- drm/libmediadrm/CryptoHal.cpp | 6 +- media/codec2/sfplugin/Codec2Buffer.cpp | 8 ++- media/libaudioclient/AudioEffect.cpp | 6 +- media/libaudioclient/AudioRecord.cpp | 12 +++- media/libaudioclient/AudioTrack.cpp | 14 ++++- media/libaudioclient/IAudioTrack.cpp | 2 +- media/libaudioclient/IEffect.cpp | 2 +- .../include/media/IAudioFlinger.h | 18 +++++- media/libheif/HeifDecoderImpl.cpp | 55 ++++++++++++++----- media/libmedia/IMediaHTTPConnection.cpp | 6 +- .../MetadataRetrieverClient.cpp | 2 +- .../nuplayer/NuPlayerStreamListener.cpp | 2 +- media/libnblog/Reader.cpp | 10 +++- media/libnblog/Writer.cpp | 6 +- media/libstagefright/ACodecBufferChannel.cpp | 6 +- media/libstagefright/BufferImpl.cpp | 6 +- media/libstagefright/CallbackDataSource.cpp | 3 +- media/libstagefright/CameraSource.cpp | 26 ++++++--- .../libstagefright/CameraSourceTimeLapse.cpp | 4 +- media/libstagefright/FrameDecoder.cpp | 8 +-- .../StagefrightMediaScanner.cpp | 6 +- .../libstagefright/foundation/MediaBuffer.cpp | 2 +- .../foundation/MediaBufferGroup.cpp | 2 +- .../include/media/stagefright/MediaBuffer.h | 36 +++++++++--- .../media/stagefright/RemoteDataSource.h | 2 +- media/libstagefright/omx/OMXNodeInstance.cpp | 8 ++- media/utils/ServiceUtilities.cpp | 4 +- services/audioflinger/AudioFlinger.cpp | 2 +- services/audioflinger/Effects.cpp | 2 +- services/audioflinger/Threads.cpp | 6 +- services/audioflinger/Tracks.cpp | 14 +++-- .../libcameraservice/api1/CameraClient.cpp | 16 ++++-- .../device1/CameraHardwareInterface.cpp | 24 ++++++-- .../soundtrigger/SoundTriggerHwService.cpp | 34 ++++++++---- soundtrigger/SoundTrigger.cpp | 15 +++-- 37 files changed, 272 insertions(+), 109 deletions(-) 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())); } }