netdev - Re: [PATCH] xfrm: add SA information to the offloaded packet

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADsK2K-mnrz8TV-8-BvBU0U9DDzJhZF2GGM22vgA6GMpvK556w@mail.gmail.com>
Date: Wed, 28 Aug 2024 14:25:22 -0700
From: Feng Wang <wangfe@...gle.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: Steffen Klassert <steffen.klassert@...unet.com>, netdev@...r.kernel.org, 
	antony.antony@...unet.com
Subject: Re: [PATCH] xfrm: add SA information to the offloaded packet

Hi Leon,

Thank you for your insightful questions and comments.

Just like in crypto offload mode, now pass SA (Security Association)
information to the driver in packet offload mode. This helps the
driver quickly identify the packet's source and destination, rather
than having to parse the packet itself. The xfrm interface ID is
especially useful here. As you can see in the kernel code
(https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_policy.c#L1993),
it's used to differentiate between various policies in complex network
setups.

During my testing of packet offload mode without the patch, I observed
that the sec_path was NULL. Consequently, xo was also NULL when
validate_xmit_xfrm was called, leading to an immediate return at line
125 (https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_device.c#L125).

Regarding the potential xfrm_state leak, upon further investigation,
it may not be the case. It seems that both secpath_reset and kfree_skb
invoke the xfrm_state_put function, which should be responsible for
releasing the state. The call flow appears to be as follows: kfree_skb
-> skb_release_all -> skb_release_head_state -> skb_ext_put ->
skb_ext_put_sp -> xfrm_state_put.

Please let me know if you have any further questions or concerns. I
appreciate your valuable feedback!

Feng

On Wed, Aug 28, 2024 at 4:26 AM Leon Romanovsky <leon@...nel.org> wrote:
>
> 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