[PATCH v3] net: macsec SCI assignment for ES = 0

[PATCH v3] net: macsec SCI assignment for ES = 0

[carlos.fernandez]

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. Current kernel does not implement
it and uses the header MAC address.

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

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

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 3427993f94f7..c188473f1532 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -256,16 +256,31 @@ 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)
 {
+	struct macsec_dev *macsec_device;
 	sci_t sci;
 
-	if (sci_present)
+	if (sci_present) {
 		memcpy(&sci, hdr->secure_channel_id,
-		       sizeof(hdr->secure_channel_id));
-	else
+			sizeof(hdr->secure_channel_id));
+	} else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {
+		list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
+			struct macsec_rx_sc *rx_sc;
+			struct macsec_secy *secy = &macsec_device->secy;
+
+			for_each_rxsc(secy, rx_sc) {
+				rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
+				if (rx_sc && rx_sc->active)
+					return rx_sc->sci;
+			}
+		}
+		/* If not found, use MAC in hdr as default*/
 		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
-
+	} else {
+		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
+	}
 	return sci;
 }
 
@@ -1150,11 +1165,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.34.1

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

[Jakub Kicinski]

A few nit picks and questions, when you repost please make sure to CC 
Sabrina Dubroca <sd@queasysnail.net>

On Tue, 20 Jun 2023 11:13:01 +0200
carlos.fernandez@technica-engineering.de wrote:
> -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)
>  {
> +	struct macsec_dev *macsec_device;
>  	sci_t sci;
>  
> -	if (sci_present)
> +	if (sci_present) {
>  		memcpy(&sci, hdr->secure_channel_id,
> -		       sizeof(hdr->secure_channel_id));
> -	else
> +			sizeof(hdr->secure_channel_id));

the alignment of sizeof() was correct, don't change it

> +	} else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {

Just
	} else if (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC)) {

> +		list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
> +			struct macsec_rx_sc *rx_sc;
> +			struct macsec_secy *secy = &macsec_device->secy;

You should reorder these two declaration, networking likes local
variable declaration lines longest to shortest.

> +			for_each_rxsc(secy, rx_sc) {
> +				rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> +				if (rx_sc && rx_sc->active)
> +					return rx_sc->sci;
> +			}

I haven't looked in detail but are you possibly returning rx_sc->sci
here just to ...

> +		}
> +		/* If not found, use MAC in hdr as default*/
>  		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> +	} else {
> +		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +	}
>  	return sci;
>  }
>  
> @@ -1150,11 +1165,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);

... look up the rx_sc based on the sci? 
-- 
pw-bot: cr

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

[Carlos Fernandez]

Hi Jakub, 

Sure, I'll add Sabrina Dubroca and add your suggestions in the next patch version.

The return of the rx_sci is the goal of this patch. When ES = 0 and SC = 0, we should return it instead of the hdr MAC addr. If not, MACSec communication will not work when using a switch between the transmitter and receiver, frames will be dropped.

Thanks, 

________________________________________
From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, June 22, 2023 2:34 AM
To: Carlos Fernandez
Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sabrina Dubroca
Subject: Re: [PATCH v3] net: macsec SCI assignment for ES = 0

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

A few nit picks and questions, when you repost please make sure to CC
Sabrina Dubroca <sd@queasysnail.net>

On Tue, 20 Jun 2023 11:13:01 +0200
carlos.fernandez@technica-engineering.de wrote:
> -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)
>  {
> +     struct macsec_dev *macsec_device;
>       sci_t sci;
>
> -     if (sci_present)
> +     if (sci_present) {
>               memcpy(&sci, hdr->secure_channel_id,
> -                    sizeof(hdr->secure_channel_id));
> -     else
> +                     sizeof(hdr->secure_channel_id));

the alignment of sizeof() was correct, don't change it

> +     } else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {

Just
        } else if (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC)) {

> +             list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
> +                     struct macsec_rx_sc *rx_sc;
> +                     struct macsec_secy *secy = &macsec_device->secy;

You should reorder these two declaration, networking likes local
variable declaration lines longest to shortest.

> +                     for_each_rxsc(secy, rx_sc) {
> +                             rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> +                             if (rx_sc && rx_sc->active)
> +                                     return rx_sc->sci;
> +                     }

I haven't looked in detail but are you possibly returning rx_sc->sci
here just to ...

> +             }
> +             /* If not found, use MAC in hdr as default*/
>               sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> +     } else {
> +             sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +     }
>       return sci;
>  }
>
> @@ -1150,11 +1165,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);

... look up the rx_sc based on the sci?
--
pw-bot: cr

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

[Carlos Fernandez]

Hi Jakub,

Also, about the double look up:
I know it's there, but I tried to only change the function that returns the correct SCI in every case. Also, it should be a 1 and only item list. I do not think this should be dangerous or slow.

Thanks,

________________________________________
From: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
Sent: Thursday, June 22, 2023 10:00 AM
To: Jakub Kicinski
Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sabrina Dubroca
Subject: Re: [PATCH v3] net: macsec SCI assignment for ES = 0

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

Hi Jakub,

Sure, I'll add Sabrina Dubroca and add your suggestions in the next patch version.

The return of the rx_sci is the goal of this patch. When ES = 0 and SC = 0, we should return it instead of the hdr MAC addr. If not, MACSec communication will not work when using a switch between the transmitter and receiver, frames will be dropped.

Thanks,

________________________________________
From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, June 22, 2023 2:34 AM
To: Carlos Fernandez
Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sabrina Dubroca
Subject: Re: [PATCH v3] net: macsec SCI assignment for ES = 0

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

A few nit picks and questions, when you repost please make sure to CC
Sabrina Dubroca <sd@queasysnail.net>

