From 67a769e18ad53d599917d6ff984682418d9982f5 Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Wed, 12 Oct 2016 14:08:36 -0700 Subject: [PATCH 1/4] Remove the kick from config switches in UsbDeviceManager Configuration switches already kick the stack due to triggers in init scripts. The switch to none config is unnecessary Test: Manually change usb configurations Change-Id: I65e530b1fa46e4f0123cb951fdd3a20ab7589bc6 --- services/usb/java/com/android/server/usb/UsbDeviceManager.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/services/usb/java/com/android/server/usb/UsbDeviceManager.java b/services/usb/java/com/android/server/usb/UsbDeviceManager.java index 220626a8d9ea3..8e3ed5ac75dd0 100644 --- a/services/usb/java/com/android/server/usb/UsbDeviceManager.java +++ b/services/usb/java/com/android/server/usb/UsbDeviceManager.java @@ -529,9 +529,6 @@ public class UsbDeviceManager { mCurrentFunctions = functions; mCurrentFunctionsApplied = false; - // Kick the USB stack to close existing connections. - setUsbConfig(UsbManager.USB_FUNCTION_NONE); - // Set the new USB configuration. if (!setUsbConfig(functions)) { Slog.e(TAG, "Failed to switch USB config to " + functions); From 7a396be6d5ba8914933a54b5bfac25e118db0e9f Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Wed, 12 Oct 2016 15:49:32 -0700 Subject: [PATCH 2/4] Refactored setCurrentFunction and setUsbDataUnlocked into single method. This gets rid of an extraneous configuration change when going from adb to adb + file transfer as previously the config would have been reset once for functions and once for data unlocked. It also simplifies some of the code. Test: manually changing usb configurations Change-Id: Ica10a195338b2189db13113f44657393db110bee --- .../com/android/commands/svc/UsbCommand.java | 2 +- .../android/hardware/usb/IUsbManager.aidl | 14 ++-- .../java/android/hardware/usb/UsbManager.java | 26 +++----- .../server/connectivity/Tethering.java | 4 +- .../android/server/usb/UsbDeviceManager.java | 66 +++++++++---------- .../com/android/server/usb/UsbService.java | 10 +-- 6 files changed, 49 insertions(+), 73 deletions(-) diff --git a/cmds/svc/src/com/android/commands/svc/UsbCommand.java b/cmds/svc/src/com/android/commands/svc/UsbCommand.java index a6ef25fc4479d..4dcb05e4f85da 100644 --- a/cmds/svc/src/com/android/commands/svc/UsbCommand.java +++ b/cmds/svc/src/com/android/commands/svc/UsbCommand.java @@ -50,7 +50,7 @@ public class UsbCommand extends Svc.Command { IUsbManager usbMgr = IUsbManager.Stub.asInterface(ServiceManager.getService( Context.USB_SERVICE)); try { - usbMgr.setCurrentFunction((args.length >=3 ? args[2] : null)); + usbMgr.setCurrentFunction((args.length >=3 ? args[2] : null), false); } catch (RemoteException e) { System.err.println("Error communicating with UsbManager: " + e); } diff --git a/core/java/android/hardware/usb/IUsbManager.aidl b/core/java/android/hardware/usb/IUsbManager.aidl index fafe116df606f..45aeb4acb3b0a 100644 --- a/core/java/android/hardware/usb/IUsbManager.aidl +++ b/core/java/android/hardware/usb/IUsbManager.aidl @@ -88,15 +88,13 @@ interface IUsbManager /* Returns true if the specified USB function is enabled. */ boolean isFunctionEnabled(String function); - /* Sets the current USB function. */ - void setCurrentFunction(String function); - - /* Sets whether USB data (for example, MTP exposed pictures) should be made - * available on the USB connection. Unlocking data should only be done with - * user involvement, since exposing pictures or other data could leak sensitive - * user information. + /* Sets the current USB function as well as whether USB data + * (for example, MTP exposed pictures) should be made available + * on the USB connection. Unlocking data should only be done with + * user involvement, since exposing pictures or other data could + * leak sensitive user information. */ - void setUsbDataUnlocked(boolean unlock); + void setCurrentFunction(String function, boolean usbDataUnlocked); /* Allow USB debugging from the attached host. If alwaysAllow is true, add the * the public key to list of host keys that the user has approved. diff --git a/core/java/android/hardware/usb/UsbManager.java b/core/java/android/hardware/usb/UsbManager.java index 1d20c78cdc97c..3636e4edeb354 100644 --- a/core/java/android/hardware/usb/UsbManager.java +++ b/core/java/android/hardware/usb/UsbManager.java @@ -542,33 +542,23 @@ public class UsbManager { * {@link #USB_FUNCTION_MIDI}, {@link #USB_FUNCTION_MTP}, {@link #USB_FUNCTION_PTP}, * or {@link #USB_FUNCTION_RNDIS}. *

+ * Also sets whether USB data (for example, MTP exposed pictures) should be made available + * on the USB connection when in device mode. Unlocking usb data should only be done with + * user involvement, since exposing pictures or other data could leak sensitive + * user information. + *

* Note: This function is asynchronous and may fail silently without applying * the requested changes. *

* * @param function name of the USB function, or null to restore the default function + * @param usbDataUnlocked whether user data is accessible * * {@hide} */ - public void setCurrentFunction(String function) { + public void setCurrentFunction(String function, boolean usbDataUnlocked) { try { - mService.setCurrentFunction(function); - } catch (RemoteException e) { - throw e.rethrowFromSystemServer(); - } - } - - /** - * Sets whether USB data (for example, MTP exposed pictures) should be made available - * on the USB connection when in device mode. Unlocking usb data should only be done with - * user involvement, since exposing pictures or other data could leak sensitive - * user information. - * - * {@hide} - */ - public void setUsbDataUnlocked(boolean unlocked) { - try { - mService.setUsbDataUnlocked(unlocked); + mService.setCurrentFunction(function, usbDataUnlocked); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } diff --git a/services/core/java/com/android/server/connectivity/Tethering.java b/services/core/java/com/android/server/connectivity/Tethering.java index da9c5474b66bf..921fd23db82ff 100644 --- a/services/core/java/com/android/server/connectivity/Tethering.java +++ b/services/core/java/com/android/server/connectivity/Tethering.java @@ -897,7 +897,7 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering } } else { mUsbTetherRequested = true; - usbManager.setCurrentFunction(UsbManager.USB_FUNCTION_RNDIS); + usbManager.setCurrentFunction(UsbManager.USB_FUNCTION_RNDIS, false); } } else { final long ident = Binder.clearCallingIdentity(); @@ -907,7 +907,7 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering Binder.restoreCallingIdentity(ident); } if (mRndisEnabled) { - usbManager.setCurrentFunction(null); + usbManager.setCurrentFunction(null, false); } mUsbTetherRequested = false; } diff --git a/services/usb/java/com/android/server/usb/UsbDeviceManager.java b/services/usb/java/com/android/server/usb/UsbDeviceManager.java index 8e3ed5ac75dd0..679fb26775c5a 100644 --- a/services/usb/java/com/android/server/usb/UsbDeviceManager.java +++ b/services/usb/java/com/android/server/usb/UsbDeviceManager.java @@ -109,9 +109,8 @@ public class UsbDeviceManager { private static final int MSG_SYSTEM_READY = 3; private static final int MSG_BOOT_COMPLETED = 4; private static final int MSG_USER_SWITCHED = 5; - private static final int MSG_SET_USB_DATA_UNLOCKED = 6; - private static final int MSG_UPDATE_USER_RESTRICTIONS = 7; - private static final int MSG_UPDATE_HOST_STATE = 8; + private static final int MSG_UPDATE_USER_RESTRICTIONS = 6; + private static final int MSG_UPDATE_HOST_STATE = 7; private static final int AUDIO_MODE_SOURCE = 1; @@ -291,7 +290,7 @@ public class UsbDeviceManager { if (functions != null) { mAccessoryModeRequestTime = SystemClock.elapsedRealtime(); - setCurrentFunctions(functions); + setCurrentFunctions(functions, false); } } @@ -347,7 +346,7 @@ public class UsbDeviceManager { SystemProperties.get(USB_STATE_PROPERTY)); mAdbEnabled = UsbManager.containsFunction(getDefaultFunctions(), UsbManager.USB_FUNCTION_ADB); - setEnabledFunctions(null, false); + setEnabledFunctions(null, false, false); String state = FileUtils.readTextFile(new File(STATE_PATH), 0, null).trim(); updateState(state); @@ -379,6 +378,14 @@ public class UsbDeviceManager { sendMessage(m); } + public void sendMessage(int what, Object arg, boolean arg1) { + removeMessages(what); + Message m = Message.obtain(this, what); + m.obj = arg; + m.arg1 = (arg1 ? 1 : 0); + sendMessage(m); + } + public void updateState(String state) { int connected, configured; @@ -443,14 +450,6 @@ public class UsbDeviceManager { return waitForState(config); } - private void setUsbDataUnlocked(boolean enable) { - if (DEBUG) Slog.d(TAG, "setUsbDataUnlocked: " + enable); - mUsbDataUnlocked = enable; - updateUsbNotification(); - updateUsbStateBroadcastIfNeeded(); - setEnabledFunctions(mCurrentFunctions, true); - } - private void setAdbEnabled(boolean enable) { if (DEBUG) Slog.d(TAG, "setAdbEnabled: " + enable); if (enable != mAdbEnabled) { @@ -465,7 +464,7 @@ public class UsbDeviceManager { } // After persisting them use the lock-down aware function set - setEnabledFunctions(mCurrentFunctions, false); + setEnabledFunctions(mCurrentFunctions, false, mUsbDataUnlocked); updateAdbNotification(); } @@ -477,10 +476,16 @@ public class UsbDeviceManager { /** * Evaluates USB function policies and applies the change accordingly. */ - private void setEnabledFunctions(String functions, boolean forceRestart) { + private void setEnabledFunctions(String functions, boolean forceRestart, boolean usbDataUnlocked) { if (DEBUG) Slog.d(TAG, "setEnabledFunctions functions=" + functions + ", " + "forceRestart=" + forceRestart); + if (usbDataUnlocked != mUsbDataUnlocked) { + mUsbDataUnlocked = usbDataUnlocked; + updateUsbNotification(); + forceRestart = true; + } + // Try to set the enabled functions. final String oldFunctions = mCurrentFunctions; final boolean oldFunctionsApplied = mCurrentFunctionsApplied; @@ -580,7 +585,7 @@ public class UsbDeviceManager { // make sure accessory mode is off // and restore default functions Slog.d(TAG, "exited USB accessory mode"); - setEnabledFunctions(null, false); + setEnabledFunctions(null, false, false); if (mCurrentAccessory != null) { if (mBootCompleted) { @@ -713,10 +718,7 @@ public class UsbDeviceManager { case MSG_UPDATE_STATE: mConnected = (msg.arg1 == 1); mConfigured = (msg.arg2 == 1); - if (!mConnected) { - // When a disconnect occurs, relock access to sensitive user data - mUsbDataUnlocked = false; - } + updateUsbNotification(); updateAdbNotification(); if (UsbManager.containsFunction(mCurrentFunctions, @@ -724,7 +726,7 @@ public class UsbDeviceManager { updateCurrentAccessory(); } else if (!mConnected) { // restore defaults when USB is disconnected - setEnabledFunctions(null, false); + setEnabledFunctions(null, false, false); } if (mBootCompleted) { updateUsbStateBroadcastIfNeeded(); @@ -747,13 +749,10 @@ public class UsbDeviceManager { break; case MSG_SET_CURRENT_FUNCTIONS: String functions = (String)msg.obj; - setEnabledFunctions(functions, false); + setEnabledFunctions(functions, false, msg.arg1 == 1); break; case MSG_UPDATE_USER_RESTRICTIONS: - setEnabledFunctions(mCurrentFunctions, false); - break; - case MSG_SET_USB_DATA_UNLOCKED: - setUsbDataUnlocked(msg.arg1 == 1); + setEnabledFunctions(mCurrentFunctions, false, mUsbDataUnlocked); break; case MSG_SYSTEM_READY: updateUsbNotification(); @@ -781,8 +780,7 @@ public class UsbDeviceManager { Slog.v(TAG, "Current user switched to " + mCurrentUser + "; resetting USB host stack for MTP or PTP"); // avoid leaking sensitive data from previous user - mUsbDataUnlocked = false; - setEnabledFunctions(mCurrentFunctions, true); + setEnabledFunctions(mCurrentFunctions, true, false); } mCurrentUser = msg.arg1; } @@ -966,14 +964,10 @@ public class UsbDeviceManager { return UsbManager.containsFunction(SystemProperties.get(USB_CONFIG_PROPERTY), function); } - public void setCurrentFunctions(String functions) { - if (DEBUG) Slog.d(TAG, "setCurrentFunctions(" + functions + ")"); - mHandler.sendMessage(MSG_SET_CURRENT_FUNCTIONS, functions); - } - - public void setUsbDataUnlocked(boolean unlocked) { - if (DEBUG) Slog.d(TAG, "setUsbDataUnlocked(" + unlocked + ")"); - mHandler.sendMessage(MSG_SET_USB_DATA_UNLOCKED, unlocked); + public void setCurrentFunctions(String functions, boolean usbDataUnlocked) { + if (DEBUG) Slog.d(TAG, "setCurrentFunctions(" + functions + ", " + + usbDataUnlocked + ")"); + mHandler.sendMessage(MSG_SET_CURRENT_FUNCTIONS, functions, usbDataUnlocked); } private void readOemUsbOverrideConfig() { diff --git a/services/usb/java/com/android/server/usb/UsbService.java b/services/usb/java/com/android/server/usb/UsbService.java index c85b363494e8c..a87ac9e8a3df2 100644 --- a/services/usb/java/com/android/server/usb/UsbService.java +++ b/services/usb/java/com/android/server/usb/UsbService.java @@ -372,7 +372,7 @@ public class UsbService extends IUsbManager.Stub { } @Override - public void setCurrentFunction(String function) { + public void setCurrentFunction(String function, boolean usbDataUnlocked) { mContext.enforceCallingOrSelfPermission(android.Manifest.permission.MANAGE_USB, null); if (!isSupportedCurrentFunction(function)) { @@ -382,7 +382,7 @@ public class UsbService extends IUsbManager.Stub { } if (mDeviceManager != null) { - mDeviceManager.setCurrentFunctions(function); + mDeviceManager.setCurrentFunctions(function, usbDataUnlocked); } else { throw new IllegalStateException("USB device mode not supported"); } @@ -404,12 +404,6 @@ public class UsbService extends IUsbManager.Stub { return false; } - @Override - public void setUsbDataUnlocked(boolean unlocked) { - mContext.enforceCallingOrSelfPermission(android.Manifest.permission.MANAGE_USB, null); - mDeviceManager.setUsbDataUnlocked(unlocked); - } - @Override public void allowUsbDebugging(boolean alwaysAllow, String publicKey) { mContext.enforceCallingOrSelfPermission(android.Manifest.permission.MANAGE_USB, null); From 58018d01a3c384b954275d15bee7f9c52a1c7c0a Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Wed, 12 Oct 2016 18:04:03 -0700 Subject: [PATCH 3/4] Fixed handling of usb state during adb changes. When changing state to adb from mtp (charging), we want to disable the old mtp function. Similarly when change to away from adb to charging, enable the mtp function. Also the mtp function should never be persisted. Bug: 31818377 Bug: 31814300 Test: Manually verify that the correct usb configuration is displayed. Test: Manually verify that logcat is not kicked during boot. Change-Id: Idcb7f53be39ea38712d5de45b323d8daeb552129 --- .../android/server/usb/UsbDeviceManager.java | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/services/usb/java/com/android/server/usb/UsbDeviceManager.java b/services/usb/java/com/android/server/usb/UsbDeviceManager.java index 679fb26775c5a..e37eee0473b03 100644 --- a/services/usb/java/com/android/server/usb/UsbDeviceManager.java +++ b/services/usb/java/com/android/server/usb/UsbDeviceManager.java @@ -339,9 +339,6 @@ public class UsbDeviceManager { // Restore default functions. mCurrentFunctions = SystemProperties.get(USB_CONFIG_PROPERTY, UsbManager.USB_FUNCTION_NONE); - if (UsbManager.USB_FUNCTION_NONE.equals(mCurrentFunctions)) { - mCurrentFunctions = UsbManager.USB_FUNCTION_MTP; - } mCurrentFunctionsApplied = mCurrentFunctions.equals( SystemProperties.get(USB_STATE_PROPERTY)); mAdbEnabled = UsbManager.containsFunction(getDefaultFunctions(), @@ -454,17 +451,25 @@ public class UsbDeviceManager { if (DEBUG) Slog.d(TAG, "setAdbEnabled: " + enable); if (enable != mAdbEnabled) { mAdbEnabled = enable; + String oldFunctions = mCurrentFunctions; - // Due to the persist.sys.usb.config property trigger, changing adb state requires - // persisting default function - String oldFunctions = getDefaultFunctions(); - String newFunctions = applyAdbFunction(oldFunctions); - if (!oldFunctions.equals(newFunctions)) { - SystemProperties.set(USB_PERSISTENT_CONFIG_PROPERTY, newFunctions); + // Persist the adb setting + String newFunction = enable ? UsbManager.USB_FUNCTION_ADB + : UsbManager.USB_FUNCTION_NONE; + if (!UsbManager.containsFunction(getDefaultFunctions(), newFunction)) + SystemProperties.set(USB_PERSISTENT_CONFIG_PROPERTY, newFunction); + + // Changing the persistent config also changes the normal + // config. Wait for this to happen before changing again. + waitForState(newFunction); + + // Remove mtp from the config if file transfer is not enabled + if (oldFunctions.equals(UsbManager.USB_FUNCTION_MTP) && + !mUsbDataUnlocked && enable) { + oldFunctions = UsbManager.USB_FUNCTION_NONE; } - // After persisting them use the lock-down aware function set - setEnabledFunctions(mCurrentFunctions, false, mUsbDataUnlocked); + setEnabledFunctions(oldFunctions, false, mUsbDataUnlocked); updateAdbNotification(); } @@ -476,7 +481,8 @@ public class UsbDeviceManager { /** * Evaluates USB function policies and applies the change accordingly. */ - private void setEnabledFunctions(String functions, boolean forceRestart, boolean usbDataUnlocked) { + private void setEnabledFunctions(String functions, boolean forceRestart, + boolean usbDataUnlocked) { if (DEBUG) Slog.d(TAG, "setEnabledFunctions functions=" + functions + ", " + "forceRestart=" + forceRestart); @@ -522,7 +528,8 @@ public class UsbDeviceManager { } private boolean trySetEnabledFunctions(String functions, boolean forceRestart) { - if (functions == null) { + if (functions == null || applyAdbFunction(functions) + .equals(UsbManager.USB_FUNCTION_NONE)) { functions = getDefaultFunctions(); } functions = applyAdbFunction(functions); @@ -602,10 +609,6 @@ public class UsbDeviceManager { if (mBroadcastedIntent == null) { for (String key : keySet) { if (intent.getBooleanExtra(key, false)) { - // MTP function is enabled by default. - if (UsbManager.USB_FUNCTION_MTP.equals(key)) { - continue; - } return true; } } From 069c34894531e6b69bd4d5ed9bd8b9ef4b297cd1 Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Thu, 13 Oct 2016 17:17:06 -0700 Subject: [PATCH 4/4] Automatically turn on adb for userdebug and eng builds. Test: flash on a userdebug build, verify adb is on. Change-Id: If9a46ca2291c034839ec1d297d20e4e6e3fefd77 --- .../android/server/usb/UsbDeviceManager.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/services/usb/java/com/android/server/usb/UsbDeviceManager.java b/services/usb/java/com/android/server/usb/UsbDeviceManager.java index e37eee0473b03..a0c6e0ee5b28f 100644 --- a/services/usb/java/com/android/server/usb/UsbDeviceManager.java +++ b/services/usb/java/com/android/server/usb/UsbDeviceManager.java @@ -83,6 +83,14 @@ public class UsbDeviceManager { */ private static final String USB_CONFIG_PROPERTY = "sys.usb.config"; + /** + * The property which stores the current build type (user/userdebug/eng). + */ + private static final String BUILD_TYPE_PROPERTY = "ro.build.type"; + + private static final String BUILD_TYPE_USERDEBUG = "userdebug"; + private static final String BUILD_TYPE_ENG = "eng"; + /** * The non-persistent property which stores the current USB actual state. */ @@ -343,6 +351,12 @@ public class UsbDeviceManager { SystemProperties.get(USB_STATE_PROPERTY)); mAdbEnabled = UsbManager.containsFunction(getDefaultFunctions(), UsbManager.USB_FUNCTION_ADB); + + String buildType = SystemProperties.get(BUILD_TYPE_PROPERTY); + if (buildType.equals(BUILD_TYPE_USERDEBUG) || buildType.equals(BUILD_TYPE_ENG)) { + setAdbEnabled(true); + } + setEnabledFunctions(null, false, false); String state = FileUtils.readTextFile(new File(STATE_PATH), 0, null).trim(); @@ -454,10 +468,9 @@ public class UsbDeviceManager { String oldFunctions = mCurrentFunctions; // Persist the adb setting - String newFunction = enable ? UsbManager.USB_FUNCTION_ADB - : UsbManager.USB_FUNCTION_NONE; - if (!UsbManager.containsFunction(getDefaultFunctions(), newFunction)) - SystemProperties.set(USB_PERSISTENT_CONFIG_PROPERTY, newFunction); + String newFunction = applyAdbFunction(SystemProperties.get( + USB_PERSISTENT_CONFIG_PROPERTY, UsbManager.USB_FUNCTION_NONE)); + SystemProperties.set(USB_PERSISTENT_CONFIG_PROPERTY, newFunction); // Changing the persistent config also changes the normal // config. Wait for this to happen before changing again.