[PATCH net 0/2] MACSec bugfixes related to MAC address change

[PATCH net 0/2] MACSec bugfixes related to MAC address change

[Igor Russkikh]

We found out that there's an issue in MACSec code when the MAC address
is changed.
Both s/w and offloaded implementations don't update SCI when the MAC
address changes at the moment, but they should do so, because SCI contains
MAC in its first 6 octets.

Dmitry Bogdanov (2):
  net: macsec: update SCI upon MAC address change.
  net: macsec: invoke mdo_upd_secy callback when mac address changed

 drivers/net/macsec.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH net 1/2] net: macsec: update SCI upon MAC address change.

[Igor Russkikh]

From: Dmitry Bogdanov <dbogdanov@marvell.com>

SCI should be updated, because it contains MAC in its first 6 octets.

Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
Signed-off-by: Dmitry Bogdanov <dbogdanov@marvell.com>
Signed-off-by: Mark Starovoytov <mstarovoitov@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
---
 drivers/net/macsec.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 45bfd99f17fa..d8e1d9290c47 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -424,6 +424,11 @@ static struct macsec_eth_header *macsec_ethhdr(struct sk_buff *skb)
 	return (struct macsec_eth_header *)skb_mac_header(skb);
 }
 
+static sci_t dev_to_sci(struct net_device *dev, __be16 port)
+{
+	return make_sci(dev->dev_addr, port);
+}
+
 static void __macsec_pn_wrapped(struct macsec_secy *secy,
 				struct macsec_tx_sa *tx_sa)
 {
@@ -3268,6 +3273,7 @@ static int macsec_set_mac_address(struct net_device *dev, void *p)
 
 out:
 	ether_addr_copy(dev->dev_addr, addr->sa_data);
+	macsec->secy.sci = dev_to_sci(dev, MACSEC_PORT_ES);
 	return 0;
 }
 
@@ -3592,11 +3598,6 @@ static bool sci_exists(struct net_device *dev, sci_t sci)
 	return false;
 }
 
