[PATCH net-next 0/3] Resolve security issue in MACsec offload Rx datapath

[PATCH net-next 0/3] Resolve security issue in MACsec offload Rx datapath

[Rahul Rameshbabu]

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
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
      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
      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

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/87r0l25y1c.fsf@nvidia.com/
Link: https://lore.kernel.org/netdev/20231116182900.46052-1-rrameshbabu@nvidia.com/
Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: stable@vger.kernel.org
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
Rahul Rameshbabu (3):
  macsec: Enable devices to advertise whether they update sk_buff md_dst
    during offloads
  macsec: Detect if Rx skb is macsec-related for offloading devices that
    update md_dst
  net/mlx5e: Advertise mlx5 ethernet driver updates sk_buff md_dst for
    MACsec

 .../mellanox/mlx5/core/en_accel/macsec.c      |  1 +
 drivers/net/macsec.c                          | 57 ++++++++++++++++---
 include/net/macsec.h                          |  2 +
 3 files changed, 51 insertions(+), 9 deletions(-)

-- 
2.42.0

[PATCH net-next 1/3] macsec: Enable devices to advertise whether they update sk_buff md_dst during offloads

[Rahul Rameshbabu]

Cannot know whether a Rx skb missing md_dst is intended for MACsec or not
without knowing whether the device is able to update this field during an
offload. Assume that an offload to a MACsec device cannot support updating
md_dst by default. Capable devices can advertise that they do indicate that
an skb is related to a MACsec offloaded packet using the md_dst.

Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: stable@vger.kernel.org
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reviewed-by: Benjamin Poirier <bpoirier@nvidia.com>
Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
---
 include/net/macsec.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/net/macsec.h b/include/net/macsec.h
index dbd22180cc5c..de216cbc6b05 100644
--- a/include/net/macsec.h
+++ b/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);
-- 
2.42.0

[PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Rahul Rameshbabu]

Can now correctly identify where the packets should be delivered by using
md_dst or its absence on devices that provide it.

This detection is not possible without device drivers that update md_dst. A
fallback pattern should be used for supporting such device drivers. This
fallback mode causes multicast messages to be cloned to both the non-macsec
and macsec ports, independent of whether the multicast message received was
encrypted over MACsec or not. Other non-macsec traffic may also fail to be
handled correctly for devices in promiscuous mode.

Link: https://lore.kernel.org/netdev/ZULRxX9eIbFiVi7v@hog/
Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: stable@vger.kernel.org
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reviewed-by: Benjamin Poirier <bpoirier@nvidia.com>
Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
---
 drivers/net/macsec.c | 57 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 9 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 0206b84284ab..679302ef1cd9 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -991,6 +991,19 @@ static struct macsec_rx_sc *find_rx_sc_rtnl(struct macsec_secy *secy, sci_t sci)
 	return NULL;
 }
 
