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

[Steffen Klassert <steffen.klassert@secunet.com>]

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.