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: <ZshHTlUb/BCtvCT0@gauss3.secunet.de>
Date: Fri, 23 Aug 2024 10:24:46 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Hangbin Liu <liuhangbin@...il.com>
CC: Sabrina Dubroca <sd@...asysnail.net>, <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 04:33:36PM +0800, Hangbin Liu wrote:
> Hi Steffen,
> On Thu, Aug 22, 2024 at 09:10:47AM +0200, Steffen Klassert wrote:
> > > 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.
> > 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.
> > 
> > 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.
> 
> Thanks for your comments. I'm not familiar with IPsec state.
> Do you mean something like
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index f74bacf071fc..8a51d0812564 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -477,6 +477,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
>  	struct net_device *bond_dev = bond->dev;
>  	struct bond_ipsec *ipsec;
>  	struct slave *slave;
> +	struct km_event c;
>  
>  	rcu_read_lock();
>  	slave = rcu_dereference(bond->curr_active_slave);
> @@ -498,6 +499,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
>  	spin_lock_bh(&bond->ipsec_lock);
>  	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
>  		ipsec->xs->xso.real_dev = slave->dev;
> +
> +		ipsec->xs->km.state = XFRM_STATE_VALID;
> +		c.data.hard = 1;
> +		c.portid = 0;
> +		c.event = XFRM_MSG_NEWSA;
> +		km_state_notify(x, &c);

The xfrm stack does that already when inserting the state.

> +
>  		if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
>  			slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__);
>  			ipsec->xs->xso.real_dev = NULL;
> @@ -580,6 +588,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
>  				   "%s: no slave xdo_dev_state_delete\n",
>  				   __func__);
>  		} else {
> +			ipsec->xs->km.state = XFRM_STATE_EXPIRED;

I think you also need to set 'x->km.dying = 1'.

> +			km_state_expired(ipsec->xs, 1, 0);

Please test this at least with libreswan and strongswan. The state is
actually not expired, so not sure if the IKE daemons behave as we want
in that case.

Downside of this approach is that you loose some packets until the new
SA is negotiated, as Sabrina mentioned.