From 6f532ba30fbfebd40698d6af0e793a1947b5d280 Mon Sep 17 00:00:00 2001
From: Lorenzo Colitti <lorenzo@google.com>
Date: Tue, 14 Apr 2020 09:21:19 +0000
Subject: [PATCH] Don't crash when receiving an RTM_DELNEIGH or NUD_FAILED.

These events don't have MAC addresses, so the code attempts to
create an Ipv6ForwardingRule with a null MAC address. This
crashes when attempting to get the raw MAC address bytes to send
to netd in the TetherOffloadRuleParcel.

This was not caught by unit tests because the test exercise this
code path in a way that is not correct (by sending RTM_DELNEIGH
and NUD_FAILED events with MAC addresses). Fix the unit tests to
properly pass in null MAC addresses for these events.

Bug: 153697068
Test: fixed existing tests to be more realistic
Merged-In: I26d89a81f1c448d9b4809652b079a5f5eace3924
Change-Id: I26d89a81f1c448d9b4809652b079a5f5eace3924
---
 Tethering/src/android/net/ip/IpServer.java               | 9 +++++++--
 .../tests/unit/src/android/net/ip/IpServerTest.java      | 8 +++++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java
index 1dac5b7846..83727bcdc6 100644
--- a/Tethering/src/android/net/ip/IpServer.java
+++ b/Tethering/src/android/net/ip/IpServer.java
@@ -122,6 +122,8 @@ public class IpServer extends StateMachine {
     // TODO: have this configurable
     private static final int DHCP_LEASE_TIME_SECS = 3600;
 
+    private static final MacAddress NULL_MAC_ADDRESS = MacAddress.fromString("00:00:00:00:00:00");
+
     private static final String TAG = "IpServer";
     private static final boolean DBG = false;
     private static final boolean VDBG = false;
@@ -902,9 +904,12 @@ public class IpServer extends StateMachine {
             return;
         }
 
+        // When deleting rules, we still need to pass a non-null MAC, even though it's ignored.
+        // Do this here instead of in the Ipv6ForwardingRule constructor to ensure that we never
+        // add rules with a null MAC, only delete them.
+        MacAddress dstMac = e.isValid() ? e.macAddr : NULL_MAC_ADDRESS;
         Ipv6ForwardingRule rule = new Ipv6ForwardingRule(upstreamIfindex,
-                mInterfaceParams.index, (Inet6Address) e.ip, mInterfaceParams.macAddr,
-                e.macAddr);
+                mInterfaceParams.index, (Inet6Address) e.ip, mInterfaceParams.macAddr, dstMac);
         if (e.isValid()) {
             addIpv6ForwardingRule(rule);
         } else {
diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
index fdfdae837d..f9be7b9d36 100644
--- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
+++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
@@ -587,6 +587,7 @@ public class IpServerTest {
         final InetAddress neighB = InetAddresses.parseNumericAddress("2001:db8::2");
         final InetAddress neighLL = InetAddresses.parseNumericAddress("fe80::1");
         final InetAddress neighMC = InetAddresses.parseNumericAddress("ff02::1234");
+        final MacAddress macNull = MacAddress.fromString("00:00:00:00:00:00");
         final MacAddress macA = MacAddress.fromString("00:00:00:00:00:0a");
         final MacAddress macB = MacAddress.fromString("11:22:33:00:00:0b");
 
@@ -612,13 +613,14 @@ public class IpServerTest {
         verifyNoMoreInteractions(mNetd);
 
         // A neighbor that is no longer valid causes the rule to be removed.
-        recvNewNeigh(myIfindex, neighA, NUD_FAILED, macA);
-        verify(mNetd).tetherOffloadRuleRemove(matches(UPSTREAM_IFINDEX, neighA, macA));
+        // NUD_FAILED events do not have a MAC address.
+        recvNewNeigh(myIfindex, neighA, NUD_FAILED, null);
+        verify(mNetd).tetherOffloadRuleRemove(matches(UPSTREAM_IFINDEX, neighA, macNull));
         reset(mNetd);
 
         // A neighbor that is deleted causes the rule to be removed.
         recvDelNeigh(myIfindex, neighB, NUD_STALE, macB);
-        verify(mNetd).tetherOffloadRuleRemove(matches(UPSTREAM_IFINDEX, neighB, macB));
+        verify(mNetd).tetherOffloadRuleRemove(matches(UPSTREAM_IFINDEX, neighB, macNull));
         reset(mNetd);
 
         // Upstream changes result in deleting and re-adding the rules.