Re: [PATCH net-next 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Sabrina Dubroca <sd@queasysnail.net>]

2024-04-18, 18:17:16 -0700, Rahul Rameshbabu wrote:
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 0206b84284ab..679302ef1cd9 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -991,6 +991,19 @@ static struct macsec_rx_sc *find_rx_sc_rtnl(struct macsec_secy *secy, sci_t sci)
>  	return NULL;
>  }
>  
> +static __u8 macsec_offload_pkt_type(const u8 *h_dest, const u8 *ndev_broadcast)
> +

nit: empty line shouldn't be here

> +{
> +	if (is_multicast_ether_addr_64bits(h_dest)) {
> +		if (ether_addr_equal_64bits(h_dest, ndev_broadcast))
> +			return PACKET_BROADCAST;
> +		else
> +			return PACKET_MULTICAST;
> +	}
> +
> +	return PACKET_HOST;
> +}
> +
>  static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  {
>  	/* Deliver to the uncontrolled port by default */
> @@ -999,10 +1012,12 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  	struct metadata_dst *md_dst;
>  	struct macsec_rxh_data *rxd;
>  	struct macsec_dev *macsec;
> +	bool is_macsec_md_dst;
>  
>  	rcu_read_lock();
>  	rxd = macsec_data_rcu(skb->dev);
>  	md_dst = skb_metadata_dst(skb);
> +	is_macsec_md_dst = md_dst && md_dst->type == METADATA_MACSEC;
>  
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct sk_buff *nskb;
> @@ -1014,13 +1029,40 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  		 */
>  		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
>  			struct macsec_rx_sc *rx_sc = NULL;

Please move this into the "if (is_macsec_md_dst)" block below, since
it's no longer used outside.

> +			const struct macsec_ops *ops;
>  
> -			if (md_dst && md_dst->type == METADATA_MACSEC)
> -				rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
> +			ops = macsec_get_ops(macsec, NULL);
>  
> -			if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc)
> +			if (ops->rx_uses_md_dst && !is_macsec_md_dst)
>  				continue;
>  
> +			if (is_macsec_md_dst) {
> +				/* All drivers that implement MACsec offload
> +				 * support using skb metadata destinations must
> +				 * indicate that they do so.
> +				 */
> +				DEBUG_NET_WARN_ON_ONCE(!ops->rx_uses_md_dst);
> +				rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
> +				if (!rx_sc)
> +					continue;
> +				/* device indicated macsec offload occurred */
> +				skb->dev = ndev;
> +				skb->pkt_type = macsec_offload_pkt_type(
> +					hdr->h_dest, ndev->broadcast);
> +				ret = RX_HANDLER_ANOTHER;
> +				goto out;
> +			}
> +
> +			/* This datapath is insecure because it is unable to
> +			 * enforce isolation of broadcast/multicast traffic and
> +			 * unicast traffic with promiscuous mode on the macsec
> +			 * netdev. Since the core stack has no mechanism to
> +			 * check that the hardware did indeed receive MACsec
> +			 * traffic, it is possible that the response handling
> +			 * done by the MACsec port was to a plaintext packet.
> +			 * This violates the MACsec protocol standard.
> +			 */
> +			DEBUG_NET_WARN_ON_ONCE(true);

If you insist on this warning (and I'm not convinced it's useful,
since if the HW is already built and cannot inform the driver, there's
nothing the driver implementer can do), I would move it somewhere into
the config path. macsec_update_offload would be a better location for
this kind of warning (maybe with a pr_warn (not limited to debug
configs) saying something like "MACsec offload on devices that don't
support md_dst are insecure: they do not provide proper isolation of
traffic"). The comment can stay here.

>  			if (ether_addr_equal_64bits(hdr->h_dest,
>  						    ndev->dev_addr)) {
>  				/* exact match, divert skb to this port */

-- 
Sabrina