| 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
| ||
|
Message-ID: <IA1PR12MB63537ADACA647FB2C6CE1E2FAB9D9@IA1PR12MB6353.namprd12.prod.outlook.com>
Date: Tue, 18 Apr 2023 07:05:07 +0000
From: Emeel Hakim <ehakim@...dia.com>
To: Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"sd@...asysnail.net" <sd@...asysnail.net>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"leon@...nel.org" <leon@...nel.org>,
Sunil Kovvuri Goutham <sgoutham@...vell.com>,
Naveen Mamindlapalli <naveenm@...vell.com>
Subject: RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC
address to identify destination MACsec device
> -----Original Message-----
> From: Subbaraya Sundeep Bhatta <sbhatta@...vell.com>
> Sent: Monday, 17 April 2023 20:37
> To: Emeel Hakim <ehakim@...dia.com>; davem@...emloft.net;
> kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
> sd@...asysnail.net
> Cc: netdev@...r.kernel.org; leon@...nel.org; Sunil Kovvuri Goutham
> <sgoutham@...vell.com>; Naveen Mamindlapalli <naveenm@...vell.com>
> Subject: RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC
> address to identify destination MACsec device
>
> External email: Use caution opening links or attachments
>
>
> Hi,
>
> >-----Original Message-----
> >From: Emeel Hakim <ehakim@...dia.com>
> >Sent: Sunday, April 16, 2023 5:45 PM
> >To: Subbaraya Sundeep Bhatta <sbhatta@...vell.com>;
> >davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
> >edumazet@...gle.com; sd@...asysnail.net
> >Cc: netdev@...r.kernel.org; leon@...nel.org; Sunil Kovvuri Goutham
> ><sgoutham@...vell.com>; Naveen Mamindlapalli <naveenm@...vell.com>
> >Subject: [EXT] RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on
> >the dst MAC address to identify destination MACsec device
> >
> >
> >
> >> -----Original Message-----
> >> From: Subbaraya Sundeep Bhatta <sbhatta@...vell.com>
> >> Sent: Friday, 14 April 2023 9:32
> >> To: Emeel Hakim <ehakim@...dia.com>; davem@...emloft.net;
> >> kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
> >> sd@...asysnail.net
> >> Cc: netdev@...r.kernel.org; leon@...nel.org; Sunil Kovvuri Goutham
> >> <sgoutham@...vell.com>; Naveen Mamindlapalli
> ><naveenm@...vell.com>
> >> Subject: RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the
> >> dst MAC address to identify destination MACsec device
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> Hi,
> >>
> >> >-----Original Message-----
> >> >From: Emeel Hakim <ehakim@...dia.com> <ehakim@...dia.com>
> >> >Sent: Thursday, April 13, 2023 4:26 PM
> >> >To: davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
> >> >edumazet@...gle.com; sd@...asysnail.net
> >> >Cc: netdev@...r.kernel.org; leon@...nel.org; Emeel Hakim
> >> ><ehakim@...dia.com>
> >> >Subject: [PATCH net-next v5 5/5] macsec: Don't rely solely on the
> >> >dst MAC address to identify destination MACsec device
> >> >
> >> >Offloading device drivers will mark offloaded MACsec SKBs with the
> >> >corresponding SCI in the skb_metadata_dst so the macsec rx handler
> >> >will know to which interface to divert those skbs, in case of a
> >> >marked skb and a mismatch on the dst MAC address, divert the skb to
> >> >the macsec net_device where the macsec rx_handler will be called to
> >> >consider cases where relying solely on the dst MAC address is insufficient.
> >> >
> >> >One such instance is when using MACsec with a VLAN as an inner
> >> >header, where the packet structure is ETHERNET | SECTAG | VLAN.
> >> >In such a scenario, the dst MAC address in the ethernet header will
> >> >correspond to the VLAN MAC address, resulting in a mismatch.
> >> >
> >>
> >> I did below commands:
> >> ifconfig eth2 up
> >> ip link add link eth2 macsec0 type macsec sci cacbcd4142430002
> >> ifconfig macsec0 hw ether ca:cb:cd:41:42:43 ip macsec offload macsec0
> >> mac ifconfig macsec0 up ip macsec add macsec0 tx sa 0 on pn 5 key 02
> >> 22222222222222222222222222222222 ip macsec add macsec0 rx sci
> >> cacbcd2122230001 ip macsec add macsec0 rx sci
> >> cacbcd2122230001 sa 0 pn 5 on key 01
> >11111111111111111111111111111111
> >> ip link add link macsec0 vlan0 type vlan id 2
> >>
> >> ifconfig vlan0 hw ether ca:cb:cd:21:22:23 ifconfig vlan0 up [
> >> 7106.072451] device
> >> macsec0 entered promiscuous mode [ 7106.077330] device eth2 entered
> >> promiscuous mode
> >>
> >> macsec0 entered promisc mode when upper_dev mac address is not equal
> >> to its mac.
> >> I think we should check if macsec device is in promisc mode instead
> >> of omitting mac address compare.
> >> Also all drivers/hardware do not support md_dst->type ==
> >> METADATA_MACSEC hence if macsec is offloaded and in promisc mode
> >then go for another round.
> >> Correct me if I am wrong.
> >>
> >> Thanks,
> >> Sundeep
> >
> >Hi thanks for bringing this up,
> >
> >I'm new to this region so correct me if I'm wrong, IIUC we are
> >entering promisc mode since the upper_dev mac address is not equal to
> >its mac hence we won't have a match, and in the current code (of this
> >patch) we are covered in case of existing metadata, however in case of
> >a driver/hardware which does not support metadata this approach does
> >not cover the promisc mode case.
> >
> >IMHO I think we should stick to the metadata approach since in the
> >future we might use it to improve macsec offload (such as allowing to
> >have same mac address for both the physical and macsec device and still
> >to be able to send traffic from the physical device this requires
> >depending solely on metadata)
>
> Aren't you doing this already in mlx5 driver? I mean, using metadata you can have
> same mac for macsec and physical device. Also you can have multiple macsec
> interfaces on top of physical interface.
>
> >however this does not prevent supporting drivers/hardwares which does
> >not support metadata.
> >so If I understood you correctly the following approach should cover
> >both
> >cases:
> >
> >@@ -1047,7 +1050,13 @@ static enum rx_handler_result
> >handle_not_macsec(struct sk_buff *skb)
> >
> > ...
> >
> > __netif_rx(nskb);
> >+ } else if (rx_sc || ndev->flags & IFF_PROMISC) {
> >+ skb->dev = ndev;
> >+ skb->pkt_type = PACKET_HOST;
> >+ ret = RX_HANDLER_ANOTHER;
> >+ goto out;
> > }
> >+
> > continue;
> > }
> >
> >if this is not what you meant can you please clarify what did you mean
> >with "go for another round" in " if macsec is offloaded and in promisc
> >mode then go for another round."
> >
>
> Yes I meant this. Above changes look good to me. I was just saying that returning
> RX_HANDLER_ANOTHER from macsec rx handler will make the code calling it
> (__netif_receive_skb_core) to do another round of processing.
ack , I will send a new version with the above changes.
>
> Thanks,
> Sundeep
>
> >Thanks,
> >Emeel
> >
> >> >Signed-off-by: Emeel Hakim <ehakim@...dia.com>
> >> >---
> >> > drivers/net/macsec.c | 13 +++++++++++--
> >> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >> >
> >> >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
> >> >25616247d7a5..c19a45dc6977 100644
> >> >--- a/drivers/net/macsec.c
> >> >+++ b/drivers/net/macsec.c
> >> >@@ -1021,8 +1021,11 @@ static enum rx_handler_result
> >> >handle_not_macsec(struct sk_buff *skb)
> >> > * the SecTAG, so we have to deduce which port to deliver to.
> >> > */
> >> > if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
> >> >- if (md_dst && md_dst->type == METADATA_MACSEC &&
> >> >- (!find_rx_sc(&macsec->secy, md_dst-
> >> >>u.macsec_info.sci)))
> >> >+ struct macsec_rx_sc *rx_sc = (md_dst &&
> >> >+ md_dst->type
> >> >== METADATA_MACSEC) ?
> >> >+
> >> >+ find_rx_sc(&macsec->secy,
> >> >+
> >> >+ md_dst-
> >> >>u.macsec_info.sci) : NULL;
> >> >+
> >> >+ if (md_dst && md_dst->type == METADATA_MACSEC
> >> >+ &&
> >> >!rx_sc)
> >> > continue;
> >> >
> >> > if (ether_addr_equal_64bits(hdr->h_dest,
> >> >@@ -1047,7 +1050,13 @@ static enum rx_handler_result
> >> >handle_not_macsec(struct sk_buff *skb)
> >> > nskb->pkt_type =
> >> >PACKET_MULTICAST;
> >> >
> >> > __netif_rx(nskb);
> >> >+ } else if (rx_sc) {
> >> >+ skb->dev = ndev;
> >> >+ skb->pkt_type = PACKET_HOST;
> >> >+ ret = RX_HANDLER_ANOTHER;
> >> >+ goto out;
> >> > }
> >> >+
> >> > continue;
> >> > }
> >> >
> >> >--
> >> >2.21.3
> >> >
Powered by blists - more mailing lists