[RESTRICT AUTOMERGE] Add protections agains use-after-free issues if cancel() or queue() is called after a device connection has been closed. am: 7a8d56b2fe

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/20234866

Change-Id: Ib21057b8df8c9d5ad4719de60e9db7fb88765046
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

core/java/android/hardware/usb/UsbDeviceConnection.java

@@ -52,6 +52,8 @@ public class UsbDeviceConnection {
 
     private final CloseGuard mCloseGuard = CloseGuard.get();
 
+    private final Object mLock = new Object();
+
     /**
      * UsbDevice should only be instantiated by UsbService implementation
      * @hide
@@ -62,13 +64,23 @@ public class UsbDeviceConnection {
 
     /* package */ boolean open(String name, ParcelFileDescriptor pfd, @NonNull Context context) {
         mContext = context.getApplicationContext();
-        boolean wasOpened = native_open(name, pfd.getFileDescriptor());
 
-        if (wasOpened) {
+        synchronized (mLock) {
-            mCloseGuard.open("close");
+            boolean wasOpened = native_open(name, pfd.getFileDescriptor());
+
+            if (wasOpened) {
+                mCloseGuard.open("close");
+            }
+
+            return wasOpened;
         }
+    }
 
-        return wasOpened;
+    /***
+     * @return If this connection is currently open and usable.
+     */
+    boolean isOpen() {
+        return mNativeContext != 0;
     }
 
     /**
@@ -80,6 +92,49 @@ public class UsbDeviceConnection {
         return mContext;
     }
 
+    /**
+     * Cancel a request which relates to this connection.
+     *
+     * @return true if the request was successfully cancelled.
+     */
+    /* package */ boolean cancelRequest(UsbRequest request) {
+        synchronized (mLock) {
+            if (!isOpen()) {
+                return false;
+            }
+
+            return request.cancelIfOpen();
+        }
+    }
+
+    /**
+     * This is meant to be called by UsbRequest's queue() in order to synchronize on
+     * UsbDeviceConnection's mLock to prevent the connection being closed while queueing.
+     */
+    /* package */ boolean queueRequest(UsbRequest request, ByteBuffer buffer, int length) {
+        synchronized (mLock) {
+            if (!isOpen()) {
+                return false;
+            }
+
+            return request.queueIfConnectionOpen(buffer, length);
+        }
+    }
+
+    /**
+     * This is meant to be called by UsbRequest's queue() in order to synchronize on
+     * UsbDeviceConnection's mLock to prevent the connection being closed while queueing.
+     */
+    /* package */ boolean queueRequest(UsbRequest request, @Nullable ByteBuffer buffer) {
+        synchronized (mLock) {
+            if (!isOpen()) {
+                return false;
+            }
+
+            return request.queueIfConnectionOpen(buffer);
+        }
+    }
+
     /**
      * Releases all system resources related to the device.
      * Once the object is closed it cannot be used again.
@@ -87,9 +142,11 @@ public class UsbDeviceConnection {
      * to retrieve a new instance to reestablish communication with the device.
      */
     public void close() {
-        if (mNativeContext != 0) {
+        synchronized (mLock) {
-            native_close();
+            if (isOpen()) {
-            mCloseGuard.close();
+                native_close();
+                mCloseGuard.close();
+            }
         }
     }
 

core/java/android/hardware/usb/UsbRequest.java

@@ -112,11 +112,13 @@ public class UsbRequest {
      * Releases all resources related to this request.
      */
     public void close() {
-        if (mNativeContext != 0) {
+        synchronized (mLock) {
-            mEndpoint = null;
+            if (mNativeContext != 0) {
-            mConnection = null;
+                mEndpoint = null;
-            native_close();
+                mConnection = null;
-            mCloseGuard.close();
+                native_close();
+                mCloseGuard.close();
+            }
         }
     }
 
@@ -190,10 +192,32 @@ public class UsbRequest {
      */
     @Deprecated
     public boolean queue(ByteBuffer buffer, int length) {
+        UsbDeviceConnection connection = mConnection;
+        if (connection == null) {
+            // The expected exception by CTS Verifier - USB Device test
+            throw new NullPointerException("invalid connection");
+        }
+
+        // Calling into the underlying UsbDeviceConnection to synchronize on its lock, to prevent
+        // the connection being closed while queueing.
+        return connection.queueRequest(this, buffer, length);
+    }
+
+    /**
+     * This is meant to be called from UsbDeviceConnection after synchronizing using the lock over
+     * there, to prevent the connection being closed while queueing.
+     */
+    /* package */ boolean queueIfConnectionOpen(ByteBuffer buffer, int length) {
+        UsbDeviceConnection connection = mConnection;
+        if (connection == null || !connection.isOpen()) {
+            // The expected exception by CTS Verifier - USB Device test
+            throw new NullPointerException("invalid connection");
+        }
+
         boolean out = (mEndpoint.getDirection() == UsbConstants.USB_DIR_OUT);
         boolean result;
 
-        if (mConnection.getContext().getApplicationInfo().targetSdkVersion < Build.VERSION_CODES.P
+        if (connection.getContext().getApplicationInfo().targetSdkVersion < Build.VERSION_CODES.P
                 && length > MAX_USBFS_BUFFER_SIZE) {
             length = MAX_USBFS_BUFFER_SIZE;
         }
@@ -242,6 +266,28 @@ public class UsbRequest {
      * @return true if the queueing operation succeeded
      */
     public boolean queue(@Nullable ByteBuffer buffer) {
+        UsbDeviceConnection connection = mConnection;
+        if (connection == null) {
+            // The expected exception by CTS Verifier - USB Device test
+            throw new IllegalStateException("invalid connection");
+        }
+
+        // Calling into the underlying UsbDeviceConnection to synchronize on its lock, to prevent
+        // the connection being closed while queueing.
+        return connection.queueRequest(this, buffer);
+    }
+
+    /**
+     * This is meant to be called from UsbDeviceConnection after synchronizing using the lock over
+     * there, to prevent the connection being closed while queueing.
+     */
+    /* package */ boolean queueIfConnectionOpen(@Nullable ByteBuffer buffer) {
+        UsbDeviceConnection connection = mConnection;
+        if (connection == null || !connection.isOpen()) {
+            // The expected exception by CTS Verifier - USB Device test
+            throw new IllegalStateException("invalid connection");
+        }
+
         // Request need to be initialized
         Preconditions.checkState(mNativeContext != 0, "request is not initialized");
 
@@ -259,7 +305,7 @@ public class UsbRequest {
                 mIsUsingNewQueue = true;
                 wasQueued = native_queue(null, 0, 0);
             } else {
-                if (mConnection.getContext().getApplicationInfo().targetSdkVersion
+                if (connection.getContext().getApplicationInfo().targetSdkVersion
                         < Build.VERSION_CODES.P) {
                     // Can only send/receive MAX_USBFS_BUFFER_SIZE bytes at once
                     Preconditions.checkArgumentInRange(buffer.remaining(), 0, MAX_USBFS_BUFFER_SIZE,
@@ -362,6 +408,32 @@ public class UsbRequest {
      * @return true if cancelling succeeded
      */
     public boolean cancel() {
+        UsbDeviceConnection connection = mConnection;
+        if (connection == null) {
+            return false;
+        }
+
+        return connection.cancelRequest(this);
+    }
+
+    /**
+     * Cancels a pending queue operation (for use when the UsbDeviceConnection associated
+     * with this request is synchronized). This ensures we don't have a race where the
+     * device is closed and then the request is canceled which would lead to a
+     * use-after-free because the cancel operation uses the device connection
+     * information freed in the when UsbDeviceConnection is closed.<br/>
+     *
+     * This method assumes the connected is not closed while this method is executed.
+     *
+     * @return true if cancelling succeeded.
+     */
+    /* package */ boolean cancelIfOpen() {
+        UsbDeviceConnection connection = mConnection;
+        if (mNativeContext == 0 || (connection != null && !connection.isOpen())) {
+            Log.w(TAG,
+                    "Detected attempt to cancel a request on a connection which isn't open");
+            return false;
+        }
         return native_cancel();
     }