| 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: <CO1PR18MB46664807F3F6AA34193F34BFA19C9@CO1PR18MB4666.namprd18.prod.outlook.com>
Date: Mon, 17 Apr 2023 17:37:02 +0000
From: Subbaraya Sundeep Bhatta <sbhatta@...vell.com>
To: Emeel Hakim <ehakim@...dia.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
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.
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