From 5e8544685d92516176f2a24cacb424cb6edbb7e3 Mon Sep 17 00:00:00 2001 From: Nathan Harold Date: Wed, 13 Dec 2017 18:51:35 -0800 Subject: [PATCH 1/2] IpSec Invalidate the Resource IDs on close() To facilitate error checking we should invalidate the resource IDs of all objects when we close() them. Today, the resource ID is invalidated on the Transform object but not on the SPI or Encap Socket. This CL unifies the behavior. Bug: 70641274 Test: cts - IpSecManagerTest Change-Id: I28caec3e913902c748c6a50b4ef742ccef8b1b09 --- core/java/android/net/IpSecConfig.java | 6 ------ core/java/android/net/IpSecManager.java | 8 +++++--- core/java/android/net/IpSecTransform.java | 17 +++++++++++++++-- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/core/java/android/net/IpSecConfig.java b/core/java/android/net/IpSecConfig.java index e6cd3fc130..f54ceb5c14 100644 --- a/core/java/android/net/IpSecConfig.java +++ b/core/java/android/net/IpSecConfig.java @@ -102,17 +102,11 @@ public final class IpSecConfig implements Parcelable { /** Set the local IP address for Tunnel mode */ public void setLocalAddress(String localAddress) { - if (localAddress == null) { - throw new IllegalArgumentException("localAddress may not be null!"); - } mLocalAddress = localAddress; } /** Set the remote IP address for this IPsec transform */ public void setRemoteAddress(String remoteAddress) { - if (remoteAddress == null) { - throw new IllegalArgumentException("remoteAddress may not be null!"); - } mRemoteAddress = remoteAddress; } diff --git a/core/java/android/net/IpSecManager.java b/core/java/android/net/IpSecManager.java index 6a4b891478..34cfa9b215 100644 --- a/core/java/android/net/IpSecManager.java +++ b/core/java/android/net/IpSecManager.java @@ -69,7 +69,7 @@ public final class IpSecManager { } /** @hide */ - public static final int INVALID_RESOURCE_ID = 0; + public static final int INVALID_RESOURCE_ID = -1; /** * Thrown to indicate that a requested SPI is in use. @@ -128,7 +128,7 @@ public final class IpSecManager { private final InetAddress mRemoteAddress; private final CloseGuard mCloseGuard = CloseGuard.get(); private int mSpi = INVALID_SECURITY_PARAMETER_INDEX; - private int mResourceId; + private int mResourceId = INVALID_RESOURCE_ID; /** Get the underlying SPI held by this object. */ public int getSpi() { @@ -146,6 +146,7 @@ public final class IpSecManager { public void close() { try { mService.releaseSecurityParameterIndex(mResourceId); + mResourceId = INVALID_RESOURCE_ID; } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } @@ -501,7 +502,7 @@ public final class IpSecManager { public static final class UdpEncapsulationSocket implements AutoCloseable { private final ParcelFileDescriptor mPfd; private final IIpSecService mService; - private final int mResourceId; + private int mResourceId = INVALID_RESOURCE_ID; private final int mPort; private final CloseGuard mCloseGuard = CloseGuard.get(); @@ -554,6 +555,7 @@ public final class IpSecManager { public void close() throws IOException { try { mService.closeUdpEncapsulationSocket(mResourceId); + mResourceId = INVALID_RESOURCE_ID; } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } diff --git a/core/java/android/net/IpSecTransform.java b/core/java/android/net/IpSecTransform.java index 7cd742b417..102ba6d94f 100644 --- a/core/java/android/net/IpSecTransform.java +++ b/core/java/android/net/IpSecTransform.java @@ -347,6 +347,9 @@ public final class IpSecTransform implements AutoCloseable { */ public IpSecTransform.Builder setSpi( @TransformDirection int direction, IpSecManager.SecurityParameterIndex spi) { + if (spi.getResourceId() == INVALID_RESOURCE_ID) { + throw new IllegalArgumentException("Invalid SecurityParameterIndex"); + } mConfig.setSpiResourceId(direction, spi.getResourceId()); return this; } @@ -381,6 +384,9 @@ public final class IpSecTransform implements AutoCloseable { public IpSecTransform.Builder setIpv4Encapsulation( IpSecManager.UdpEncapsulationSocket localSocket, int remotePort) { mConfig.setEncapType(ENCAP_ESPINUDP); + if (localSocket.getResourceId() == INVALID_RESOURCE_ID) { + throw new IllegalArgumentException("Invalid UdpEncapsulationSocket"); + } mConfig.setEncapSocketResourceId(localSocket.getResourceId()); mConfig.setEncapRemotePort(remotePort); return this; @@ -426,6 +432,9 @@ public final class IpSecTransform implements AutoCloseable { public IpSecTransform buildTransportModeTransform(InetAddress remoteAddress) throws IpSecManager.ResourceUnavailableException, IpSecManager.SpiUnavailableException, IOException { + if (remoteAddress == null) { + throw new IllegalArgumentException("Remote address may not be null or empty!"); + } mConfig.setMode(MODE_TRANSPORT); mConfig.setRemoteAddress(remoteAddress.getHostAddress()); // FIXME: modifying a builder after calling build can change the built transform. @@ -447,8 +456,12 @@ public final class IpSecTransform implements AutoCloseable { */ public IpSecTransform buildTunnelModeTransform( InetAddress localAddress, InetAddress remoteAddress) { - // FIXME: argument validation here - // throw new IllegalArgumentException("Natt Keepalive requires UDP Encapsulation"); + if (localAddress == null) { + throw new IllegalArgumentException("Local address may not be null or empty!"); + } + if (remoteAddress == null) { + throw new IllegalArgumentException("Remote address may not be null or empty!"); + } mConfig.setLocalAddress(localAddress.getHostAddress()); mConfig.setRemoteAddress(remoteAddress.getHostAddress()); mConfig.setMode(MODE_TUNNEL); From fdafce24cfa6122a0da2feff7bb5f26ea993d408 Mon Sep 17 00:00:00 2001 From: Nathan Harold Date: Wed, 13 Dec 2017 19:16:33 -0800 Subject: [PATCH 2/2] IpSecService - Convert mNextResourceId from AtomicInt to Int The mNextResourceId variable is only accessed within synchronized blocks, so there is no need to use an atomic integer to synchronize it. This eliminates the misleading notion that the variable is accessed outside of guarded blocks, which it is not. Bug: 62279167 Test: cts Change-Id: I815835622659f54d2d2d33b349b17c632ebced8d --- .../java/com/android/server/IpSecService.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index 989cb886c1..5c098e3204 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -102,8 +102,14 @@ public class IpSecService extends IIpSecService.Stub { /* Binder context for this service */ private final Context mContext; - /** Should be a never-repeating global ID for resources */ - private static AtomicInteger mNextResourceId = new AtomicInteger(0x00FADED0); + /** + * The next non-repeating global ID for tracking resources between users, this service, + * and kernel data structures. Accessing this variable is not thread safe, so it is + * only read or modified within blocks synchronized on IpSecService.this. We want to + * avoid -1 (INVALID_RESOURCE_ID) and 0 (we probably forgot to initialize it). + */ + @GuardedBy("IpSecService.this") + private int mNextResourceId = 1; interface IpSecServiceConfiguration { INetd getNetdInstance() throws RemoteException; @@ -856,7 +862,7 @@ public class IpSecService extends IIpSecService.Stub { checkNotNull(binder, "Null Binder passed to allocateSecurityParameterIndex"); UserRecord userRecord = mUserResourceTracker.getUserRecord(Binder.getCallingUid()); - int resourceId = mNextResourceId.getAndIncrement(); + final int resourceId = mNextResourceId++; int spi = IpSecManager.INVALID_SECURITY_PARAMETER_INDEX; String localAddress = ""; @@ -979,7 +985,7 @@ public class IpSecService extends IIpSecService.Stub { int callingUid = Binder.getCallingUid(); UserRecord userRecord = mUserResourceTracker.getUserRecord(callingUid); - int resourceId = mNextResourceId.getAndIncrement(); + final int resourceId = mNextResourceId++; FileDescriptor sockFd = null; try { if (!userRecord.mSocketQuotaTracker.isAvailable()) { @@ -1102,7 +1108,7 @@ public class IpSecService extends IIpSecService.Stub { IpSecConfig c, IBinder binder) throws RemoteException { checkIpSecConfig(c); checkNotNull(binder, "Null Binder passed to createTransportModeTransform"); - int resourceId = mNextResourceId.getAndIncrement(); + final int resourceId = mNextResourceId++; UserRecord userRecord = mUserResourceTracker.getUserRecord(Binder.getCallingUid());