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
diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index aef0ade..27d7856 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -652,7 +652,7 @@
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 13152d0..d54ab42 100644
--- a/services/audioflinger/Effects.cpp
+++ b/services/audioflinger/Effects.cpp
@@ -1588,7 +1588,7 @@
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<effect_param_cblk_t *>(mCblkMemory->pointer())) == NULL) {
+ (mCblk = static_cast<effect_param_cblk_t *>(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 6ca50a7..8fddc13 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -2079,9 +2079,9 @@
// 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 @@
sp<IMemory> 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 fa35e5d..6a999fc 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -150,7 +150,7 @@
if (client != 0) {
mCblkMemory = client->heap()->allocate(size);
if (mCblkMemory == 0 ||
- (mCblk = static_cast<audio_track_cblk_t *>(mCblkMemory->pointer())) == NULL) {
+ (mCblk = static_cast<audio_track_cblk_t *>(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 @@
const sp<MemoryDealer> 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 @@
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 @@
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 @@
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 764b3a9..388a5dc 100644
--- a/services/camera/libcameraservice/api1/CameraClient.cpp
+++ b/services/camera/libcameraservice/api1/CameraClient.cpp
@@ -534,7 +534,7 @@
}
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 @@
}
if (!disconnected) {
- VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(dataPtr->pointer());
+ VideoNativeHandleMetadata *metadata = (VideoNativeHandleMetadata*)(dataPtr->unsecurePointer());
metadata->eType = kMetadataBufferTypeNativeHandleSource;
metadata->pHandle = handle;
frames.push_back(dataPtr);
@@ -916,8 +916,12 @@
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 @@
// 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 522d521..62ef681 100644
--- a/services/camera/libcameraservice/device1/CameraHardwareInterface.cpp
+++ b/services/camera/libcameraservice/device1/CameraHardwareInterface.cpp
@@ -165,8 +165,12 @@
mem = mHidlMemPoolMap.at(data);
}
sp<CameraHeapMemory> heapMem(static_cast<CameraHeapMemory *>(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<native_handle_t*>(frameData.getNativeHandle());
sDataCbTimestamp(timestamp, (int32_t) msgType, mem, bufferIndex, this);
return hardware::Void();
@@ -192,8 +196,12 @@
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<native_handle_t*>(hidl_msg.frameData.getNativeHandle());
msgs.push_back({hidl_msg.timestamp, mem->mBuffers[hidl_msg.bufferIndex]});
@@ -578,7 +586,11 @@
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 @@
if (size == sizeof(VideoNativeHandleMetadata)) {
uint32_t heapId = heap->getHeapID();
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 377d30b..69e5f50 100644
--- a/services/soundtrigger/SoundTriggerHwService.cpp
+++ b/services/soundtrigger/SoundTriggerHwService.cpp
@@ -234,11 +234,11 @@
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 @@
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 @@
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 @@
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 @@
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 @@
{
ALOGV("onCallbackEvent type %d", event->mType);
+ // Memory is coming from a trusted process.
sp<IMemory> eventMemory = event->mMemory;
- if (eventMemory == 0 || eventMemory->pointer() == NULL) {
+ if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) {
return;
}
if (mModuleClients.isEmpty()) {
@@ -749,7 +758,7 @@
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> model = getModel(recognitionEvent->model);
@@ -769,7 +778,7 @@
} 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> model = getModel(soundmodelEvent->model);
@@ -1082,7 +1091,8 @@
sp<IMemory> eventMemory = event->mMemory;
- if (eventMemory == 0 || eventMemory->pointer() == NULL) {
+ // Memory is coming from a trusted process.
+ if (eventMemory == 0 || eventMemory->unsecurePointer() == NULL) {
return;
}