[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240828112619.GA8373@unreal>
Date: Wed, 28 Aug 2024 14:26:19 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: Feng Wang <wangfe@...gle.com>, netdev@...r.kernel.org,
antony.antony@...unet.com
Subject: Re: [PATCH] xfrm: add SA information to the offloaded packet
On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> On Thu, Aug 22, 2024 at 01:02:52PM -0700, Feng Wang wrote:
> > From: wangfe <wangfe@...gle.com>
> >
> > In packet offload mode, append Security Association (SA) information
> > to each packet, replicating the crypto offload implementation.
> > The XFRM_XMIT flag is set to enable packet to be returned immediately
> > from the validate_xmit_xfrm function, thus aligning with the existing
> > code path for packet offload mode.
> >
> > This SA info helps HW offload match packets to their correct security
> > policies. The XFRM interface ID is included, which is crucial in setups
> > with multiple XFRM interfaces where source/destination addresses alone
> > can't pinpoint the right policy.
> >
> > Signed-off-by: wangfe <wangfe@...gle.com>
>
> Applied to ipsec-next, thanks Feng!
Stephen, can you please explain why do you think that this is correct
thing to do?
There are no in-tree any drivers which is using this information, and it
is unclear to me how state is released and it has controversial code
around validity of xfrm_offload() too.
For example:
+ sp->olen++;
+ sp->xvec[sp->len++] = x;
+ xfrm_state_hold(x);
+
+ xo = xfrm_offload(skb);
+ if (!xo) { <--- previous code handled this case perfectly in validate_xmit_xfrm
+ secpath_reset(skb);
+ XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+ kfree_skb(skb);
+ return -EINVAL; <--- xfrm state leak
+ }
Can you please revert/drop this patch for now?
Thanks
Powered by blists - more mailing lists