-static sci_t dev_to_sci(struct net_device *dev, __be16 port)
-{
-	return make_sci(dev->dev_addr, port);
-}
-
 static int macsec_add_dev(struct net_device *dev, sci_t sci, u8 icv_len)
 {
 	struct macsec_dev *macsec = macsec_priv(dev);
-- 
2.17.1

[PATCH net 2/2] net: macsec: invoke mdo_upd_secy callback when mac address changed

[Igor Russkikh]

From: Dmitry Bogdanov <dbogdanov@marvell.com>

Notify the offload engine about MAC address change to reconfigure it
accordingly.

Fixes: 3cf3227a21d1 ("net: macsec: hardware offloading infrastructure")
Signed-off-by: Dmitry Bogdanov <dbogdanov@marvell.com>
Signed-off-by: Mark Starovoytov <mstarovoitov@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
---
 drivers/net/macsec.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index d8e1d9290c47..9fda72de761c 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3274,6 +3274,19 @@ static int macsec_set_mac_address(struct net_device *dev, void *p)
 out:
 	ether_addr_copy(dev->dev_addr, addr->sa_data);
 	macsec->secy.sci = dev_to_sci(dev, MACSEC_PORT_ES);
+
+	/* If h/w offloading is available, propagate to the device */
+	if (macsec_is_offloaded(macsec)) {
+		const struct macsec_ops *ops;
+		struct macsec_context ctx;
+
+		ops = macsec_get_ops(macsec, &ctx);
+		if (ops) {
+			ctx.secy = &macsec->secy;
+			macsec_offload(ops->mdo_upd_secy, &ctx);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH net 0/2] MACSec bugfixes related to MAC address change

[David Miller]

From: Igor Russkikh <irusskikh@marvell.com>
Date: Tue, 10 Mar 2020 18:22:23 +0300

> We found out that there's an issue in MACSec code when the MAC address
> is changed.
> Both s/w and offloaded implementations don't update SCI when the MAC
> address changes at the moment, but they should do so, because SCI contains
> MAC in its first 6 octets.

Series applied and patch #1 queued up for -stable.

Re: [PATCH net 1/2] net: macsec: update SCI upon MAC address change.

[Sabrina Dubroca]

Hello,

2020-03-10, 18:22:24 +0300, Igor Russkikh wrote:
> From: Dmitry Bogdanov <dbogdanov@marvell.com>
> 
> SCI should be updated, because it contains MAC in its first 6 octets.

Sorry for catching this so late. I don't think this change is correct.

Changing the SCI means wpa_supplicant (or whatever MKA you're using)
will disagree as to which SCI is in use. The peer probably doesn't
have an RXSC for the new SCI either, so the packets will be dropped
anyway.

Plus, if you're using "send_sci on", there's no real reason to change
the SCI, since it's also in the packet, and may or may not have any
relationship to the MAC address of the device.

I'm guessing the issue you're trying to solve is that in the "send_sci
off" case, macsec_encrypt() will use the SCI stored in the secy, but
the receiver will construct the SCI based on the source MAC
address. Can you confirm that? If that's the real problem, I have a
couple of ideas to solve it.


Thanks, and sorry again for the delay in looking at this,

-- 
Sabrina

RE: [EXT] Re: [PATCH net 1/2] net: macsec: update SCI upon MAC address change.

[Dmitry Bogdanov]

Hi Sabrina,

Thanks for the feedback.
But this patch  does not directly related to send_sci parameter.

Any  manual change of macsec interface by ip tool will break wpa_supplicant work. It's OK, they are not intended to be used together.

Having a different MAC address on  each macsec interface allows to make a configuration with several *offloaded* SecY.  That is to make feasible to route the ingress decrypted traffic to the right (macsecX /ethX) interface by DST address. And to apply a different SecY for the egress packets by SRC address. That is the only option for the macsec offload at PHY level when upper layers know nothing about macsec.


BR,
 Dmitry

-----Original Message-----
From: Sabrina Dubroca <sd@queasysnail.net> 
Sent: Friday, April 17, 2020 12:06 PM
To: Igor Russkikh <irusskikh@marvell.com>
Cc: netdev@vger.kernel.org; Mark Starovoytov <mstarovoitov@marvell.com>; Antoine Tenart <antoine.tenart@bootlin.com>; Dmitry Bogdanov <dbogdanov@marvell.com>
Subject: [EXT] Re: [PATCH net 1/2] net: macsec: update SCI upon MAC address change.

External Email

----------------------------------------------------------------------
Hello,

2020-03-10, 18:22:24 +0300, Igor Russkikh wrote:
> From: Dmitry Bogdanov <dbogdanov@marvell.com>
> 
> SCI should be updated, because it contains MAC in its first 6 octets.

Sorry for catching this so late. I don't think this change is correct.

Changing the SCI means wpa_supplicant (or whatever MKA you're using) will disagree as to which SCI is in use. The peer probably doesn't have an RXSC for the new SCI either, so the packets will be dropped anyway.

Plus, if you're using "send_sci on", there's no real reason to change the SCI, since it's also in the packet, and may or may not have any relationship to the MAC address of the device.

I'm guessing the issue you're trying to solve is that in the "send_sci off" case, macsec_encrypt() will use the SCI stored in the secy, but the receiver will construct the SCI based on the source MAC address. Can you confirm that? If that's the real problem, I have a couple of ideas to solve it.


Thanks, and sorry again for the delay in looking at this,

--
Sabrina

Re: [EXT] Re: [PATCH net 1/2] net: macsec: update SCI upon MAC address change.

[Sabrina Dubroca]

2020-04-20, 09:51:23 +0000, Dmitry Bogdanov wrote:
> Hi Sabrina,
> 
> Thanks for the feedback.
> But this patch  does not directly related to send_sci parameter.
>
> Any manual change of macsec interface by ip tool will break
> wpa_supplicant work. It's OK, they are not intended to be used
> together.

Are you sure?  Before this patch, if you change the MAC address on a
macsec device with "send_sci on", packets can still be encrypted and
decrypted correctly.

> Having a different MAC address on each macsec interface allows to
> make a configuration with several *offloaded* SecY.

If you need to change the MAC address on the macsec device anyway, why
not do that at creation? Then the SCI is already picked:

  ip link add link ens3 macsec0 addr 92:23:25:22:bf:bc type macsec
  ip macsec show
      13: macsec0: [...]
          [...]
          TXSC: 92232522bfbc0001 on SA 0


> That is to make
> feasible to route the ingress decrypted traffic to the right
> (macsecX /ethX) interface by DST address. And to apply a different
> SecY for the egress packets by SRC address. That is the only option
> for the macsec offload at PHY level when upper layers know nothing
> about macsec.

I see. It would have been nice to have all this information in the
commit message.

I'm concerned about the software implementation, and that patch
is changing its behavior, AFAICT without fixing a bug in it.

-- 
Sabrina