From: Carlos Fernandez <carlos.fernandez@technica-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 but current code uses the header
MAC address.

Without this patch, when ES flag is 0 (using a bridge or switch), header
MAC will not be equal to the SCI and MACSec frames will be discarted.

Signed-off-by: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
---
 drivers/net/macsec.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 3d315e30ee47..9a743aee2cea 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -247,15 +247,29 @@ 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_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
+			      struct macsec_rxh_data *rxd)
 {
-	sci_t sci;
+	struct macsec_dev *macsec_device;
+	sci_t sci = 0;
 
-	if (sci_present)
+	if (sci_present) {
 		memcpy(&sci, hdr->secure_channel_id,
 		       sizeof(hdr->secure_channel_id));
-	else
+	} else if (!(hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {
+		list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
+			struct macsec_secy *secy = &macsec_device->secy;
+			struct macsec_rx_sc *rx_sc;
+
+			for_each_rxsc(secy, rx_sc) {
+				rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
+				if (rx_sc && rx_sc->active)
+					sci = rx_sc->sci;
+			}
+		}
+	} else {
 		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
+	}
 
 	return sci;
 }
@@ -1156,11 +1170,12 @@ 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);
+
 	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
 		struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci);
 
-- 
2.43.0

Hi, 

I'll ammend the patch and send it to net-next instead.

Thanks, 
Carlos

On 6/3/25 10:06 AM, Carlos Fernandez wrote:
> I'll ammend the patch and send it to net-next instead.

While at it:
- please include the changelog comprising a reference (URL) to the prior
discussion after the tag area and a '---' separator
- please include the patch revision in the subj prefix (v5 AFAICS)
- consider extracting the active sci lookup logic in a separate helper,
will probably simplify the code and will reduce the indentation.

Thanks,

Paolo

Hi,

On 2025-05-29 at 12:44:42, carlos.fernandez@technica-engineering.de (carlos.fernandez@technica-engineering.de) wrote:
> From: Carlos Fernandez <carlos.fernandez@technica-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 but current code uses the header
> MAC address.
> 
> Without this patch, when ES flag is 0 (using a bridge or switch), header
> MAC will not be equal to the SCI and MACSec frames will be discarted.
> 
> Signed-off-by: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
> ---
>  drivers/net/macsec.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 3d315e30ee47..9a743aee2cea 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -247,15 +247,29 @@ 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_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> +			      struct macsec_rxh_data *rxd)
>  {
> -	sci_t sci;
> +	struct macsec_dev *macsec_device;
> +	sci_t sci = 0;
>  
> -	if (sci_present)
> +	if (sci_present) {
>  		memcpy(&sci, hdr->secure_channel_id,
>  		       sizeof(hdr->secure_channel_id));
> -	else
> +	} else if (!(hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {
> +		list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
> +			struct macsec_secy *secy = &macsec_device->secy;
> +			struct macsec_rx_sc *rx_sc;
> +
> +			for_each_rxsc(secy, rx_sc) {
> +				rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> +				if (rx_sc && rx_sc->active)
> +					sci = rx_sc->sci;
The intention of this logic is not clear to reader since you want
last sci in list or you forgot to return/break. Digging previous mail
chain you said loop iteration count will only be 1 but we are not
really sure about it. Please change as Sabrina suggested to check whether
RXSC is exactly one for lower device and if not drop the packet.
Write a comment on top of 'else if' so that we dont need to dig
into history of why this logic is like that.
Also this looks like net-next material, if you feel strongly this as
a fix which was missed from beginning then add Fixes tag.

Thanks,
Sundeep


> +			}
> +		}
> +	} else {
>  		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +	}
>  
>  	return sci;
>  }
> @@ -1156,11 +1170,12 @@ 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);
> +
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci);
>  
> -- 
> 2.43.0
>