| 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: <IA1PR12MB63536C1FFAE366790A03FF49AB9F9@IA1PR12MB6353.namprd12.prod.outlook.com>
Date: Sun, 16 Apr 2023 12:14:38 +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: 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)
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."
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