From 3ba0bbe12904d0dfa2245fa3abf2b92034b15db3 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Mon, 8 Feb 2016 10:56:13 -0800 Subject: IOMX.cpp uninitialized pointer in BnOMX::onTransact This can lead to local code execution in media server. Fix initializes the pointer and checks the error conditions before returning Bug: 26403627 Change-Id: I7fa90682060148448dba01d6acbe3471d1ddb500 --- media/libmedia/IOMX.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 5423c2a..8f55eb9 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -446,7 +446,7 @@ public: remote()->transact(CONFIGURE_VIDEO_TUNNEL_MODE, data, &reply); status_t err = reply.readInt32(); - if (sidebandHandle) { + if (err == OK && sidebandHandle) { *sidebandHandle = (native_handle_t *)reply.readNativeHandle(); } return err; @@ -948,11 +948,13 @@ status_t BnOMX::onTransact( OMX_BOOL tunneled = (OMX_BOOL)data.readInt32(); OMX_U32 audio_hw_sync = data.readInt32(); - native_handle_t *sideband_handle; + native_handle_t *sideband_handle = NULL; status_t err = configureVideoTunnelMode( node, port_index, tunneled, audio_hw_sync, &sideband_handle); reply->writeInt32(err); - reply->writeNativeHandle(sideband_handle); + if(err == OK){ + reply->writeNativeHandle(sideband_handle); + } return NO_ERROR; } -- cgit v1.1 From 57bf4973b57dc62e7171c1cb2e0854f9e21fd3e4 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Mon, 8 Feb 2016 17:07:06 -0800 Subject: 3 uninitialized variables in IOMX.cpp Uninitialized MetadataBufferType pointer in case: SET_INPUT_SURFACE STORE_META_DATA_IN_BUFFERS CREATE_INPUT_SURFACE Fix: initialize them to kMetadataBufferTypeInvalid Bug: 26324358 Change-Id: Ib3bcac9bedc98bc65efa39f67cdbffbd90b374c9 --- media/libmedia/IOMX.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 8f55eb9..07f3697 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -844,9 +844,13 @@ status_t BnOMX::onTransact( OMX_U32 port_index = data.readInt32(); sp bufferProducer; - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = createInputSurface(node, port_index, &bufferProducer, &type); + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); @@ -886,9 +890,13 @@ status_t BnOMX::onTransact( sp bufferConsumer = interface_cast(data.readStrongBinder()); - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = setInputSurface(node, port_index, bufferConsumer, &type); + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); return NO_ERROR; @@ -914,8 +922,13 @@ status_t BnOMX::onTransact( OMX_U32 port_index = data.readInt32(); OMX_BOOL enable = (OMX_BOOL)data.readInt32(); - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = storeMetaDataInBuffers(node, port_index, enable, &type); + + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); -- cgit v1.1 From 1f76ce4e2c13d0347523e8c9a27077c820715f08 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Fri, 19 Feb 2016 12:10:48 -0800 Subject: Fixing safteynet logging bug introduced in ag/862848 We moved the safteynet logging into storeMetaDataInBuffers_l Bug: 26324358 Change-Id: I2171742b53192ebb71d546bcb8970bb3c68f366f --- media/libmedia/IOMX.cpp | 4 ---- 1 file changed, 4 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 07f3697..97b7ff0 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -925,10 +925,6 @@ status_t BnOMX::onTransact( MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = storeMetaDataInBuffers(node, port_index, enable, &type); - if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { - android_errorWriteLog(0x534e4554, "26324358"); - } - reply->writeInt32(type); reply->writeInt32(err); -- cgit v1.1 From 3a90a02bb6c4e8352112dbb9f2316935e4dd7315 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Mon, 22 Feb 2016 13:05:15 -0800 Subject: Clear allocation to avoid info leak Bug: 26914474 Change-Id: Ie1a86e86d78058d041149fe599a4996e7f8185cf --- media/libmedia/IOMX.cpp | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 97b7ff0..865d575 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -692,34 +692,39 @@ status_t BnOMX::onTransact( size_t size = data.readInt64(); - void *params = malloc(size); - data.read(params, size); - - status_t err; - switch (code) { - case GET_PARAMETER: - err = getParameter(node, index, params, size); - break; - case SET_PARAMETER: - err = setParameter(node, index, params, size); - break; - case GET_CONFIG: - err = getConfig(node, index, params, size); - break; - case SET_CONFIG: - err = setConfig(node, index, params, size); - break; - case SET_INTERNAL_OPTION: - { - InternalOptionType type = - (InternalOptionType)data.readInt32(); - - err = setInternalOption(node, index, type, params, size); - break; + status_t err = NO_MEMORY; + void *params = calloc(size, 1); + if (params) { + err = data.read(params, size); + if (err != OK) { + android_errorWriteLog(0x534e4554, "26914474"); + } else { + switch (code) { + case GET_PARAMETER: + err = getParameter(node, index, params, size); + break; + case SET_PARAMETER: + err = setParameter(node, index, params, size); + break; + case GET_CONFIG: + err = getConfig(node, index, params, size); + break; + case SET_CONFIG: + err = setConfig(node, index, params, size); + break; + case SET_INTERNAL_OPTION: + { + InternalOptionType type = + (InternalOptionType)data.readInt32(); + + err = setInternalOption(node, index, type, params, size); + break; + } + + default: + TRESPASS(); + } } - - default: - TRESPASS(); } reply->writeInt32(err); -- cgit v1.1 From 52fbd46e2b7bc185628e94178cc3ec8e2a340b40 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Mon, 8 Feb 2016 10:56:13 -0800 Subject: IOMX.cpp uninitialized pointer in BnOMX::onTransact This can lead to local code execution in media server. Fix initializes the pointer and checks the error conditions before returning Bug: 26403627 Change-Id: I7fa90682060148448dba01d6acbe3471d1ddb500 --- media/libmedia/IOMX.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 5423c2a..8f55eb9 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -446,7 +446,7 @@ public: remote()->transact(CONFIGURE_VIDEO_TUNNEL_MODE, data, &reply); status_t err = reply.readInt32(); - if (sidebandHandle) { + if (err == OK && sidebandHandle) { *sidebandHandle = (native_handle_t *)reply.readNativeHandle(); } return err; @@ -948,11 +948,13 @@ status_t BnOMX::onTransact( OMX_BOOL tunneled = (OMX_BOOL)data.readInt32(); OMX_U32 audio_hw_sync = data.readInt32(); - native_handle_t *sideband_handle; + native_handle_t *sideband_handle = NULL; status_t err = configureVideoTunnelMode( node, port_index, tunneled, audio_hw_sync, &sideband_handle); reply->writeInt32(err); - reply->writeNativeHandle(sideband_handle); + if(err == OK){ + reply->writeNativeHandle(sideband_handle); + } return NO_ERROR; } -- cgit v1.1 From 1a5ab71d6a43a53535f37648b09d22b741265bb2 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Mon, 8 Feb 2016 17:07:06 -0800 Subject: 3 uninitialized variables in IOMX.cpp Uninitialized MetadataBufferType pointer in case: SET_INPUT_SURFACE STORE_META_DATA_IN_BUFFERS CREATE_INPUT_SURFACE Fix: initialize them to kMetadataBufferTypeInvalid Bug: 26324358 Change-Id: Ib3bcac9bedc98bc65efa39f67cdbffbd90b374c9 --- media/libmedia/IOMX.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 8f55eb9..07f3697 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -844,9 +844,13 @@ status_t BnOMX::onTransact( OMX_U32 port_index = data.readInt32(); sp bufferProducer; - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = createInputSurface(node, port_index, &bufferProducer, &type); + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); @@ -886,9 +890,13 @@ status_t BnOMX::onTransact( sp bufferConsumer = interface_cast(data.readStrongBinder()); - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = setInputSurface(node, port_index, bufferConsumer, &type); + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); return NO_ERROR; @@ -914,8 +922,13 @@ status_t BnOMX::onTransact( OMX_U32 port_index = data.readInt32(); OMX_BOOL enable = (OMX_BOOL)data.readInt32(); - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = storeMetaDataInBuffers(node, port_index, enable, &type); + + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); -- cgit v1.1 From a685aea781026471ad82f5729198c60bcb4f8fe9 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Fri, 19 Feb 2016 12:10:48 -0800 Subject: Fixing safteynet logging bug introduced in ag/862848 We moved the safteynet logging into storeMetaDataInBuffers_l Bug: 26324358 Change-Id: I2171742b53192ebb71d546bcb8970bb3c68f366f --- media/libmedia/IOMX.cpp | 4 ---- 1 file changed, 4 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 07f3697..97b7ff0 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -925,10 +925,6 @@ status_t BnOMX::onTransact( MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = storeMetaDataInBuffers(node, port_index, enable, &type); - if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { - android_errorWriteLog(0x534e4554, "26324358"); - } - reply->writeInt32(type); reply->writeInt32(err); -- cgit v1.1 From d3e5eca42feaa337cd3aa36b7eaa2b7aadbbab15 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Mon, 22 Feb 2016 13:05:15 -0800 Subject: Clear allocation to avoid info leak Bug: 26914474 Change-Id: Ie1a86e86d78058d041149fe599a4996e7f8185cf --- media/libmedia/IOMX.cpp | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 97b7ff0..865d575 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -692,34 +692,39 @@ status_t BnOMX::onTransact( size_t size = data.readInt64(); - void *params = malloc(size); - data.read(params, size); - - status_t err; - switch (code) { - case GET_PARAMETER: - err = getParameter(node, index, params, size); - break; - case SET_PARAMETER: - err = setParameter(node, index, params, size); - break; - case GET_CONFIG: - err = getConfig(node, index, params, size); - break; - case SET_CONFIG: - err = setConfig(node, index, params, size); - break; - case SET_INTERNAL_OPTION: - { - InternalOptionType type = - (InternalOptionType)data.readInt32(); - - err = setInternalOption(node, index, type, params, size); - break; + status_t err = NO_MEMORY; + void *params = calloc(size, 1); + if (params) { + err = data.read(params, size); + if (err != OK) { + android_errorWriteLog(0x534e4554, "26914474"); + } else { + switch (code) { + case GET_PARAMETER: + err = getParameter(node, index, params, size); + break; + case SET_PARAMETER: + err = setParameter(node, index, params, size); + break; + case GET_CONFIG: + err = getConfig(node, index, params, size); + break; + case SET_CONFIG: + err = setConfig(node, index, params, size); + break; + case SET_INTERNAL_OPTION: + { + InternalOptionType type = + (InternalOptionType)data.readInt32(); + + err = setInternalOption(node, index, type, params, size); + break; + } + + default: + TRESPASS(); + } } - - default: - TRESPASS(); } reply->writeInt32(err); -- cgit v1.1 From 295c883fe3105b19bcd0f9e07d54c6b589fc5bff Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Mon, 29 Feb 2016 12:47:20 -0800 Subject: DO NOT MERGE Verify OMX buffer sizes prior to access Bug: 27207275 Change-Id: I4412825d1ee233d993af0a67708bea54304ff62d --- media/libmedia/IOMX.cpp | 96 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 66 insertions(+), 30 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 865d575..84925f1 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -18,6 +18,8 @@ #define LOG_TAG "IOMX" #include +#include + #include #include #include @@ -692,38 +694,70 @@ status_t BnOMX::onTransact( size_t size = data.readInt64(); - status_t err = NO_MEMORY; - void *params = calloc(size, 1); - if (params) { - err = data.read(params, size); - if (err != OK) { - android_errorWriteLog(0x534e4554, "26914474"); + status_t err = NOT_ENOUGH_DATA; + void *params = NULL; + size_t pageSize = 0; + size_t allocSize = 0; + if (code != SET_INTERNAL_OPTION && size < 8) { + // we expect the structure to contain at least the size and + // version, 8 bytes total + ALOGE("b/27207275 (%zu)", size); + android_errorWriteLog(0x534e4554, "27207275"); + } else { + err = NO_MEMORY; + pageSize = (size_t) sysconf(_SC_PAGE_SIZE); + if (size > SIZE_MAX - (pageSize * 2)) { + ALOGE("requested param size too big"); } else { - switch (code) { - case GET_PARAMETER: - err = getParameter(node, index, params, size); - break; - case SET_PARAMETER: - err = setParameter(node, index, params, size); - break; - case GET_CONFIG: - err = getConfig(node, index, params, size); - break; - case SET_CONFIG: - err = setConfig(node, index, params, size); - break; - case SET_INTERNAL_OPTION: - { - InternalOptionType type = - (InternalOptionType)data.readInt32(); - - err = setInternalOption(node, index, type, params, size); - break; + allocSize = (size + pageSize * 2) & ~(pageSize - 1); + params = mmap(NULL, allocSize, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1 /* fd */, 0 /* offset */); + } + if (params != MAP_FAILED) { + err = data.read(params, size); + if (err != OK) { + android_errorWriteLog(0x534e4554, "26914474"); + } else { + err = NOT_ENOUGH_DATA; + OMX_U32 declaredSize = *(OMX_U32*)params; + if (code != SET_INTERNAL_OPTION && declaredSize > size) { + // the buffer says it's bigger than it actually is + ALOGE("b/27207275 (%u/%zu)", declaredSize, size); + android_errorWriteLog(0x534e4554, "27207275"); + } else { + // mark the last page as inaccessible, to avoid exploitation + // of codecs that access past the end of the allocation because + // they didn't check the size + mprotect((char*)params + allocSize - pageSize, pageSize, PROT_NONE); + switch (code) { + case GET_PARAMETER: + err = getParameter(node, index, params, size); + break; + case SET_PARAMETER: + err = setParameter(node, index, params, size); + break; + case GET_CONFIG: + err = getConfig(node, index, params, size); + break; + case SET_CONFIG: + err = setConfig(node, index, params, size); + break; + case SET_INTERNAL_OPTION: + { + InternalOptionType type = + (InternalOptionType)data.readInt32(); + + err = setInternalOption(node, index, type, params, size); + break; + } + + default: + TRESPASS(); + } } - - default: - TRESPASS(); } + } else { + ALOGE("couldn't map: %s", strerror(errno)); } } @@ -733,7 +767,9 @@ status_t BnOMX::onTransact( reply->write(params, size); } - free(params); + if (params) { + munmap(params, allocSize); + } params = NULL; return NO_ERROR; -- cgit v1.1 From 0bb5ced60304da7f61478ffd359e7ba65d72f181 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Thu, 10 Mar 2016 15:02:13 -0800 Subject: Fix size check for OMX_IndexParamConsumerUsageBits since it doesn't follow the OMX convention. And remove support for the kClientNeedsFrameBuffer flag. Bug: 27207275 Change-Id: Ia2c119e2456ebf9e2f4e1de5104ef9032a212255 --- media/libmedia/IOMX.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 84925f1..9be9b41 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -24,6 +24,7 @@ #include #include #include +#include namespace android { @@ -698,7 +699,8 @@ status_t BnOMX::onTransact( void *params = NULL; size_t pageSize = 0; size_t allocSize = 0; - if (code != SET_INTERNAL_OPTION && size < 8) { + if ((index == (OMX_INDEXTYPE) OMX_IndexParamConsumerUsageBits && size < 4) || + (code != SET_INTERNAL_OPTION && size < 8)) { // we expect the structure to contain at least the size and // version, 8 bytes total ALOGE("b/27207275 (%zu)", size); @@ -720,7 +722,9 @@ status_t BnOMX::onTransact( } else { err = NOT_ENOUGH_DATA; OMX_U32 declaredSize = *(OMX_U32*)params; - if (code != SET_INTERNAL_OPTION && declaredSize > size) { + if (code != SET_INTERNAL_OPTION && + index != (OMX_INDEXTYPE) OMX_IndexParamConsumerUsageBits && + declaredSize > size) { // the buffer says it's bigger than it actually is ALOGE("b/27207275 (%u/%zu)", declaredSize, size); android_errorWriteLog(0x534e4554, "27207275"); -- cgit v1.1 From db829699d3293f254a7387894303451a91278986 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Mon, 21 Mar 2016 11:31:53 -0700 Subject: Fix OMX_IndexParamConsumerUsageBits size check Bug: 27207275 Change-Id: I9a7c9fb22a0e84a490ff09c151bd2f88141fdbc0 --- media/libmedia/IOMX.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 9be9b41..b082fe4 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -699,11 +699,12 @@ status_t BnOMX::onTransact( void *params = NULL; size_t pageSize = 0; size_t allocSize = 0; - if ((index == (OMX_INDEXTYPE) OMX_IndexParamConsumerUsageBits && size < 4) || - (code != SET_INTERNAL_OPTION && size < 8)) { + bool isUsageBits = (index == (OMX_INDEXTYPE) OMX_IndexParamConsumerUsageBits); + if ((isUsageBits && size < 4) || + (!isUsageBits && code != SET_INTERNAL_OPTION && size < 8)) { // we expect the structure to contain at least the size and // version, 8 bytes total - ALOGE("b/27207275 (%zu)", size); + ALOGE("b/27207275 (%zu) (%d/%d)", size, int(index), int(code)); android_errorWriteLog(0x534e4554, "27207275"); } else { err = NO_MEMORY; -- cgit v1.1 From bc8a45f506a8be33250c523d71fab637a5fdaf81 Mon Sep 17 00:00:00 2001 From: Lajos Molnar Date: Tue, 2 Aug 2016 07:07:05 -0700 Subject: IOMX: work against metadata buffer spoofing - Prohibit direct set/getParam/Settings for extensions meant for OMXNodeInstance alone. This disallows enabling metadata mode without the knowledge of OMXNodeInstance. - Do not share metadata mode buffers cross process. - Disallow setting up metadata mode/tunneling/input surface after first sendCommand. - Disallow store-meta for input cross process. - Disallow emptyBuffer for surface input (via IOMX). - Fix checking for input surface. Bug: 29422020 Change-Id: I801c77b80e703903f62e42d76fd2e76a34e4bc8e (cherry picked from commit f8a4cb410115045278f534e54b41ac78d6bf6c07) --- media/libmedia/IOMX.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index b082fe4..c28eac8 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -248,7 +248,7 @@ public: virtual status_t useBuffer( node_id node, OMX_U32 port_index, const sp ¶ms, - buffer_id *buffer, OMX_U32 allottedSize) { + buffer_id *buffer, OMX_U32 allottedSize, OMX_BOOL /* crossProcess */) { Parcel data, reply; data.writeInterfaceToken(IOMX::getInterfaceDescriptor()); data.writeInt32((int32_t)node); @@ -481,7 +481,7 @@ public: virtual status_t allocateBufferWithBackup( node_id node, OMX_U32 port_index, const sp ¶ms, - buffer_id *buffer, OMX_U32 allottedSize) { + buffer_id *buffer, OMX_U32 allottedSize, OMX_BOOL /* crossProcess */) { Parcel data, reply; data.writeInterfaceToken(IOMX::getInterfaceDescriptor()); data.writeInt32((int32_t)node); @@ -834,7 +834,8 @@ status_t BnOMX::onTransact( OMX_U32 allottedSize = data.readInt32(); buffer_id buffer; - status_t err = useBuffer(node, port_index, params, &buffer, allottedSize); + status_t err = useBuffer( + node, port_index, params, &buffer, allottedSize, OMX_TRUE /* crossProcess */); reply->writeInt32(err); if (err == OK) { @@ -969,7 +970,10 @@ status_t BnOMX::onTransact( OMX_BOOL enable = (OMX_BOOL)data.readInt32(); MetadataBufferType type = kMetadataBufferTypeInvalid; - status_t err = storeMetaDataInBuffers(node, port_index, enable, &type); + status_t err = + // only control output metadata via Binder + port_index != 1 /* kOutputPortIndex */ ? BAD_VALUE : + storeMetaDataInBuffers(node, port_index, enable, &type); reply->writeInt32(type); reply->writeInt32(err); @@ -1054,7 +1058,7 @@ status_t BnOMX::onTransact( buffer_id buffer; status_t err = allocateBufferWithBackup( - node, port_index, params, &buffer, allottedSize); + node, port_index, params, &buffer, allottedSize, OMX_TRUE /* crossProcess */); reply->writeInt32(err); -- cgit v1.1 From c2dd82bfd6d7aea5b6760efc0712ae11d7a52e6b Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Tue, 20 Sep 2016 13:36:40 -0700 Subject: Check mprotect result mprotect can theoretically fail, which could then let one exploit a vulnerable codec if one exists on the device. Bug: 31350239 Change-Id: I7b99c190619f0fb2eb93119596e6da0d2deb8ba5 (cherry picked from commit 866c800c0624bb13eee44973cc8a2ecd0012de6e) --- media/libmedia/IOMX.cpp | 52 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 24 deletions(-) (limited to 'media/libmedia/IOMX.cpp') diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index c28eac8..365d9ac 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -733,31 +733,35 @@ status_t BnOMX::onTransact( // mark the last page as inaccessible, to avoid exploitation // of codecs that access past the end of the allocation because // they didn't check the size - mprotect((char*)params + allocSize - pageSize, pageSize, PROT_NONE); - switch (code) { - case GET_PARAMETER: - err = getParameter(node, index, params, size); - break; - case SET_PARAMETER: - err = setParameter(node, index, params, size); - break; - case GET_CONFIG: - err = getConfig(node, index, params, size); - break; - case SET_CONFIG: - err = setConfig(node, index, params, size); - break; - case SET_INTERNAL_OPTION: - { - InternalOptionType type = - (InternalOptionType)data.readInt32(); - - err = setInternalOption(node, index, type, params, size); - break; + if (mprotect((char*)params + allocSize - pageSize, pageSize, + PROT_NONE) != 0) { + ALOGE("mprotect failed: %s", strerror(errno)); + } else { + switch (code) { + case GET_PARAMETER: + err = getParameter(node, index, params, size); + break; + case SET_PARAMETER: + err = setParameter(node, index, params, size); + break; + case GET_CONFIG: + err = getConfig(node, index, params, size); + break; + case SET_CONFIG: + err = setConfig(node, index, params, size); + break; + case SET_INTERNAL_OPTION: + { + InternalOptionType type = + (InternalOptionType)data.readInt32(); + + err = setInternalOption(node, index, type, params, size); + break; + } + + default: + TRESPASS(); } - - default: - TRESPASS(); } } } -- cgit v1.1