netdev - Re: [PATCH net v3] macsec: MACsec SCI assignment for ES = 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aEEp1IOlVY9BrXVY@4c2d361be676>
Date: Thu, 5 Jun 2025 05:23:32 +0000
From: Subbaraya Sundeep <sbhatta@...vell.com>
To: <carlos.fernandez@...hnica-engineering.de>
CC: <andreu.montiel@...hnica-enginnering.de>,
        Andreu Montiel
	<Andreu.Montiel@...hnica-engineering.de>,
        Sabrina Dubroca
	<sd@...asysnail.net>,
        Andrew Lunn <andrew+netdev@...n.ch>,
        "David S. Miller"
	<davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Hannes Frederic Sowa
	<hannes@...essinduktion.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net v3] macsec: MACsec SCI assignment for ES = 0

On 2025-06-04 at 12:33:55, carlos.fernandez@...hnica-engineering.de (carlos.fernandez@...hnica-engineering.de) wrote:
> From: Carlos Fernandez <carlos.fernandez@...hnica-engineering.de>
> 
> According to 802.1AE standard, when ES and SC flags in TCI are zero,
> used SCI should be the current active SC_RX. Current code uses the
> header MAC address. Without this patch, when ES flag is 0 (using a
> bridge or switch), header MAC will not fit the SCI and MACSec frames
> will be discarted.
> 
> Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
> Co-developed-by: Andreu Montiel <Andreu.Montiel@...hnica-engineering.de>
> Signed-off-by: Andreu Montiel <Andreu.Montiel@...hnica-engineering.de>
> Signed-off-by: Carlos Fernandez <carlos.fernandez@...hnica-engineering.de>

 Reviewed-by: Subbaraya Sundeep <sbhatta@...vell.com>

Also please let me know how to test this single secy and single
RXSC for my understanding.

Thanks,
Sundeep

> ---
> v3:
> * Wrong drop frame afer macsec_frame_sci
> * Wrong Fixes tag in message 
> 
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20250604113213.2595524-1-carlos.fernandez@technica-engineering.de/
> * Active sci lookup logic in a separate helper.
> * Unnecessary loops avoided. 
> * Check RXSC is exactly one for lower device.
> * Drops frame in case of error.
> 
> 
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20250529124455.2761783-1-carlos.fernandez@technica-engineering.de/
> 
>  drivers/net/macsec.c | 40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 3d315e30ee47..7edbe76b5455 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -247,15 +247,39 @@ static sci_t make_sci(const u8 *addr, __be16 port)
>  	return sci;
>  }
>  
> -static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present)
> +static sci_t macsec_active_sci(struct macsec_secy *secy)
>  {
> -	sci_t sci;
> +	struct macsec_rx_sc *rx_sc = rcu_dereference_bh(secy->rx_sc);
> +
> +	/* Case single RX SC */
> +	if (rx_sc && !rcu_dereference_bh(rx_sc->next))
> +		return (rx_sc->active) ? rx_sc->sci : 0;
> +	/* Case no RX SC or multiple */
> +	else
> +		return 0;
> +}
> +
> +static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> +			      struct macsec_rxh_data *rxd)
> +{
> +	struct macsec_dev *macsec;
> +	sci_t sci = 0;
>  
> -	if (sci_present)
> +	/* SC = 1 */
> +	if (sci_present) {
>  		memcpy(&sci, hdr->secure_channel_id,
>  		       sizeof(hdr->secure_channel_id));
> -	else
> +	/* SC = 0; ES = 0 */
> +	} else if ((!(hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) &&
> +		   (list_is_singular(&rxd->secys))) {
> +		/* Only one SECY should exist on this scenario */
> +		macsec = list_first_or_null_rcu(&rxd->secys, struct macsec_dev,
> +						secys);
> +		if (macsec)
> +			return macsec_active_sci(&macsec->secy);
> +	} else {
>  		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +	}
>  
>  	return sci;
>  }
> @@ -1109,7 +1133,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>  	struct macsec_rxh_data *rxd;
>  	struct macsec_dev *macsec;
>  	unsigned int len;
> -	sci_t sci;
> +	sci_t sci = 0;
>  	u32 hdr_pn;
>  	bool cbit;
>  	struct pcpu_rx_sc_stats *rxsc_stats;
> @@ -1156,11 +1180,14 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>  
>  	macsec_skb_cb(skb)->has_sci = !!(hdr->tci_an & MACSEC_TCI_SC);
>  	macsec_skb_cb(skb)->assoc_num = hdr->tci_an & MACSEC_AN_MASK;
> -	sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci);
>  
>  	rcu_read_lock();
>  	rxd = macsec_data_rcu(skb->dev);
>  
> +	sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci, rxd);
> +	if (!sci)
> +		goto drop_nosc;
> +
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci);
>  
> @@ -1283,6 +1310,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>  	macsec_rxsa_put(rx_sa);
>  drop_nosa:
>  	macsec_rxsc_put(rx_sc);
> +drop_nosc:
>  	rcu_read_unlock();
>  drop_direct:
>  	kfree_skb(skb);
> -- 
> 2.43.0
>