Message-ID: <ZiKH52u_sjpm2mhf@hog>
Date: Fri, 19 Apr 2024 17:04:07 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc: netdev@...r.kernel.org, stable@...r.kernel.org,
	Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Paolo Abeni <pabeni@...hat.com>, Gal Pressman <gal@...dia.com>,
	Tariq Toukan <tariqt@...dia.com>,
	Yossi Kuperman <yossiku@...dia.com>,
	Benjamin Poirier <bpoirier@...dia.com>,
	Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net-next 0/3] Resolve security issue in MACsec offload Rx
 datapath

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@...asysnail.net>
> Cc: stable@...r.kernel.org
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@...dia.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

Powered by blists - more mailing lists