| Message ID | 20200310152225.2338-2-irusskikh@marvell.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | MACSec bugfixes related to MAC address change | expand |
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,
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
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.
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);