netdev - Re: [PATCHv3 net-next 2/3] bonding: Add ESN support to IPSec HW offload

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: <ZscLLbkZnJmiYViM@gauss3.secunet.de>
Date: Thu, 22 Aug 2024 11:55:57 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Sabrina Dubroca <sd@...asysnail.net>
CC: Hangbin Liu <liuhangbin@...il.com>, <netdev@...r.kernel.org>, Jay Vosburgh
	<j.vosburgh@...il.com>, "David S . Miller" <davem@...emloft.net>, "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Eric Dumazet
	<edumazet@...gle.com>, Nikolay Aleksandrov <razor@...ckwall.org>, "Tariq
 Toukan" <tariqt@...dia.com>, Jianbo Liu <jianbol@...dia.com>, Simon Horman
	<horms@...nel.org>
Subject: Re: [PATCHv3 net-next 2/3] bonding: Add ESN support to IPSec HW
 offload

On Thu, Aug 22, 2024 at 10:39:11AM +0200, Sabrina Dubroca wrote:
> 2024-08-22, 09:10:47 +0200, Steffen Klassert wrote:
> > On Thu, Aug 22, 2024 at 08:33:17AM +0800, Hangbin Liu wrote:
> > > On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote:
> > > > > > > > +	if (!real_dev->xfrmdev_ops ||
> > > > > > > > +	    !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) {
> > > > > > > > +		pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name);
> > > > > > > 
> > > > > > > xdo_dev_state_advance_esn is called on the receive path for every
> > > > > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance ->
> > > > > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to
> > > > > > > be ratelimited.
> > > > > > 
> > > > > > How does xfrm_state offload work on bonding?
> > > > > > Does every slave have its own negotiated SA?
> > > > > 
> > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> > > > > current active slave keep the SA. When active slave changes, the sa on
> > > > > previous slave is deleted and re-added on new active slave.
> > > > 
> > > > It's the same SA, there's no DELSA+NEWSA when we change the active
> > > > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
> > > > the driver/HW), and only a single NEWSA to install the offloaded SA on
> > > > the bond device (which calls the active slave's xdo_dev_state_add).
> > > 
> > > Yes, thanks for the clarification. The SA is not changed, we just delete it
> > > on old active slave
> > > 
> > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> > > 
> > > And add to now one.
> > > 
> > > ipsec->xs->xso.real_dev = slave->dev;
> > > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)
> > 
> > Using the same key on two different devices is very dangerous.
> 
> It's only used by one device at a time, we only support offload with
> "active-backup" mode, where only the current active slave can send
> packets.
> 
> > Counter mode algorithms have the requirement that the IV
> > must not repeat. If you use the same key on two devices,
> > you can't guarantee that. If both devices use an internal
> > counter (initialized to one) to generate the IV, then the
> > IV repeats for the mumber of packets that were already
> > sent on the old device. The algorithm is cryptographically
> > broken in that case.
> 
> Aren't they basing the IV on the sequence number filled in the header?
> If not, then I guess this stuff has been broken since 2020 :(
> (18cb261afd7b ("bonding: support hardware encryption offload to slaves"))

Linux does that, but it is not guaranteed that other devices do that
too. It is perfectly Ok to use some internal counter (or anything
elase that does not repeat) to generate the IV.

> 
> > Instead of moving the existing state, it is better to
> > request a rekey. Maybe by setting the old state to
> > 'expired' and then send a km_state_expired() message.
> 
> But then you're going to drop packets during the whole rekey?

Yes, I know. That would be the downside of that.