commit 22795302be4ec35449908cf566aa7c16945df836
author Nathan Harold <nharold@google.com> Tue Feb 27 19:19:40 2018 -0800
committer Nathan Harold <nharold@google.com> Tue Mar 20 11:26:29 2018 -0700
tree 246c4a0b96cfd28a4386cea4cb2a2b7c65bb5bff
parent 448003fc491d191e3d0f6fc96ee67293f7b1dc8e

Check mOwnedByTransform to avoid DELSA on SPI

The owned by transform flag prevents the removal
of an SPI from accidentally deleting an associated
SA in the kernel. That flag wasn't actually being
checked, so deleting an SPI would result in the
transform being removed.

The existing code already guarantees that the SA is
deleted when the transform is deleted

Bug: 73258845
Test: runtest frameworks-net
Change-Id: I4c26aea7af817a5d9e54da5db1cdf4f943bcae06

services/core/java/com/android/server/IpSecService.java

diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java
index 45e9481..89f599b 100644
--- a/services/core/java/com/android/server/IpSecService.java
+++ b/services/core/java/com/android/server/IpSecService.java
@@ -676,10 +676,12 @@
         @Override
         public void freeUnderlyingResources() {
             try {
-                mSrvConfig
-                        .getNetdInstance()
-                        .ipSecDeleteSecurityAssociation(
-                                mResourceId, mSourceAddress, mDestinationAddress, mSpi, 0, 0);
+                if (!mOwnedByTransform) {
+                    mSrvConfig
+                            .getNetdInstance()
+                            .ipSecDeleteSecurityAssociation(
+                                    mResourceId, mSourceAddress, mDestinationAddress, mSpi, 0, 0);
+                }
             } catch (ServiceSpecificException | RemoteException e) {
                 Log.e(TAG, "Failed to delete SPI reservation with ID: " + mResourceId, e);
             }

tests/net/java/com/android/server/IpSecServiceParameterizedTest.java

diff --git a/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java b/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java
index a5c55e8..410f754 100644
--- a/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java
+++ b/tests/net/java/com/android/server/IpSecServiceParameterizedTest.java
@@ -23,6 +23,7 @@
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -320,6 +321,30 @@
     }
 
     @Test
+    public void testReleaseOwnedSpi() throws Exception {
+        IpSecConfig ipSecConfig = new IpSecConfig();
+        addDefaultSpisAndRemoteAddrToIpSecConfig(ipSecConfig);
+        addAuthAndCryptToIpSecConfig(ipSecConfig);
+
+        IpSecTransformResponse createTransformResp =
+                mIpSecService.createTransform(ipSecConfig, new Binder());
+        IpSecService.UserRecord userRecord =
+                mIpSecService.mUserResourceTracker.getUserRecord(Os.getuid());
+        assertEquals(1, userRecord.mSpiQuotaTracker.mCurrent);
+        mIpSecService.releaseSecurityParameterIndex(ipSecConfig.getSpiResourceId());
+        verify(mMockNetd, times(0))
+                .ipSecDeleteSecurityAssociation(
+                        eq(createTransformResp.resourceId),
+                        anyString(),
+                        anyString(),
+                        eq(TEST_SPI),
+                        anyInt(),
+                        anyInt());
+        // quota is not released until the SPI is released by the Transform
+        assertEquals(1, userRecord.mSpiQuotaTracker.mCurrent);
+    }
+
+    @Test
     public void testDeleteTransform() throws Exception {
         IpSecConfig ipSecConfig = new IpSecConfig();
         addDefaultSpisAndRemoteAddrToIpSecConfig(ipSecConfig);
@@ -329,7 +354,7 @@
                 mIpSecService.createTransform(ipSecConfig, new Binder());
         mIpSecService.deleteTransform(createTransformResp.resourceId);
 
-        verify(mMockNetd)
+        verify(mMockNetd, times(1))
                 .ipSecDeleteSecurityAssociation(
                         eq(createTransformResp.resourceId),
                         anyString(),
@@ -342,6 +367,21 @@
         IpSecService.UserRecord userRecord =
                 mIpSecService.mUserResourceTracker.getUserRecord(Os.getuid());
         assertEquals(0, userRecord.mTransformQuotaTracker.mCurrent);
+        assertEquals(1, userRecord.mSpiQuotaTracker.mCurrent);
+
+        mIpSecService.releaseSecurityParameterIndex(ipSecConfig.getSpiResourceId());
+        // Verify that ipSecDeleteSa was not called when the SPI was released because the
+        // ownedByTransform property should prevent it; (note, the called count is cumulative).
+        verify(mMockNetd, times(1))
+                .ipSecDeleteSecurityAssociation(
+                        anyInt(),
+                        anyString(),
+                        anyString(),
+                        anyInt(),
+                        anyInt(),
+                        anyInt());
+        assertEquals(0, userRecord.mSpiQuotaTracker.mCurrent);
+
         try {
             userRecord.mTransformRecords.getRefcountedResourceOrThrow(
                     createTransformResp.resourceId);