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

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

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