From a6fe4d1d8f82c444722aac6d3ef6e317f3f1459c Mon Sep 17 00:00:00 2001
From: Bruno Dal Bo <brunodalbo@google.com>
Date: Thu, 12 Jan 2023 13:05:02 -0800
Subject: [PATCH] Handle gratuitous ARP as unsolicited reply

A gratuitous ARP does not infer two-way connectivity and must not move neighbor
state to reachable.

PiperOrigin-RevId: 501645481
---
 pkg/tcpip/network/arp/arp.go      |  9 ++-
 pkg/tcpip/network/arp/arp_test.go | 94 +++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/pkg/tcpip/network/arp/arp.go b/pkg/tcpip/network/arp/arp.go
index 1f1489993..67b97efa8 100644
--- a/pkg/tcpip/network/arp/arp.go
+++ b/pkg/tcpip/network/arp/arp.go
@@ -230,12 +230,11 @@ func (e *endpoint) HandlePacket(pkt stack.PacketBufferPtr) {
 		e.dad.StopLocked(addr, &stack.DADDupAddrDetected{HolderLinkAddress: linkAddr})
 		e.mu.Unlock()
 
-		// The solicited, override, and isRouter flags are not available for ARP;
-		// they are only available for IPv6 Neighbor Advertisements.
 		switch err := e.nic.HandleNeighborConfirmation(header.IPv4ProtocolNumber, addr, linkAddr, stack.ReachabilityConfirmationFlags{
-			// Solicited and unsolicited (also referred to as gratuitous) ARP Replies
-			// are handled equivalently to a solicited Neighbor Advertisement.
-			Solicited: true,
+			// Only unicast ARP replies are considered solicited. Broadcast replies
+			// are gratuitous ARP replies and should not move neighbor entries to the
+			// reachable state.
+			Solicited: pkt.PktType == tcpip.PacketHost,
 			// If a different link address is received than the one cached, the entry
 			// should always go to Stale.
 			Override: false,
diff --git a/pkg/tcpip/network/arp/arp_test.go b/pkg/tcpip/network/arp/arp_test.go
index 21755fe28..8afd995dd 100644
--- a/pkg/tcpip/network/arp/arp_test.go
+++ b/pkg/tcpip/network/arp/arp_test.go
@@ -420,6 +420,100 @@ func TestDirectRequest(t *testing.T) {
 	}
 }
 
+func TestReplyPacketType(t *testing.T) {
+	for _, testCase := range []struct {
+		name             string
+		packetType       tcpip.PacketType
+		becomesReachable bool
+	}{
+		{
+			name:             "unicast",
+			packetType:       tcpip.PacketHost,
+			becomesReachable: true,
+		},
+		{
+			name:             "broadcast",
+			packetType:       tcpip.PacketBroadcast,
+			becomesReachable: false,
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			c := makeTestContext(t, 1, 1)
+			defer c.cleanup()
+
+			// Inject an incoming ARP request first.
+			v := make([]byte, header.ARPSize)
+			h := header.ARP(v)
+			h.SetIPv4OverEthernet()
+			h.SetOp(header.ARPRequest)
+			if got, want := copy(h.HardwareAddressSender(), remoteLinkAddr), header.EthernetAddressSize; got != want {
+				t.Fatalf("got copy(_, _) = %d, want = %d", got, want)
+			}
+			if got, want := copy(h.ProtocolAddressSender(), remoteAddr), header.IPv4AddressSize; got != want {
+				t.Fatalf("got copy(_, _) = %d, want = %d", got, want)
+			}
+			if got, want := copy(h.ProtocolAddressTarget(), stackAddr), header.IPv4AddressSize; got != want {
+				t.Fatalf("got copy(_, _) = %d, want = %d", got, want)
+			}
+			pkt := stack.NewPacketBuffer(stack.PacketBufferOptions{
+				Payload: bufferv2.MakeWithData(v),
+			})
+			pkt.PktType = tcpip.PacketBroadcast
+			c.linkEP.InjectInbound(arp.ProtocolNumber, pkt)
+			pkt.DecRef()
+
+			if got, ok := c.nudDisp.nextEvent(); ok {
+				want := eventInfo{
+					eventType: entryAdded,
+					nicID:     nicID,
+					entry: stack.NeighborEntry{
+						Addr:     remoteAddr,
+						LinkAddr: remoteLinkAddr,
+						State:    stack.Stale,
+					},
+				}
+				if diff := cmp.Diff(want, got, cmp.AllowUnexported(eventInfo{}), cmpopts.IgnoreFields(stack.NeighborEntry{}, "UpdatedAt")); diff != "" {
+					t.Errorf("got invalid event (-want +got):\n%s", diff)
+				}
+			} else {
+				t.Fatal("event didn't arrive")
+			}
+
+			// Then inject replies with different packet types.
+			h.SetIPv4OverEthernet()
+			h.SetOp(header.ARPReply)
+			pkt = stack.NewPacketBuffer(stack.PacketBufferOptions{
+				Payload: bufferv2.MakeWithData(v),
+			})
+			pkt.PktType = testCase.packetType
+			c.linkEP.InjectInbound(arp.ProtocolNumber, pkt)
+			pkt.DecRef()
+
+			got, ok := c.nudDisp.nextEvent()
+			// If the entry doesn't become reachable we're not supposed to see a new
+			// event.
+			if got, want := ok, testCase.becomesReachable; got != want {
+				t.Errorf("got c.nudDisp.nextEvent() = %t, want %t", got, want)
+			}
+			if ok {
+				want := eventInfo{
+					eventType: entryChanged,
+					nicID:     nicID,
+					entry: stack.NeighborEntry{
+						Addr:     remoteAddr,
+						LinkAddr: remoteLinkAddr,
+						State:    stack.Reachable,
+					},
+				}
+				if diff := cmp.Diff(want, got, cmp.AllowUnexported(eventInfo{}), cmpopts.IgnoreFields(stack.NeighborEntry{}, "UpdatedAt")); diff != "" {
+					t.Errorf("got invalid event (-want +got):\n%s", diff)
+				}
+			}
+		})
+	}
+
+}
+
 var _ stack.LinkEndpoint = (*testLinkEndpoint)(nil)
 
 type testLinkEndpoint struct {