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/media/libaudioclient/AudioEffect.cpp b/media/libaudioclient/AudioEffect.cpp
index cf11936..1cc5fe6 100644
--- a/media/libaudioclient/AudioEffect.cpp
+++ b/media/libaudioclient/AudioEffect.cpp
@@ -159,7 +159,11 @@
mIEffect = iEffect;
mCblkMemory = cblk;
- mCblk = static_cast<effect_param_cblk_t*>(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<effect_param_cblk_t*>(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 a1b04ca..0f2d48e 100644
--- a/media/libaudioclient/AudioRecord.cpp
+++ b/media/libaudioclient/AudioRecord.cpp
@@ -759,7 +759,11 @@
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 @@
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 4a80cd3..e8d7b60 100644
--- a/media/libaudioclient/AudioTrack.cpp
+++ b/media/libaudioclient/AudioTrack.cpp
@@ -406,7 +406,7 @@
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 = 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 @@
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 83a568a..6219e7a 100644
--- a/media/libaudioclient/IAudioTrack.cpp
+++ b/media/libaudioclient/IAudioTrack.cpp
@@ -62,7 +62,7 @@
status_t status = remote()->transact(GET_CBLK, data, &reply);
if (status == NO_ERROR) {
cblk = interface_cast<IMemory>(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 ce72dae..5d47dff 100644
--- a/media/libaudioclient/IEffect.cpp
+++ b/media/libaudioclient/IEffect.cpp
@@ -122,7 +122,7 @@
status_t status = remote()->transact(GET_CBLK, data, &reply);
if (status == NO_ERROR) {
cblk = interface_cast<IMemory>(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 db09ddf..b580a88 100644
--- a/media/libaudioclient/include/media/IAudioFlinger.h
+++ b/media/libaudioclient/include/media/IAudioFlinger.h
@@ -70,8 +70,12 @@
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<IMemory>(parcel->readStrongBinder());
- if (sharedBuffer == 0 || sharedBuffer->pointer() == NULL) {
+ if (sharedBuffer == 0 || sharedBuffer->unsecurePointer() == NULL) {
return BAD_VALUE;
}
}
@@ -269,13 +273,21 @@
(void)parcel->read(&inputId, sizeof(audio_io_handle_t));
if (parcel->readInt32() != 0) {
cblk = interface_cast<IMemory>(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<IMemory>(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;
}
}