Merge branch 'fix-isolation-of-broadcast-traffic-and-unmatched-unicast-traffic-with-macsec-offload'

Rahul Rameshbabu says:

====================
Fix isolation of broadcast traffic and unmatched unicast traffic with MACsec offload

Some device drivers support devices that enable them to annotate whether a
Rx skb refers to a packet that was processed by the MACsec offloading
functionality of the device. Logic in the Rx handling for MACsec offload
does not utilize this information to preemptively avoid forwarding to the
macsec netdev currently. Because of this, things like multicast messages or
unicast messages with an unmatched destination address such as ARP requests
are forwarded to the macsec netdev whether the message received was MACsec
encrypted or not. The goal of this patch series is to improve the Rx
handling for MACsec offload for devices capable of annotating skbs received
that were decrypted by the NIC offload for MACsec.

Here is a summary of the issue that occurs with the existing logic today.

    * The current design of the MACsec offload handling path tries to use
      "best guess" mechanisms for determining whether a packet associated
      with the currently handled skb in the datapath was processed via HW
      offload
    * The best guess mechanism uses the following heuristic logic (in order of
      precedence)
      - Check if header destination MAC address matches MACsec netdev MAC
        address -> forward to MACsec port
      - Check if packet is multicast traffic -> forward to MACsec port
      - MACsec security channel was able to be looked up from skb offload
        context (mlx5 only) -> forward to MACsec port
    * Problem: plaintext traffic can potentially solicit a MACsec encrypted
      response from the offload device
      - Core aspect of MACsec is that it identifies unauthorized LAN connections
        and excludes them from communication
        + This behavior can be seen when not enabling offload for MACsec
      - The offload behavior violates this principle in MACsec

I believe this behavior is a security bug since applications utilizing
MACsec could be exploited using this behavior, and the correct way to
resolve this is by having the hardware correctly indicate whether MACsec
offload occurred for the packet or not. In the patches in this series, I
leave a warning for when the problematic path occurs because I cannot
figure out a secure way to fix the security issue that applies to the core
MACsec offload handling in the Rx path without breaking MACsec offload for
other vendors.

Shown at the bottom is an example use case where plaintext traffic sent to
a physical port of a NIC configured for MACsec offload is unable to be
handled correctly by the software stack when the NIC provides awareness to
the kernel about whether the received packet is MACsec traffic or not. In
this specific example, plaintext ARP requests are being responded with
MACsec encrypted ARP replies (which leads to routing information being
unable to be built for the requester).

    Side 1

      ip link del macsec0
      ip address flush mlx5_1
      ip address add 1.1.1.1/24 dev mlx5_1
      ip link set dev mlx5_1 up
      ip link add link mlx5_1 macsec0 type macsec sci 1 encrypt on
      ip link set dev macsec0 address 00:11:22:33:44:66
      ip macsec offload macsec0 mac
      ip macsec add macsec0 tx sa 0 pn 1 on key 00 dffafc8d7b9a43d5b9a3dfbbf6a30c16
      ip macsec add macsec0 rx sci 2 on
      ip macsec add macsec0 rx sci 2 sa 0 pn 1 on key 00 ead3664f508eb06c40ac7104cdae4ce5
      ip address flush macsec0
      ip address add 2.2.2.1/24 dev macsec0
      ip link set dev macsec0 up

      # macsec0 enters promiscuous mode.
      # This enables all traffic received on macsec_vlan to be processed by
      # the macsec offload rx datapath. This however means that traffic
      # meant to be received by mlx5_1 will be incorrectly steered to
      # macsec0 as well.

      ip link add link macsec0 name macsec_vlan type vlan id 1
      ip link set dev macsec_vlan address 00:11:22:33:44:88
      ip address flush macsec_vlan
      ip address add 3.3.3.1/24 dev macsec_vlan
      ip link set dev macsec_vlan up

    Side 2

      ip link del macsec0
      ip address flush mlx5_1
      ip address add 1.1.1.2/24 dev mlx5_1
      ip link set dev mlx5_1 up
      ip link add link mlx5_1 macsec0 type macsec sci 2 encrypt on
      ip link set dev macsec0 address 00:11:22:33:44:77
      ip macsec offload macsec0 mac
      ip macsec add macsec0 tx sa 0 pn 1 on key 00 ead3664f508eb06c40ac7104cdae4ce5
      ip macsec add macsec0 rx sci 1 on
      ip macsec add macsec0 rx sci 1 sa 0 pn 1 on key 00 dffafc8d7b9a43d5b9a3dfbbf6a30c16
      ip address flush macsec0
      ip address add 2.2.2.2/24 dev macsec0
      ip link set dev macsec0 up

      # macsec0 enters promiscuous mode.
      # This enables all traffic received on macsec_vlan to be processed by
      # the macsec offload rx datapath. This however means that traffic
      # meant to be received by mlx5_1 will be incorrectly steered to
      # macsec0 as well.

      ip link add link macsec0 name macsec_vlan type vlan id 1
      ip link set dev macsec_vlan address 00:11:22:33:44:99
      ip address flush macsec_vlan
      ip address add 3.3.3.2/24 dev macsec_vlan
      ip link set dev macsec_vlan up

    Side 1

      ping -I mlx5_1 1.1.1.2
      PING 1.1.1.2 (1.1.1.2) from 1.1.1.1 mlx5_1: 56(84) bytes of data.
      From 1.1.1.1 icmp_seq=1 Destination Host Unreachable
      ping: sendmsg: No route to host
      From 1.1.1.1 icmp_seq=2 Destination Host Unreachable
      From 1.1.1.1 icmp_seq=3 Destination Host Unreachable

