Fix security vulnerability: potential OOB write in audioserver

Bug: 32705438 Bug: 32703959 Test: cts security test Change-Id: I8900c92fa55b56c4c2c9d721efdbabe6bfc8a4a4 (cherry picked from commit e275907) (cherry picked from commit b0bcddb44d992e74140a3f5eedc7177977ea8e34)
android-security · Mar 17, 2017 · b317df8 · b317df8
1 parent cf6aea2
commit b317df8
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 6 deletions.
diff --git a/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp b/media/libeffects/lvm/wrapper/Bundle/EffectBundle.cpp
@@ -2829,24 +2829,39 @@ int Effect_command(effect_handle_t  self,
             //ALOGV("\tEffect_command cmdCode Case: EFFECT_CMD_GET_PARAM start");
 
             effect_param_t *p = (effect_param_t *)pCmdData;
-            if (SIZE_MAX - sizeof(effect_param_t) < (size_t)p->psize) {
-                android_errorWriteLog(0x534e4554, "26347509");
-                return -EINVAL;
-            }
             if (pCmdData == NULL || cmdSize < sizeof(effect_param_t) ||
                     cmdSize < (sizeof(effect_param_t) + p->psize) ||
                     pReplyData == NULL || replySize == NULL ||
                     *replySize < (sizeof(effect_param_t) + p->psize)) {
                 ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: ERROR");
                 return -EINVAL;
             }
+            if (EFFECT_PARAM_SIZE_MAX - sizeof(effect_param_t) < (size_t)p->psize) {
+                android_errorWriteLog(0x534e4554, "26347509");
+                ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: psize too big");
+                return -EINVAL;
+            }
+            uint32_t paddedParamSize = ((p->psize + sizeof(int32_t) - 1) / sizeof(int32_t)) *
+                    sizeof(int32_t);
+            if ((EFFECT_PARAM_SIZE_MAX - sizeof(effect_param_t) < paddedParamSize) ||
+                (EFFECT_PARAM_SIZE_MAX - sizeof(effect_param_t) - paddedParamSize <
+                    p->vsize)) {
+                ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: padded_psize or vsize too big");
+                return -EINVAL;
+            }
+            uint32_t expectedReplySize = sizeof(effect_param_t) + paddedParamSize + p->vsize;
+            if (*replySize < expectedReplySize) {
+                ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: min. replySize %u, got %u bytes",
+                        expectedReplySize, *replySize);
+                android_errorWriteLog(0x534e4554, "32705438");
+                return -EINVAL;
+            }
 
             memcpy(pReplyData, pCmdData, sizeof(effect_param_t) + p->psize);
 
             p = (effect_param_t *)pReplyData;
 
-            int voffset = ((p->psize - 1) / sizeof(int32_t) + 1) * sizeof(int32_t);
+            uint32_t voffset = paddedParamSize;
-
             if(pContext->EffectType == LVM_BASS_BOOST){
                 p->status = android::BassBoost_getParameter(pContext,
                                                             p->data,

diff --git a/media/libmedia/IEffect.cpp b/media/libmedia/IEffect.cpp
@@ -25,6 +25,9 @@
 
 namespace android {
 
+// Maximum command/reply size expected
+#define EFFECT_PARAM_SIZE_MAX       65536
+
 enum {
     ENABLE = IBinder::FIRST_CALL_TRANSACTION,
     DISABLE,
@@ -153,6 +156,10 @@ status_t BnEffect::onTransact(
             uint32_t cmdSize = data.readInt32();
             char *cmd = NULL;
             if (cmdSize) {
+                if (cmdSize > EFFECT_PARAM_SIZE_MAX) {
+                    reply->writeInt32(NO_MEMORY);
+                    return NO_ERROR;
+                }
                 cmd = (char *)calloc(cmdSize, 1);
                 if (cmd == NULL) {
                     reply->writeInt32(NO_MEMORY);
@@ -164,6 +171,11 @@ status_t BnEffect::onTransact(
             uint32_t replySz = replySize;
             char *resp = NULL;
             if (replySize) {
+                if (replySize > EFFECT_PARAM_SIZE_MAX) {
+                    free(cmd);
+                    reply->writeInt32(NO_MEMORY);
+                    return NO_ERROR;
+                }
                 resp = (char *)calloc(replySize, 1);
                 if (resp == NULL) {
                     free(cmd);

diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp
@@ -546,6 +546,22 @@ status_t AudioFlinger::EffectModule::command(uint32_t cmdCode,
         android_errorWriteLog(0x534e4554, "32438594");
         return -EINVAL;
     }
+    if (cmdCode == EFFECT_CMD_GET_PARAM &&
+        (sizeof(effect_param_t) > *replySize
+          || ((effect_param_t *)pCmdData)->psize > *replySize
+                                                   - sizeof(effect_param_t)
+          || ((effect_param_t *)pCmdData)->vsize > *replySize
+                                                   - sizeof(effect_param_t)
+                                                   - ((effect_param_t *)pCmdData)->psize
+          || roundUpDelta(((effect_param_t *)pCmdData)->psize, (uint32_t)sizeof(int)) >
+                                                   *replySize
+                                                   - sizeof(effect_param_t)
+                                                   - ((effect_param_t *)pCmdData)->psize
+                                                   - ((effect_param_t *)pCmdData)->vsize)) {
+        ALOGV("\tLVM_ERROR : EFFECT_CMD_GET_PARAM: reply size inconsistent");
+                     android_errorWriteLog(0x534e4554, "32705438");
+        return -EINVAL;
+    }
     if ((cmdCode == EFFECT_CMD_SET_PARAM
             || cmdCode == EFFECT_CMD_SET_PARAM_DEFERRED) &&  // DEFERRED not generally used
         (sizeof(effect_param_t) > cmdSize