netdev - Re: [RFC net-next v1 4/5] net: macsec: introduce mdo_insert_tx

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: <ZNfKeCGf0xZTTum7@vergenet.net>
Date: Sat, 12 Aug 2023 20:07:52 +0200
From: Simon Horman <horms@...nel.org>
To: "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@....nxp.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, richardcochran@...il.com, sd@...asysnail.net,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC net-next v1 4/5] net: macsec: introduce mdo_insert_tx_tag

On Fri, Aug 11, 2023 at 06:32:48PM +0300, Radu Pirea (NXP OSS) wrote:
> Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in
> the ethernet frame. This operation will increase the frame size with 32
> bytes. If the frames are sent at line rate, the PHY will not have enough
> room to insert the SecTAG and the ICV.
> 
> To mitigate this scenario, the PHY offer to use require a specific
> ethertype with some padding bytes present in the ethernet frame. This
> ethertype and its associated bytes will be replaced by the SecTAG and ICV.
> 
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@....nxp.com>

...

> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c

...

> @@ -4137,6 +4211,11 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
>  			if (err)
>  				goto del_dev;
>  		}
> +
> +		dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
> +		dev->needed_headroom += ops->needed_headroom;
> +		dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
> +		dev->needed_tailroom += ops->needed_tailroom;

Hi Radu,

Just above the beginning of this hunk it is assumed that ops may be NULL.
However, here it is dereferenced unconditionally. Is this safe?

Flagged by Smatch.

>  	}
>  
>  	err = register_macsec_dev(real_dev, dev);
> diff --git a/include/net/macsec.h b/include/net/macsec.h
> index 33dc7f2aa42e..a988249d9608 100644
> --- a/include/net/macsec.h
> +++ b/include/net/macsec.h
> @@ -272,6 +272,7 @@ struct macsec_context {
>  		struct macsec_rx_sa_stats *rx_sa_stats;
>  		struct macsec_dev_stats  *dev_stats;
>  	} stats;
> +	struct sk_buff *skb;

Not strictly related to this patch,
but it would be nice to update the kernel doc for this
structure so that it's fields are documented.

>  };
>  
>  /**
> @@ -302,6 +303,10 @@ struct macsec_ops {
>  	int (*mdo_get_tx_sa_stats)(struct macsec_context *ctx);
>  	int (*mdo_get_rx_sc_stats)(struct macsec_context *ctx);
>  	int (*mdo_get_rx_sa_stats)(struct macsec_context *ctx);
> +	/* Offload tag */
> +	int (*mdo_insert_tx_tag)(struct macsec_context *ctx);
> +	int needed_headroom;
> +	int needed_tailroom;

Ditto.

>  };
>  
>  #if IS_ENABLED(CONFIG_MACSEC)
> -- 
> 2.34.1
> 
>