Changes:

  v2->v3:
    * Made dev paramater const for eth_skb_pkt_type helper as suggested by Sabrina
      Dubroca <sd@queasysnail.net>
  v1->v2:
    * Fixed series subject to detail the issue being fixed
    * Removed strange characters from cover letter
    * Added comment in example that illustrates the impact involving
      promiscuous mode
    * Added patch for generalizing packet type detection
    * Added Fixes: tags and targeting net
    * Removed pointless warning in the heuristic Rx path for macsec offload
    * Applied small refactor in Rx path offload to minimize scope of rx_sc
      local variable

Link: https://github.com/Binary-Eater/macsec-rx-offload/blob/trunk/MACsec_violation_in_core_stack_offload_rx_handling.pdf
Link: https://lore.kernel.org/netdev/20240419213033.400467-5-rrameshbabu@nvidia.com/
Link: https://lore.kernel.org/netdev/20240419011740.333714-1-rrameshbabu@nvidia.com/
Link: https://lore.kernel.org/netdev/87r0l25y1c.fsf@nvidia.com/
Link: https://lore.kernel.org/netdev/20231116182900.46052-1-rrameshbabu@nvidia.com/
====================

Link: https://lore.kernel.org/r/20240423181319.115860-1-rrameshbabu@nvidia.comSigned-off-by: Jakub Kicinski <kuba@kernel.org>

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c

@@ -1640,6 +1640,7 @@ static const struct macsec_ops macsec_offload_ops = {
 	.mdo_add_secy = mlx5e_macsec_add_secy,
 	.mdo_upd_secy = mlx5e_macsec_upd_secy,
 	.mdo_del_secy = mlx5e_macsec_del_secy,
+	.rx_uses_md_dst = true,
 };
 
 bool mlx5e_macsec_handle_tx_skb(struct mlx5e_macsec *macsec, struct sk_buff *skb)

drivers/net/macsec.c

@@ -999,10 +999,12 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 	struct metadata_dst *md_dst;
 	struct macsec_rxh_data *rxd;
 	struct macsec_dev *macsec;
+	bool is_macsec_md_dst;
 
 	rcu_read_lock();
 	rxd = macsec_data_rcu(skb->dev);
 	md_dst = skb_metadata_dst(skb);
+	is_macsec_md_dst = md_dst && md_dst->type == METADATA_MACSEC;
 
 	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
 		struct sk_buff *nskb;
@@ -1013,14 +1015,42 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 		 * the SecTAG, so we have to deduce which port to deliver to.
 		 */
 		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
-			struct macsec_rx_sc *rx_sc = NULL;
+			const struct macsec_ops *ops;
 
-			if (md_dst && md_dst->type == METADATA_MACSEC)
-				rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
+			ops = macsec_get_ops(macsec, NULL);
 
-			if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc)
+			if (ops->rx_uses_md_dst && !is_macsec_md_dst)
 				continue;
 
