From d3880b76cbeaa1cadc38388d5684e00172566376 Mon Sep 17 00:00:00 2001
From: Ghanan Gowripalan <ghanan@google.com>
Date: Tue, 15 Sep 2020 11:51:17 -0700
Subject: [PATCH] Don't conclude broadcast from route destination

The routing table (in its current) form should not be used to make
decisions about whether a remote address is a broadcast address or
not (for IPv4).

Note, a destination subnet does not always map to a network.
E.g. RouterA may have a route to 192.168.0.0/22 through RouterB,
but RouterB may be configured with 4x /24 subnets on 4 different
interfaces.

See https://github.com/google/gvisor/issues/3938.

PiperOrigin-RevId: 331819868
---
 pkg/tcpip/stack/route.go            | 23 +++++++++++------------
 pkg/tcpip/stack/stack.go            |  4 +---
 pkg/tcpip/transport/udp/udp_test.go |  6 ++++--
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/pkg/tcpip/stack/route.go b/pkg/tcpip/stack/route.go
index c2eabde9e..2cbbf0de8 100644
--- a/pkg/tcpip/stack/route.go
+++ b/pkg/tcpip/stack/route.go
@@ -48,10 +48,6 @@ type Route struct {
 
 	// Loop controls where WritePacket should send packets.
 	Loop PacketLooping
-
-	// directedBroadcast indicates whether this route is sending a directed
-	// broadcast packet.
-	directedBroadcast bool
 }
 
 // makeRoute initializes a new route. It takes ownership of the provided
@@ -303,24 +299,27 @@ func (r *Route) Stack() *Stack {
 	return r.ref.stack()
 }
 
+func (r *Route) isV4Broadcast(addr tcpip.Address) bool {
+	if addr == header.IPv4Broadcast {
+		return true
+	}
+
+	subnet := r.ref.addrWithPrefix().Subnet()
+	return subnet.IsBroadcast(addr)
+}
+
 // IsOutboundBroadcast returns true if the route is for an outbound broadcast
 // packet.
 func (r *Route) IsOutboundBroadcast() bool {
 	// Only IPv4 has a notion of broadcast.
-	return r.directedBroadcast || r.RemoteAddress == header.IPv4Broadcast
+	return r.isV4Broadcast(r.RemoteAddress)
 }
 
 // IsInboundBroadcast returns true if the route is for an inbound broadcast
 // packet.
 func (r *Route) IsInboundBroadcast() bool {
 	// Only IPv4 has a notion of broadcast.
-	if r.LocalAddress == header.IPv4Broadcast {
-		return true
-	}
-
-	addr := r.ref.addrWithPrefix()
-	subnet := addr.Subnet()
-	return subnet.IsBroadcast(r.LocalAddress)
+	return r.isV4Broadcast(r.LocalAddress)
 }
 
 // ReverseRoute returns new route with given source and destination address.
diff --git a/pkg/tcpip/stack/stack.go b/pkg/tcpip/stack/stack.go
index def8b0b43..6a683545d 100644
--- a/pkg/tcpip/stack/stack.go
+++ b/pkg/tcpip/stack/stack.go
@@ -1311,13 +1311,11 @@ func (s *Stack) FindRoute(id tcpip.NICID, localAddr, remoteAddr tcpip.Address, n
 					}
 
 					r := makeRoute(netProto, ref.address(), remoteAddr, nic.linkEP.LinkAddress(), ref, s.handleLocal && !nic.isLoopback(), multicastLoop && !nic.isLoopback())
-					r.directedBroadcast = route.Destination.IsBroadcast(remoteAddr)
-
 					if len(route.Gateway) > 0 {
 						if needRoute {
 							r.NextHop = route.Gateway
 						}
-					} else if r.directedBroadcast {
+					} else if subnet := ref.addrWithPrefix().Subnet(); subnet.IsBroadcast(remoteAddr) {
 						r.RemoteLinkAddress = header.EthernetBroadcastAddress
 					}
 
diff --git a/pkg/tcpip/transport/udp/udp_test.go b/pkg/tcpip/transport/udp/udp_test.go
index 0cbc045d8..d5881d183 100644
--- a/pkg/tcpip/transport/udp/udp_test.go
+++ b/pkg/tcpip/transport/udp/udp_test.go
@@ -2343,8 +2343,10 @@ func TestOutgoingSubnetBroadcast(t *testing.T) {
 					NIC:         nicID1,
 				},
 			},
-			remoteAddr:           remNetSubnetBcast,
-			requiresBroadcastOpt: true,
+			remoteAddr: remNetSubnetBcast,
+			// TODO(gvisor.dev/issue/3938): Once we support marking a route as
+			// broadcast, this test should require the broadcast option to be set.
+			requiresBroadcastOpt: false,
 		},
 	}