On Tue, 20 Jun 2023 11:13:01 +0200
carlos.fernandez@technica-engineering.de wrote:
> -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)
>  {
> +     struct macsec_dev *macsec_device;
>       sci_t sci;
>
> -     if (sci_present)
> +     if (sci_present) {
>               memcpy(&sci, hdr->secure_channel_id,
> -                    sizeof(hdr->secure_channel_id));
> -     else
> +                     sizeof(hdr->secure_channel_id));

the alignment of sizeof() was correct, don't change it

> +     } else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {

Just
        } else if (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC)) {

> +             list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
> +                     struct macsec_rx_sc *rx_sc;
> +                     struct macsec_secy *secy = &macsec_device->secy;

You should reorder these two declaration, networking likes local
variable declaration lines longest to shortest.

> +                     for_each_rxsc(secy, rx_sc) {
> +                             rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> +                             if (rx_sc && rx_sc->active)
> +                                     return rx_sc->sci;
> +                     }

I haven't looked in detail but are you possibly returning rx_sc->sci
here just to ...

> +             }
> +             /* If not found, use MAC in hdr as default*/
>               sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> +     } else {
> +             sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +     }
>       return sci;
>  }
>
> @@ -1150,11 +1165,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);

... look up the rx_sc based on the sci?
--
pw-bot: cr

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

[Sabrina Dubroca]

Hi Carlos,

2023-06-22, 11:49:44 +0000, Carlos Fernandez wrote:
> Hi Jakub,
> 
> Also, about the double look up:
> I know it's there, but I tried to only change the function that returns the correct SCI in every case. Also, it should be a 1 and only item list. I do not think this should be dangerous or slow.

Why is it a 1 item list? Even if that was guaranteed to be true in
normal conditions, we could have a situation where lots of MACsec
SecYs and RXSCs are set up, and packets start hitting this loop.


And could you quote the correct section of 802.1AE? I can't find the
reference for the behavior you're implementing in this patch. All I
can find is (from section 9.5):

    The ES bit is clear if the Source Address is not used to determine the SCI.

    The SC bit shall be clear if an SCI is not present in the SecTAG.

which doesn't say anything about how to interpret both bits being
clear.


(and please don't top-post)


Thanks,

-- 
Sabrina

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

[carlos.fernandez]

Regarding ES, it is only set if the first 6 octets of the SCI are equal to the MAC, 
in which case SC=0 as well (IEEE802.1AE 9.5 TAG Control information). 
However, if ES=0, it is incorrect to use source MAC as SCI (current implementation)

 
Regarding SC, as said in IEEE 802.1AE 9.9:


"An explicitly encoded SCI field in the SecTAG is not required on point-to-point links, 
which are identified by (...), if the transmitting SecY uses only one transmit SC. 
In that case, the secure association created by the SecY for the peer SecYs, together with
the direction of transmission of the secured MPDU, can be used to identify the transmitting SecY."

 
Therefore the case SC=0 is reserved for cases where both conditions apply: point-to-point links, 
and only one transmit SC. This requirement makes the size of the reception lookup 1.
 

In conclusion, if we're in a NON end station MPDU scenario (ES = 0)  and SCI it's not in the SegTAG (SC = 0), 
we need to find the correct SCI. This can be done by searching it at the current (only) active RX_SC.

Thanks
--
Carlos

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

[Sabrina Dubroca]

2023-06-23, 12:13:55 +0200, carlos.fernandez@technica-engineering.de wrote:
> Regarding ES, it is only set if the first 6 octets of the SCI are equal to the MAC, 
> in which case SC=0 as well (IEEE802.1AE 9.5 TAG Control information). 
> However, if ES=0, it is incorrect to use source MAC as SCI (current implementation)

But the spec doesn't really say what else we should be using.

Also, if it's incorrect to use the source MAC to build the SCI, we
shouldn't have the fallback that you added in v2/v3 in response to
Simon's comments (we'll probably end up dropping the packet when we
look for the rxsc/secy anyway).

> Regarding SC, as said in IEEE 802.1AE 9.9:
> 
> 
> "An explicitly encoded SCI field in the SecTAG is not required on point-to-point links, 
> which are identified by (...), if the transmitting SecY uses only one transmit SC. 
> In that case, the secure association created by the SecY for the peer SecYs, together with
> the direction of transmission of the secured MPDU, can be used to identify the transmitting SecY."

Thanks for the reference. This should be quoted in the commit message
to explain the reason for the change, as well as this sentence from 9.5:

    The ES bit is clear if the Source Address is not used to determine
    the SCI.

> Therefore the case SC=0 is reserved for cases where both conditions apply: point-to-point links, 
> and only one transmit SC. This requirement makes the size of the reception lookup 1.

That's the intention of the spec. We can't know that we'll only
receive such packets in situations where we have a single RXSC for the
entire real device.

I'm not too worried about the loop being slow (because it's a bit
unlikely that we'll go through lots of iterations on either loop
before we hit an active RXSC), but I'm not convinced that we should be
dumping those packets in the first RXSC we can find.

So I wonder if we should instead (only for ES=0 SC=0):
 - if we have exactly one RXSC for the lower device (ie across the 2
   loops), use it
 - otherwise (0 or > 1), drop the packet and increment InPktsNoSCI

and we can use MACSEC_UNDEF_SCI as return value to indicate a bogus
packet that needs to be dropped.

Would that take care of your use case?

-- 
Sabrina

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

[carlos.fernandez]

Hi Sabrina, 

Your proposal seems good and fills all the possible alternatives. 
We'll prepare a new patch following your recomendations and send it again.

Thanks
--
Carlos