netdev - Re: [PATCH v3] net: macsec SCI assignment for ES = 0

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230621173429.18348fc8@kernel.org>
Date: Wed, 21 Jun 2023 17:34:29 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: carlos.fernandez@...hnica-engineering.de
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Sabrina Dubroca
 <sd@...asysnail.net>
Subject: Re: [PATCH v3] net: macsec SCI assignment for ES = 0

A few nit picks and questions, when you repost please make sure to CC 
Sabrina Dubroca <sd@...asysnail.net>

On Tue, 20 Jun 2023 11:13:01 +0200
carlos.fernandez@...hnica-engineering.de wrote:
> -static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present)
> +static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> +			      struct macsec_rxh_data *rxd)
>  {
> +	struct macsec_dev *macsec_device;
>  	sci_t sci;
>  
> -	if (sci_present)
> +	if (sci_present) {
>  		memcpy(&sci, hdr->secure_channel_id,
> -		       sizeof(hdr->secure_channel_id));
> -	else
> +			sizeof(hdr->secure_channel_id));

the alignment of sizeof() was correct, don't change it

> +	} else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {

Just
	} else if (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC)) {

> +		list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
> +			struct macsec_rx_sc *rx_sc;
> +			struct macsec_secy *secy = &macsec_device->secy;

You should reorder these two declaration, networking likes local
variable declaration lines longest to shortest.

> +			for_each_rxsc(secy, rx_sc) {
> +				rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> +				if (rx_sc && rx_sc->active)
> +					return rx_sc->sci;
> +			}

I haven't looked in detail but are you possibly returning rx_sc->sci
here just to ...

> +		}
> +		/* If not found, use MAC in hdr as default*/
>  		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> +	} else {
> +		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +	}
>  	return sci;
>  }
>  
> @@ -1150,11 +1165,12 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>  
>  	macsec_skb_cb(skb)->has_sci = !!(hdr->tci_an & MACSEC_TCI_SC);
>  	macsec_skb_cb(skb)->assoc_num = hdr->tci_an & MACSEC_AN_MASK;
> -	sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci);
>  
>  	rcu_read_lock();
>  	rxd = macsec_data_rcu(skb->dev);
>  
> +	sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci, rxd);
> +
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci);

... look up the rx_sc based on the sci? 
-- 
pw-bot: cr