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: <Zsb34DsLwVrDI-w5@Laptop-X1>
Date: Thu, 22 Aug 2024 16:33:36 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Steffen Klassert <steffen.klassert@...unet.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

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);
+
 		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;
+			km_state_expired(ipsec->xs, 1, 0);
 			slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
 		}
 	}