Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 66ebaf7f authored by TreeHugger Robot's avatar TreeHugger Robot Committed by Android (Google) Code Review
Browse files

Merge "Fix vulnerability in MemoryIntArray"

parents 45cf37aa 74c9983e

Android.bp

+4 −0
Original line number Diff line number Diff line
@@ -17,3 +17,7 @@ subdirs = [ 
    "native/android",

    "native/graphics/jni",

]



optional_subdirs = [

    "core/tests/utiltests/jni",

]

core/java/android/util/MemoryIntArray.java

+21 −39
Original line number Diff line number Diff line
@@ -19,7 +19,6 @@ package android.util; 
import android.os.Parcel;

import android.os.ParcelFileDescriptor;

import android.os.Parcelable;

import android.os.Process;



import libcore.io.IoUtils;

import dalvik.system.CloseGuard;
@@ -37,13 +36,13 @@ import java.util.UUID; 
 * each other.

 * <p>

 * The data structure is designed to have one owner process that can

 * read/write. There may be multiple client processes that can only read or

 * read/write depending how the data structure was configured when

 * instantiated. The owner process is the process that created the array.

 * The shared memory is pinned (not reclaimed by the system) until the

 * owning process dies or the data structure is closed. This class

 * is <strong>not</strong> thread safe. You should not interact with

 * an instance of this class once it is closed.

 * read/write. There may be multiple client processes that can only read.

 * The owner process is the process that created the array. The shared

 * memory is pinned (not reclaimed by the system) until the owning process

 * dies or the data structure is closed. This class is <strong>not</strong>

 * thread safe. You should not interact with an instance of this class

 * once it is closed. If you pass back to the owner process an instance

 * it will be read only even in the owning process.

 * </p>

 *

 * @hide
