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 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.
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.