netdev - Re: [RFC net-next v2 5/5] net: phy: nxp-c45-tja11xx: implement 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: <518c11e9000f895fddb5b3dc4d5b2bf445cf320f.camel@nxp.com>
Date: Fri, 1 Sep 2023 09:09:06 +0000
From: Radu Pirea <radu-nicolae.pirea@....com>
To: "atenart@...nel.org" <atenart@...nel.org>, "Radu-nicolae Pirea (OSS)"
	<radu-nicolae.pirea@....nxp.com>, "sd@...asysnail.net" <sd@...asysnail.net>
CC: "andrew@...n.ch" <andrew@...n.ch>, "linux@...linux.org.uk"
	<linux@...linux.org.uk>, "hkallweit1@...il.com" <hkallweit1@...il.com>,
	"davem@...emloft.net" <davem@...emloft.net>, Sebastian Tobuschat
	<sebastian.tobuschat@....com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"richardcochran@...il.com" <richardcochran@...il.com>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC net-next v2 5/5] net: phy: nxp-c45-tja11xx: implement
 mdo_insert_tx_tag

On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote:
...

> And it's not restored when the link goes back up? That's inconvenient
> :/
> Do we end up with inconsistent state? ie driver and core believe
> everything is still offloaded, but HW lost all state? do we leak
> some resources allocated by the driver?

Yes. We end up with inconsistent state. The HW will lost all state when
the phy is reseted. No resource is leaked, everything is there, but the
configuration needs to be reapplied.

> 
> We could add a flush/restore in macsec_notify when the lower device
> goes down/up, maybe limited to devices that request this (I don't
> know
> if all devices would need it, or maybe all devices offloading to the
> PHY but not to the MAC).

Agreed.
We can do a flush very simple, but to restore the configuration maybe
we should to save the key in the macsec_key structure. I am not sure if
the key can be extracted from crypto_aead structure.

> 
> And what happens in this case?
>     ip link add link eth0 type macsec offload phy
>     ip link set eth0 down
>     ip macsec add macsec0 rx sci ...
>     ip macsec add macsec0 tx sa 0 ...
>     # etc
>     ip link set eth0 up
> 
> Will offload work with the current code?

(the interface was up before)
[root@...rm ~]# ip link add link end0 macsec0 type macsec encrypt on
offload phy 
[root@...rm ~]# ip link set end0 down
[root@...rm ~]# ip macsec add macsec0 rx port 1 address
00:01:be:be:ef:33
RTNETLINK answers: Operation not supported

But let's consider the next case:
    ip link add link eth0 type macsec offload phy
    ip link set eth0 down
    ip link set eth0 up
    ip macsec add macsec0 rx sci ...
    ip macsec add macsec0 tx sa 0 ...
    # etc

In this case, any HW configuration written by .mdo_add_secy will be
lost.

> 
> > The only drawback is related to the PTP frames encryption. Due to
> > hardware
> > limitations, PHY timestamping + MACsec will not work if the custom
> > header is
> > inserted. The only way to get this work is by using the MAC SA
> > selection and
> > running PTP on the real netdev.
> 
> Could you add some documentation explaining that? Users need this
> information to make the right choice for their use case. Maybe
> directly in the description of the module parameter, something like:
> "Select the TX SC using TLV header information. PTP frames encryption
> cannot work when this feature is enabled."
> 
> If it's in the module parameter I guess it can't be too
> verbose. Otherwise I don't know where else to put it.
> 
> And the parameter's name and/or description should probably include
> macsec/MACsec if it's visible at the level of the whole module (ie if
> macsec support isn't a separate module), just to give context at to
> what the TXSC is (and what the encryption for the PTP frames refers
> to).

I will improve the comment and change the name. Thank you.