+			if (is_macsec_md_dst) {
+				struct macsec_rx_sc *rx_sc;
+
+				/* All drivers that implement MACsec offload
+				 * support using skb metadata destinations must
+				 * indicate that they do so.
+				 */
+				DEBUG_NET_WARN_ON_ONCE(!ops->rx_uses_md_dst);
+				rx_sc = find_rx_sc(&macsec->secy,
+						   md_dst->u.macsec_info.sci);
+				if (!rx_sc)
+					continue;
+				/* device indicated macsec offload occurred */
+				skb->dev = ndev;
+				skb->pkt_type = PACKET_HOST;
+				eth_skb_pkt_type(skb, ndev);
+				ret = RX_HANDLER_ANOTHER;
+				goto out;
+			}
+
+			/* This datapath is insecure because it is unable to
+			 * enforce isolation of broadcast/multicast traffic and
+			 * unicast traffic with promiscuous mode on the macsec
+			 * netdev. Since the core stack has no mechanism to
+			 * check that the hardware did indeed receive MACsec
+			 * traffic, it is possible that the response handling
+			 * done by the MACsec port was to a plaintext packet.
+			 * This violates the MACsec protocol standard.
+			 */
 			if (ether_addr_equal_64bits(hdr->h_dest,
 						    ndev->dev_addr)) {
 				/* exact match, divert skb to this port */
@@ -1036,14 +1066,10 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 					break;
 
 				nskb->dev = ndev;
-				if (ether_addr_equal_64bits(hdr->h_dest,
-							    ndev->broadcast))
-					nskb->pkt_type = PACKET_BROADCAST;
-				else
-					nskb->pkt_type = PACKET_MULTICAST;
+				eth_skb_pkt_type(nskb, ndev);
 
 				__netif_rx(nskb);
-			} else if (rx_sc || ndev->flags & IFF_PROMISC) {
+			} else if (ndev->flags & IFF_PROMISC) {
 				skb->dev = ndev;
 				skb->pkt_type = PACKET_HOST;
 				ret = RX_HANDLER_ANOTHER;

include/linux/etherdevice.h

@@ -607,6 +607,31 @@ static inline void eth_hw_addr_gen(struct net_device *dev, const u8 *base_addr,
 	eth_hw_addr_set(dev, addr);
 }
 
+/**
+ * eth_skb_pkt_type - Assign packet type if destination address does not match
+ * @skb: Assigned a packet type if address does not match @dev address
+ * @dev: Network device used to compare packet address against
+ *
+ * If the destination MAC address of the packet does not match the network
+ * device address, assign an appropriate packet type.
+ */
+static inline void eth_skb_pkt_type(struct sk_buff *skb,
+				    const struct net_device *dev)
+{
+	const struct ethhdr *eth = eth_hdr(skb);
+
+	if (unlikely(!ether_addr_equal_64bits(eth->h_dest, dev->dev_addr))) {
+		if (unlikely(is_multicast_ether_addr_64bits(eth->h_dest))) {
+			if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
+				skb->pkt_type = PACKET_BROADCAST;
+			else
+				skb->pkt_type = PACKET_MULTICAST;
+		} else {
+			skb->pkt_type = PACKET_OTHERHOST;
+		}
+	}
+}
+
 /**
  * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
  * @skb: Buffer to pad

include/net/macsec.h

@@ -321,6 +321,7 @@ struct macsec_context {
  *	for the TX tag
  * @needed_tailroom: number of bytes reserved at the end of the sk_buff for the
  *	TX tag
+ * @rx_uses_md_dst: whether MACsec device offload supports sk_buff md_dst
  */
 struct macsec_ops {
 	/* Device wide */
@@ -352,6 +353,7 @@ struct macsec_ops {
 				 struct sk_buff *skb);
 	unsigned int needed_headroom;
 	unsigned int needed_tailroom;
+	bool rx_uses_md_dst;
 };
 
 void macsec_pn_wrapped(struct macsec_secy *secy, struct macsec_tx_sa *tx_sa);

net/ethernet/eth.c

@@ -164,17 +164,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 	eth = (struct ethhdr *)skb->data;
 	skb_pull_inline(skb, ETH_HLEN);
 
-	if (unlikely(!ether_addr_equal_64bits(eth->h_dest,
-					      dev->dev_addr))) {
-		if (unlikely(is_multicast_ether_addr_64bits(eth->h_dest))) {
-			if (ether_addr_equal_64bits(eth->h_dest, dev->broadcast))
-				skb->pkt_type = PACKET_BROADCAST;
-			else
-				skb->pkt_type = PACKET_MULTICAST;
-		} else {
-			skb->pkt_type = PACKET_OTHERHOST;
-		}
-	}
+	eth_skb_pkt_type(skb, dev);
 
 	/*
 	 * Some variants of DSA tagging don't have an ethertype field