From 0269ebe0fe86d9fbe2aaa624fbe96d340b58c04b Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Wed, 12 Oct 2016 18:04:03 -0700 Subject: [PATCH 1/3] 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 (cherry-picked from commit 58018d01a3c384b954275d15bee7f9c52a1c7c0a) --- .../android/server/usb/UsbDeviceManager.java | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/services/usb/java/com/android/server/usb/UsbDeviceManager.java b/services/usb/java/com/android/server/usb/UsbDeviceManager.java index df9242dc0aa12..fb2477b1754da 100644 --- a/services/usb/java/com/android/server/usb/UsbDeviceManager.java +++ b/services/usb/java/com/android/server/usb/UsbDeviceManager.java @@ -335,14 +335,12 @@ 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(), UsbManager.USB_FUNCTION_ADB); - setEnabledFunctions(null, false); + + setEnabledFunctions(null, false, false); String state = FileUtils.readTextFile(new File(STATE_PATH), 0, null).trim(); updateState(state); @@ -439,17 +437,24 @@ 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 = 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. + 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); + setEnabledFunctions(oldFunctions, false, mUsbDataUnlocked); updateAdbNotification(); } @@ -461,7 +466,8 @@ 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); @@ -501,7 +507,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); @@ -583,10 +590,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 dfacad026dfc1582504969f8b268e7c3970c4496 Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Mon, 17 Oct 2016 17:37:13 -0700 Subject: [PATCH 2/3] Clean up persistent usb state on boot. b/31814300 was fixed, but mtp can still stick around in the persistent config even after flashing. This block of code will only run once, but will ensure that mtp is not in the config after the update. Bug: 31814300 Test: Manual Change-Id: Icf02be38c9e1f769412ac963ed6afc14e6092bfb (cherry-picked from commit a45dac0e83f4f907b6b42f453181a7d5c01f65f3) --- .../com/android/server/usb/UsbDeviceManager.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/services/usb/java/com/android/server/usb/UsbDeviceManager.java b/services/usb/java/com/android/server/usb/UsbDeviceManager.java index fb2477b1754da..6f68bbb3e37d7 100644 --- a/services/usb/java/com/android/server/usb/UsbDeviceManager.java +++ b/services/usb/java/com/android/server/usb/UsbDeviceManager.java @@ -340,6 +340,16 @@ public class UsbDeviceManager { mAdbEnabled = UsbManager.containsFunction(getDefaultFunctions(), UsbManager.USB_FUNCTION_ADB); + /** + * Remove MTP from persistent config, to bring usb to a good state + * after fixes to b/31814300. This block can be removed after the update + */ + String persisted = SystemProperties.get(USB_PERSISTENT_CONFIG_PROPERTY); + if (UsbManager.containsFunction(persisted, UsbManager.USB_FUNCTION_MTP)) { + SystemProperties.set(USB_PERSISTENT_CONFIG_PROPERTY, + UsbManager.removeFunction(persisted, UsbManager.USB_FUNCTION_MTP)); + } + setEnabledFunctions(null, false, false); String state = FileUtils.readTextFile(new File(STATE_PATH), 0, null).trim(); @@ -444,17 +454,13 @@ public class UsbDeviceManager { 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. - 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; } - setEnabledFunctions(oldFunctions, false, mUsbDataUnlocked); + setEnabledFunctions(oldFunctions, true, mUsbDataUnlocked); updateAdbNotification(); } From 935372bfd642f15afc6488f7df100f1bf76fc773 Mon Sep 17 00:00:00 2001 From: Jerry Zhang Date: Wed, 12 Oct 2016 15:49:32 -0700 Subject: [PATCH 3/3] 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 (cherry picked from commit 7a396be6d5ba8914933a54b5bfac25e118db0e9f) --- .../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 | 60 +++++++++---------- .../com/android/server/usb/UsbService.java | 10 +--- 6 files changed, 46 insertions(+), 70 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 6e4c9de1c8ef7..00b0bffdd1f53 100644 --- a/core/java/android/hardware/usb/IUsbManager.aidl +++ b/core/java/android/hardware/usb/IUsbManager.aidl @@ -87,15 +87,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 f9a7d192b5bb3..6341cbc79f3f0 100644 --- a/core/java/android/hardware/usb/UsbManager.java +++ b/core/java/android/hardware/usb/UsbManager.java @@ -509,33 +509,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 e888faaac0752..cec4141a41f6e 100644 --- a/services/core/java/com/android/server/connectivity/Tethering.java +++ b/services/core/java/com/android/server/connectivity/Tethering.java @@ -900,7 +900,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(); @@ -910,7 +910,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 6f68bbb3e37d7..43b8fa51a80e6 100644 --- a/services/usb/java/com/android/server/usb/UsbDeviceManager.java +++ b/services/usb/java/com/android/server/usb/UsbDeviceManager.java @@ -108,9 +108,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; @@ -287,7 +286,7 @@ public class UsbDeviceManager { if (functions != null) { mAccessoryModeRequestTime = SystemClock.elapsedRealtime(); - setCurrentFunctions(functions); + setCurrentFunctions(functions, false); } } @@ -382,6 +381,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; @@ -435,14 +442,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) { @@ -477,6 +476,12 @@ public class UsbDeviceManager { 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; @@ -579,7 +584,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) { @@ -708,10 +713,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, @@ -719,7 +721,7 @@ public class UsbDeviceManager { updateCurrentAccessory(); } else if (!mConnected) { // restore defaults when USB is disconnected - setEnabledFunctions(null, false); + setEnabledFunctions(null, false, false); } if (mBootCompleted) { updateUsbStateBroadcastIfNeeded(); @@ -739,13 +741,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(); @@ -773,8 +772,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; } @@ -953,14 +951,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 d6dbe90584f51..daccc00ad80c5 100644 --- a/services/usb/java/com/android/server/usb/UsbService.java +++ b/services/usb/java/com/android/server/usb/UsbService.java @@ -287,7 +287,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)) { @@ -297,7 +297,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"); } @@ -319,12 +319,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);