Re: [PATCH net v4] net: Fix Hole-196 vulnerability by dropping unicast ARP/IP in L2 BMC

[Star Chang]

Hi Julian,

Thank you very much for the insightful review and for pointing out the RFC 826 and RFC 2002 references.

You are completely right on both points:
1. Per RFC 826, the Target Hardware Address (tha) in an ARP Request has no protocol meaning, and Linux updates neighbor entries using (sip, sha), never tha. Inspecting tha on ARP Requests was a fundamental mistake and would break legitimate devices that leave arbitrary/unicast addresses in tha.
2. In RFC 2002 (section 4.6), many real-world GARP reply implementations legitimately set tha to the broadcast address (ff:ff:ff:ff:ff:ff) since the announcement is broadcast to all hosts. Requiring tha == sha in our check was overly restrictive and would cause false-positive drops for legitimate GARP replies.

Regarding your question about arp_accept:
In our testing, arp_accept = 0 successfully prevents creating *new* neighbor entries from unsolicited broadcasts. However, in typical client scenarios, the victim station almost always already has an existing neighbor entry for its default gateway (i.e. n is not NULL in arp_process).

When n already exists, arp_accept is bypassed, and once the 1-second LOCKTIME window has passed, arp_process() calls:
    neigh_update(n, sha, state, NEIGH_UPDATE_F_OVERRIDE, 0);
which executes memcpy(&neigh->ha, lladdr, dev->addr_len) in __neigh_update(), overwriting the gateway's cached MAC address with the attacker's MAC.

In a Wi-Fi (Hole-196) scenario where clients share the GTK, an attacker cannot forge unicast frames directly to the victim, so they broadcast an unsolicited unicast ARP Reply (sip = gateway_ip, sha = attacker_mac, tip = victim_ip) over L2 broadcast (PACKET_BROADCAST).

Under standard networking behavior, a non-GARP ARP Reply is strictly meant to be delivered as an L2 unicast frame (PACKET_HOST) to the requester. The only legitimate reason an ARP Reply is ever sent over L2 broadcast is for Gratuitous ARP (sip == tip).

Based on your feedback, we plan to completely drop all checks on tha, leave ARP Requests entirely untouched, and strictly narrow the ARP defense to drop ARP Replies received over L2 BMC *only* if they are not Gratuitous ARP (sip != tip):

--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -791,32 +791,18 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
 	    (!IN_DEV_ROUTE_LOCALNET(in_dev) && ipv4_is_loopback(tip)))
 		goto out_free_skb;
 
 	if ((skb->pkt_type == PACKET_BROADCAST ||
 	     skb->pkt_type == PACKET_MULTICAST) &&
 	    IN_DEV_ORCONF(in_dev, DROP_UNICAST_IN_L2_MULTICAST)) {
-		bool is_valid_garp = (sip == tip) && tha &&
-				     !memcmp(tha, sha, dev->addr_len);
-
-		if (arp->ar_op == htons(ARPOP_REPLY) && !is_valid_garp) {
-			net_warn_ratelimited("Drop ARP Reply in L2 BMC on %s\n",
+		/*
+		 * Standard ARP Replies are strictly unicast (PACKET_HOST).
+		 * An ARP Reply in an L2 broadcast frame with (sip != tip)
+		 * is an injected unicast reply. Legitimate GARP (sip == tip)
+		 * is preserved and defers to DROP_GRATUITOUS_ARP.
+		 * ARP Requests are left untouched per RFC 826.
+		 */
+		if (arp->ar_op == htons(ARPOP_REPLY) && sip != tip) {
+			net_warn_ratelimited("Drop non-GARP ARP Reply in L2 BMC on %s\n",
 					     dev->name);
 			goto out_free_skb;
 		}
-		if (dev->addr_len == ETH_ALEN && tha &&
-		    is_valid_ether_addr(tha) && !is_valid_garp) {
-			net_warn_ratelimited("Drop unicast THA ARP Req in L2 BMC on %s\n",
-					     dev->name);
-			goto out_free_skb;
-		}
 	}

With this approach:
1. ARP Requests are completely untouched (fully adhering to RFC 826).
2. Legitimate GARP announcements (sip == tip) for VRRP/HSRP router failovers are preserved regardless of what value they put in tha (respecting RFC 2002).
3. Injected unicast ARP Replies in L2 BMC (sip != tip) that poison existing gateway entries are dropped.

Does this sound like the right direction to you for v5?

Thanks again for your time, guidance, and expertise!

Best regards,
Star Chang