@@ -55,8 +54,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { 


    private final CloseGuard mCloseGuard = CloseGuard.get();



    private final int mOwnerPid;

    private final boolean mClientWritable;

    private final boolean mIsOwner;

    private final long mMemoryAddr;

    private int mFd;
@@ -65,35 +63,27 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
     *

     * @param size The size of the array in terms of integer slots. Cannot be

     *     more than {@link #getMaxSize()}.

     * @param clientWritable Whether other processes can write to the array.

     * @throws IOException If an error occurs while accessing the shared memory.

     */

    public MemoryIntArray(int size, boolean clientWritable) throws IOException {

    public MemoryIntArray(int size) throws IOException {

        if (size > MAX_SIZE) {

            throw new IllegalArgumentException("Max size is " + MAX_SIZE);

        }

        mOwnerPid = Process.myPid();

        mClientWritable = clientWritable;

        mIsOwner = true;

        final String name = UUID.randomUUID().toString();

        mFd = nativeCreate(name, size);

        mMemoryAddr = nativeOpen(mFd, true, clientWritable);

        mMemoryAddr = nativeOpen(mFd, mIsOwner);

        mCloseGuard.open("close");

    }



    private MemoryIntArray(Parcel parcel) throws IOException {

        mOwnerPid = parcel.readInt();

        mClientWritable = (parcel.readInt() == 1);

        mIsOwner = false;

        ParcelFileDescriptor pfd = parcel.readParcelable(null);

        if (pfd == null) {

            throw new IOException("No backing file descriptor");

        }

        mFd = pfd.detachFd();

        final long memoryAddress = parcel.readLong();

        if (isOwner()) {

            mMemoryAddr = memoryAddress;

        } else {

            mMemoryAddr = nativeOpen(mFd, false, mClientWritable);

        }

        mMemoryAddr = nativeOpen(mFd, mIsOwner);

        mCloseGuard.open("close");

    }
@@ -102,7 +92,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
     */

    public boolean isWritable() {

        enforceNotClosed();

        return isOwner() || mClientWritable;

        return mIsOwner;

    }



    /**
@@ -115,7 +105,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
    public int get(int index) throws IOException {

        enforceNotClosed();

        enforceValidIndex(index);

        return nativeGet(mFd, mMemoryAddr, index, isOwner());

        return nativeGet(mFd, mMemoryAddr, index);

    }



    /**
@@ -131,7 +121,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
        enforceNotClosed();

        enforceWritable();

        enforceValidIndex(index);

        nativeSet(mFd, mMemoryAddr, index, value, isOwner());

        nativeSet(mFd, mMemoryAddr, index, value);

    }



    /**
@@ -152,7 +142,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
    @Override

    public void close() throws IOException {

        if (!isClosed()) {

            nativeClose(mFd, mMemoryAddr, isOwner());

            nativeClose(mFd, mMemoryAddr, mIsOwner);

            mFd = -1;

            mCloseGuard.close();

        }
@@ -184,11 +174,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
    public void writeToParcel(Parcel parcel, int flags) {

        ParcelFileDescriptor pfd = ParcelFileDescriptor.adoptFd(mFd);

        try {

            parcel.writeInt(mOwnerPid);

            parcel.writeInt(mClientWritable ? 1 : 0);

            // Don't let writing to a parcel to close our fd - plz

            parcel.writeParcelable(pfd, flags & ~Parcelable.PARCELABLE_WRITE_RETURN_VALUE);

            parcel.writeLong(mMemoryAddr);

        } finally {

            pfd.detachFd();

        }
@@ -214,10 +201,6 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
        return mFd;

    }



    private boolean isOwner() {

        return mOwnerPid == Process.myPid();

    }



    private void enforceNotClosed() {

        if (isClosed()) {

            throw new IllegalStateException("cannot interact with a closed instance");
@@ -239,10 +222,10 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
    }



    private native int nativeCreate(String name, int size);

    private native long nativeOpen(int fd, boolean owner, boolean writable);

    private native long nativeOpen(int fd, boolean owner);

    private native void nativeClose(int fd, long memoryAddr, boolean owner);

    private native int nativeGet(int fd, long memoryAddr, int index, boolean owner);

    private native void nativeSet(int fd, long memoryAddr, int index, int value, boolean owner);

    private native int nativeGet(int fd, long memoryAddr, int index);

    private native void nativeSet(int fd, long memoryAddr, int index, int value);

    private native int nativeSize(int fd);



    /**
@@ -259,8 +242,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { 
            try {

                return new MemoryIntArray(parcel);

            } catch (IOException ioe) {

                Log.e(TAG, "Error unparceling MemoryIntArray");

                return null;

                throw new IllegalArgumentException("Error unparceling MemoryIntArray");

            }

        }

core/jni/android_util_MemoryIntArray.cpp

+24 −8
Original line number Diff line number Diff line
@@ -54,7 +54,7 @@ static jint android_util_MemoryIntArray_create(JNIEnv* env, jobject clazz, jstri 
}



static jlong android_util_MemoryIntArray_open(JNIEnv* env, jobject clazz, jint fd,

    jboolean owner, jboolean writable)

    jboolean owner)

{

    if (fd < 0) {

        jniThrowException(env, "java/io/IOException", "bad file descriptor");
@@ -72,19 +72,35 @@ static jlong android_util_MemoryIntArray_open(JNIEnv* env, jobject clazz, jint f 
        return -1;

    }



    int protMode = (owner || writable) ? (PROT_READ | PROT_WRITE) : PROT_READ;

    // IMPORTANT: Ashmem allows the caller to change its size until

    // it is memory mapped for the first time which lazily creates

    // the underlying VFS file. So the size we get above may not

    // reflect the size of the underlying shared memory region. Therefore,

    // we first memory map to set the size in stone an verify if

    // the underlying ashmem region has the same size as the one we

    // memory mapped. This is critical as we use the underlying

    // ashmem size for boundary checks and memory unmapping.

    int protMode = owner ? (PROT_READ | PROT_WRITE) : PROT_READ;

    void* ashmemAddr = mmap(NULL, ashmemSize, protMode, MAP_SHARED, fd, 0);

    if (ashmemAddr == MAP_FAILED) {

        jniThrowException(env, "java/io/IOException", "cannot mmap ashmem");

        return -1;

    }



    // Check if the mapped size is the same as the ashmem region.

    int mmapedSize = ashmem_get_size_region(fd);

    if (mmapedSize != ashmemSize) {

        munmap(reinterpret_cast<void *>(ashmemAddr), ashmemSize);

        jniThrowException(env, "java/io/IOException", "bad file descriptor");

        return -1;

    }



    if (owner) {

        int size = ashmemSize / sizeof(std::atomic_int);

        new (ashmemAddr) std::atomic_int[size];

    }



    if (owner && !writable) {

    if (owner) {

        int setProtResult = ashmem_set_prot_region(fd, PROT_READ);

        if (setProtResult < 0) {

            jniThrowException(env, "java/io/IOException", "cannot set ashmem prot mode");
@@ -131,7 +147,7 @@ static void android_util_MemoryIntArray_close(JNIEnv* env, jobject clazz, jint f 
}



static jint android_util_MemoryIntArray_get(JNIEnv* env, jobject clazz,

        jint fd, jlong address, jint index, jboolean owner)

        jint fd, jlong address, jint index)

{

    if (fd < 0) {

        jniThrowException(env, "java/io/IOException", "bad file descriptor");
@@ -153,7 +169,7 @@ static jint android_util_MemoryIntArray_get(JNIEnv* env, jobject clazz, 
}



static void android_util_MemoryIntArray_set(JNIEnv* env, jobject clazz,

        jint fd, jlong address, jint index, jint newValue, jboolean owner)

        jint fd, jlong address, jint index, jint newValue)

{

    if (fd < 0) {

        jniThrowException(env, "java/io/IOException", "bad file descriptor");
@@ -195,10 +211,10 @@ static jint android_util_MemoryIntArray_size(JNIEnv* env, jobject clazz, jint fd 


static const JNINativeMethod methods[] = {

    {"nativeCreate",  "(Ljava/lang/String;I)I", (void*)android_util_MemoryIntArray_create},

    {"nativeOpen",  "(IZZ)J", (void*)android_util_MemoryIntArray_open},

    {"nativeOpen",  "(IZ)J", (void*)android_util_MemoryIntArray_open},

    {"nativeClose", "(IJZ)V", (void*)android_util_MemoryIntArray_close},

    {"nativeGet",  "(IJIZ)I", (void*)android_util_MemoryIntArray_get},

    {"nativeSet", "(IJIIZ)V", (void*) android_util_MemoryIntArray_set},

    {"nativeGet",  "(IJI)I", (void*)android_util_MemoryIntArray_get},

    {"nativeSet", "(IJII)V", (void*) android_util_MemoryIntArray_set},

    {"nativeSize", "(I)I", (void*) android_util_MemoryIntArray_size},

};

core/tests/utiltests/Android.mk

+2 −0
Original line number Diff line number Diff line
@@ -12,6 +12,8 @@ LOCAL_MODULE_TAGS := tests 
LOCAL_SRC_FILES := $(call all-java-files-under, src)

LOCAL_SRC_FILES += src/android/util/IRemoteMemoryIntArray.aidl



LOCAL_JNI_SHARED_LIBRARIES := libmemoryintarraytest libcutils libc++



LOCAL_STATIC_JAVA_LIBRARIES := \

    android-support-test \

    frameworks-base-testutils \

core/tests/utiltests/jni/Android.bp

0 → 100644
+27 −0
Original line number Diff line number Diff line 
// Copyright (C) 2016 The Android Open Source Project

//

// Licensed under the Apache License, Version 2.0 (the "License");

// you may not use this file except in compliance with the License.

// You may obtain a copy of the License at

//

//      http://www.apache.org/licenses/LICENSE-2.0

//

// Unless required by applicable law or agreed to in writing, software

// distributed under the License is distributed on an "AS IS" BASIS,

// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

// See the License for the specific language governing permissions and

// limitations under the License.



cc_library_shared {

    name: "libmemoryintarraytest",

    shared_libs: [

        "libcutils",

    ],

    clang: true,

    stl: "libc++",

    srcs: [

        "registration.cpp",

        "android_util_MemoryIntArrayTest.cpp",

    ],

    cflags: ["-Werror"],

}
 
 No newline at end of file