+static __u8 macsec_offload_pkt_type(const u8 *h_dest, const u8 *ndev_broadcast)
+
+{
+	if (is_multicast_ether_addr_64bits(h_dest)) {
+		if (ether_addr_equal_64bits(h_dest, ndev_broadcast))
+			return PACKET_BROADCAST;
+		else
+			return PACKET_MULTICAST;
+	}
+
+	return PACKET_HOST;
+}
+
 static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 {
 	/* Deliver to the uncontrolled port by default */
@@ -999,10 +1012,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;
@@ -1014,13 +1029,40 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 		 */
 		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) {
+				/* 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 = macsec_offload_pkt_type(
+					hdr->h_dest, ndev->broadcast);
+				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.
+			 */
+			DEBUG_NET_WARN_ON_ONCE(true);
 			if (ether_addr_equal_64bits(hdr->h_dest,
 						    ndev->dev_addr)) {
 				/* exact match, divert skb to this port */
@@ -1036,14 +1078,11 @@ 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;
+				nskb->pkt_type = macsec_offload_pkt_type(
+					hdr->h_dest, ndev->broadcast);
 
 				__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;
-- 
2.42.0

[PATCH net-next 3/3] net/mlx5e: Advertise mlx5 ethernet driver updates sk_buff md_dst for MACsec

[Rahul Rameshbabu]

mlx5 Rx flow steering and CQE handling enable the driver to be able to
update an skb's md_dst attribute as MACsec when MACsec traffic arrives when
a device is configured for offloading. Advertise this to the core stack to
take advantage of this capability.

Cc: stable@vger.kernel.org
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reviewed-by: Benjamin Poirier <bpoirier@nvidia.com>
Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index b2cabd6ab86c..cc9bcc420032 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/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)
-- 
2.42.0

Re: [PATCH net-next 0/3] Resolve security issue in MACsec offload Rx datapath

[Sabrina Dubroca]

This should go to net, not net-next. It fixes a serious bug. Also
please change the title to:
  fix isolation of broadcast traffic with MACsec offload

"resolve security issue" is too vague.

2024-04-18, 18:17:14 -0700, Rahul Rameshbabu wrote:
> 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
> 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​

nit: there's a strange character after "offload" and at the end of a
few other lines in this list

>     * 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​
                                                                   here ^

>       - MACsec security channel was able to be looked up from skb offload
>         context (mlx5 only) -> forward to MACsec port​
                                                  here ^

>     * 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​
                                                                     here ^

>       - The offload behavior violates this principle in MACsec
> 

> 
> 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/87r0l25y1c.fsf@nvidia.com/
> Link: https://lore.kernel.org/netdev/20231116182900.46052-1-rrameshbabu@nvidia.com/
> Cc: Sabrina Dubroca <sd@queasysnail.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>

I would put some Fixes tags on this series. Since we can't do anything
about non-md_dst devices, I would say that the main patch fixes
860ead89b851 ("net/macsec: Add MACsec skb_metadata_dst Rx Data path
support"), and the driver patch fixes b7c9400cbc48 ("net/mlx5e:
Implement MACsec Rx data path using MACsec skb_metadata_dst"). Jakub,
Rahul, does that sound ok to both of you?

-- 
Sabrina

Re: [PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Sabrina Dubroca]

2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote:
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 0206b84284ab..679302ef1cd9 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -991,6 +991,19 @@ static struct macsec_rx_sc *find_rx_sc_rtnl(struct macsec_secy *secy, sci_t sci)
>  	return NULL;
>  }
>  
> +static __u8 macsec_offload_pkt_type(const u8 *h_dest, const u8 *ndev_broadcast)
> +

nit: empty line shouldn't be here

> +{
> +	if (is_multicast_ether_addr_64bits(h_dest)) {
> +		if (ether_addr_equal_64bits(h_dest, ndev_broadcast))
> +			return PACKET_BROADCAST;
> +		else
> +			return PACKET_MULTICAST;
> +	}
> +
> +	return PACKET_HOST;
> +}
> +
>  static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  {
>  	/* Deliver to the uncontrolled port by default */
> @@ -999,10 +1012,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;
> @@ -1014,13 +1029,40 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  		 */
>  		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
>  			struct macsec_rx_sc *rx_sc = NULL;

Please move this into the "if (is_macsec_md_dst)" block below, since
it's no longer used outside.

> +			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) {
> +				/* 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 = macsec_offload_pkt_type(
> +					hdr->h_dest, ndev->broadcast);
> +				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.
> +			 */
> +			DEBUG_NET_WARN_ON_ONCE(true);

If you insist on this warning (and I'm not convinced it's useful,
since if the HW is already built and cannot inform the driver, there's
nothing the driver implementer can do), I would move it somewhere into
the config path. macsec_update_offload would be a better location for
this kind of warning (maybe with a pr_warn (not limited to debug
configs) saying something like "MACsec offload on devices that don't
support md_dst are insecure: they do not provide proper isolation of
traffic"). The comment can stay here.

>  			if (ether_addr_equal_64bits(hdr->h_dest,
>  						    ndev->dev_addr)) {
>  				/* exact match, divert skb to this port */

-- 
Sabrina

Re: [PATCH net-next 0/3] Resolve security issue in MACsec offload Rx datapath

[Rahul Rameshbabu]

On Fri, 19 Apr, 2024 17:04:07 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
> This should go to net, not net-next. It fixes a serious bug. Also
> please change the title to:
>   fix isolation of broadcast traffic with MACsec offload
>
> "resolve security issue" is too vague.

Ack. It also fixes an issue where macsec should not reply to arbitrary
unicast traffic even in promiscuous mode. ARP unicast without a matching
destination address should not be replied to by the macsec device even
if its in promiscuous mode (the software implementation of macsec
behaves correctly in this regard).

>
> 2024-04-18, 18:17:14 -0700, Rahul Rameshbabu wrote:
>> 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
>> 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​
>
> nit: there's a strange character after "offload" and at the end of a
> few other lines in this list

Will clean up. They got carried over from the presentation I copied this
list from.

>
>>     * 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​
>                                                                    here ^
>
>>       - MACsec security channel was able to be looked up from skb offload
>>         context (mlx5 only) -> forward to MACsec port​
>                                                   here ^
>
>>     * 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​
>                                                                      here ^
>
>>       - The offload behavior violates this principle in MACsec
>> 
>

Thanks for taking the time to explicitly point them out.

>> 
>> 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/87r0l25y1c.fsf@nvidia.com/
>> Link: https://lore.kernel.org/netdev/20231116182900.46052-1-rrameshbabu@nvidia.com/
>> Cc: Sabrina Dubroca <sd@queasysnail.net>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
>
> I would put some Fixes tags on this series. Since we can't do anything
> about non-md_dst devices, I would say that the main patch fixes
> 860ead89b851 ("net/macsec: Add MACsec skb_metadata_dst Rx Data path
> support"), and the driver patch fixes b7c9400cbc48 ("net/mlx5e:
> Implement MACsec Rx data path using MACsec skb_metadata_dst"). Jakub,
> Rahul, does that sound ok to both of you?

I am aligned with this.

--
Thanks,

Rahul Rameshbabu

Re: [PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Rahul Rameshbabu]

On Fri, 19 Apr, 2024 17:05:52 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote:
<snip>
>> +			/* 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.
>> +			 */
>> +			DEBUG_NET_WARN_ON_ONCE(true);
>
> If you insist on this warning (and I'm not convinced it's useful,
> since if the HW is already built and cannot inform the driver, there's
> nothing the driver implementer can do), I would move it somewhere into
> the config path. macsec_update_offload would be a better location for
> this kind of warning (maybe with a pr_warn (not limited to debug
> configs) saying something like "MACsec offload on devices that don't
> support md_dst are insecure: they do not provide proper isolation of
> traffic"). The comment can stay here.
>

I do not like the warning either. I left it mainly if it needed further
discussion on the mailing list. Will remove it in my next revision. That
said, it may make sense to advertise rx_uses_md_dst over netlink to
annotate what macsec offload path a device uses? Just throwing out an
idea here.

>>  			if (ether_addr_equal_64bits(hdr->h_dest,
>>  						    ndev->dev_addr)) {
>>  				/* exact match, divert skb to this port */

--
Thanks,

Rahul Rameshbabu

Re: [PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Sabrina Dubroca]

2024-04-19, 11:01:20 -0700, Rahul Rameshbabu wrote:
> On Fri, 19 Apr, 2024 17:05:52 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
> > 2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote:
> <snip>
> >> +			/* 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.
> >> +			 */
> >> +			DEBUG_NET_WARN_ON_ONCE(true);
> >
> > If you insist on this warning (and I'm not convinced it's useful,
> > since if the HW is already built and cannot inform the driver, there's
> > nothing the driver implementer can do), I would move it somewhere into
> > the config path. macsec_update_offload would be a better location for
> > this kind of warning (maybe with a pr_warn (not limited to debug
> > configs) saying something like "MACsec offload on devices that don't
> > support md_dst are insecure: they do not provide proper isolation of
> > traffic"). The comment can stay here.
> >
> 
> I do not like the warning either. I left it mainly if it needed further
> discussion on the mailing list. Will remove it in my next revision. That
> said, it may make sense to advertise rx_uses_md_dst over netlink to
> annotate what macsec offload path a device uses? Just throwing out an
> idea here.

Maybe. I was also thinking about adding a way to restrict offloading
only to devices with rx_uses_md_dst.

(Slightly related) I also find it annoying that users have to tell the
kernel whether to use PHY or MAC offload, but have no way to know
which one their HW supports. That should probably have been an
implementation detail that didn't need to be part of uapi :/

-- 
Sabrina

Re: [PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Rahul Rameshbabu]

On Mon, 22 Apr, 2024 11:23:05 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2024-04-19, 11:01:20 -0700, Rahul Rameshbabu wrote:
>> On Fri, 19 Apr, 2024 17:05:52 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
>> > 2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote:
>> <snip>
>> >> +			/* 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.
>> >> +			 */
>> >> +			DEBUG_NET_WARN_ON_ONCE(true);
>> >
>> > If you insist on this warning (and I'm not convinced it's useful,
>> > since if the HW is already built and cannot inform the driver, there's
>> > nothing the driver implementer can do), I would move it somewhere into
>> > the config path. macsec_update_offload would be a better location for
>> > this kind of warning (maybe with a pr_warn (not limited to debug
>> > configs) saying something like "MACsec offload on devices that don't
>> > support md_dst are insecure: they do not provide proper isolation of
>> > traffic"). The comment can stay here.
>> >
>> 
>> I do not like the warning either. I left it mainly if it needed further
>> discussion on the mailing list. Will remove it in my next revision. That
>> said, it may make sense to advertise rx_uses_md_dst over netlink to
>> annotate what macsec offload path a device uses? Just throwing out an
>> idea here.
>
> Maybe. I was also thinking about adding a way to restrict offloading
> only to devices with rx_uses_md_dst.

That's an option. Basically, devices that do not support rx_uses_md_dst
really only just do SW MACsec but do not return an error if the offload
parameter is passed over netlink so user scripts do not break?

>
> (Slightly related) I also find it annoying that users have to tell the
> kernel whether to use PHY or MAC offload, but have no way to know
> which one their HW supports. That should probably have been an
> implementation detail that didn't need to be part of uapi :/

We could leave the phy / mac netlink keywords and introduce an "on"
option. We deduce whether the device is a phydev or not when on is
passed and set the macsec->offload flag based on that. The phy and mac
options for offload in ip-macsec can then be deprecated.

--
Thanks,

Rahul Rameshbabu

Re: [PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Sabrina Dubroca]

2024-04-22, 22:55:02 -0700, Rahul Rameshbabu wrote:
> On Mon, 22 Apr, 2024 11:23:05 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
> > 2024-04-19, 11:01:20 -0700, Rahul Rameshbabu wrote:
> >> On Fri, 19 Apr, 2024 17:05:52 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote:
> >> > 2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote:
> >> <snip>
> >> >> +			/* 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.
> >> >> +			 */
> >> >> +			DEBUG_NET_WARN_ON_ONCE(true);
> >> >
> >> > If you insist on this warning (and I'm not convinced it's useful,
> >> > since if the HW is already built and cannot inform the driver, there's
> >> > nothing the driver implementer can do), I would move it somewhere into
> >> > the config path. macsec_update_offload would be a better location for
> >> > this kind of warning (maybe with a pr_warn (not limited to debug
> >> > configs) saying something like "MACsec offload on devices that don't
> >> > support md_dst are insecure: they do not provide proper isolation of
> >> > traffic"). The comment can stay here.
> >> >
> >> 
> >> I do not like the warning either. I left it mainly if it needed further
> >> discussion on the mailing list. Will remove it in my next revision. That
> >> said, it may make sense to advertise rx_uses_md_dst over netlink to
> >> annotate what macsec offload path a device uses? Just throwing out an
> >> idea here.
> >
> > Maybe. I was also thinking about adding a way to restrict offloading
> > only to devices with rx_uses_md_dst.
> 
> That's an option. Basically, devices that do not support rx_uses_md_dst
> really only just do SW MACsec but do not return an error if the offload
> parameter is passed over netlink so user scripts do not break?

Forcing a fallback to SW could be considered a breakage because of the
performance regression, so I don't think we can turn this on by
default. Then I would simply reject offload on those devices. We could
have a compat mode that does the SW fallback you suggest. I don't know
if it would be used.


> > (Slightly related) I also find it annoying that users have to tell the
> > kernel whether to use PHY or MAC offload, but have no way to know
> > which one their HW supports. That should probably have been an
> > implementation detail that didn't need to be part of uapi :/
> 
> We could leave the phy / mac netlink keywords and introduce an "on"
> option. We deduce whether the device is a phydev or not when on is
> passed and set the macsec->offload flag based on that. The phy and mac
> options for offload in ip-macsec can then be deprecated.

I thought about doing exactly that, and then dropped the idea because
it would only help with newer kernels.

-- 
Sabrina