| 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
| ||
|
Message-ID: <CADsK2K85L8Q3Q26w2n3WBDpN4VhY0jB8nQgXOhFmutwEHqk60g@mail.gmail.com>
Date: Mon, 2 Dec 2024 12:41:14 -0800
From: Feng Wang <wangfe@...gle.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com,
antony.antony@...unet.com, pabeni@...hat.com
Subject: Re: [PATCH v6] xfrm: add SA information to the offloaded packet when
if_id is set
Updated the comments, thanks.
On Wed, Nov 27, 2024 at 1:00 AM Leon Romanovsky <leon@...nel.org> wrote:
>
> On Tue, Nov 26, 2024 at 01:54:20PM -0800, Feng Wang wrote:
> > Please see embedded answers below. Thanks.
> >
> > On Sun, Nov 24, 2024 at 12:15 AM Leon Romanovsky <leon@...nel.org> wrote:
> > >
> > > On Thu, Nov 21, 2024 at 09:52:58PM +0000, Feng Wang wrote:
> > > > In packet offload mode, append Security Association (SA) information
> > > > to each packet, replicating the crypto offload implementation.
> > >
> > > Please write explicitly WHY "replicating ..." is right thing to do and
> > > why current code is not enough.
> > >
> > > The best thing will be to see how packet traversal in the netdev stack
> > > till it gets to the wire.
> > >
> > I explained why doing this change in the third paragraph based on
> > Steffen's suggestion, I can move it here.
>
> Steffen didn't say "let's replicate ...". All that he said that he wants
> to see API complete.
>
I will move the third paragraph here to explain the reason.
> >
> > > >
> > > > 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 chunk was dropped mysteriously. It doesn't exist in the current patch.
> > > What type of testing did you perform? Can you please add it to the
> > > commit message?
> > >
> > I didn't drop any code in the current patch, I created a test and the
> > patch will be upstreamed after this change is checked in.
> > The link is https://lore.kernel.org/all/20241104233315.3387982-1-wangfe@google.com/
> > Do I need to add the test details in this commit?
>
> No, you need to create test that knows to handle case with and without
> if_id properly. You should add test to the patchset as first patch and
> implementation as second one.
>
I probably don't fully understand what you mean. Before my CL, there
is no handling of if_id. My CL adds this handling and my test shows
how to test it.
What does a test without if_id handling show? Show the code not unable
to handle if_id? And I checked other features, they only add a test
after a new feature is checked in.
> >
> > > According to the strongswan documentation https://docs.strongswan.org/docs/5.9/features/routeBasedVpn.html,
> > > "Traffic that’s routed to an XFRM interface, while no policies and SAs with matching interface ID exist,
> > > will be dropped by the kernel."
> > >
> > > It looks like the current code doesn't handle this case, does it?
> > >
> > The current code will drop the packet if the interface ID is not matched.
>
> How? Who will drop it?
>
The main function is xfrm_policy_match(), you can trace it.
> >
> > > >
> > > > This SA info helps HW offload match packets to their correct security
> > > > policies. The XFRM interface ID is included, which is used in setups
> > > > with multiple XFRM interfaces where source/destination addresses alone
> > > > can't pinpoint the right policy.
> > >
> > > Please add an examples of iproute2 commands on how it is achieved,
> > > together with tcprdump examples of before and after.
> > >
> > A test patch will be upstreamed. There is no need for tcpdump because
> > this change won't change packet content.
>
> Of course it is needed, it will show that without if_id patch packets
> are unencrypted, while after applying the patch, we will see encrypted
> packets.
>
The netdevsim code doesn't really change the packet content because
there is no real hardware encryption engine.
So there is no encrypted packet.
> >
> > > >
> > > > Enable packet offload mode on netdevsim and add code to check the XFRM
> > > > interface ID.
> > >
> > > You still need to add checks in existing drivers to make sure that SAs
> > > with if_id won't be offloaded.
> > >
> > There is no existing driver supporting packet offload mode except netdevsim.
>
> It is not true, please check the code.
>
I have searched the tree, and there is no packet offload mode driver
implementation. If you know it, please let me know.
> >
> > > IMHO, netdevsim implementation is not a replacement to support
> > > out-of-tree code, but a way to help testing features without need to
> > > have complex HW, but still for the code which is in-tree.
> > >
> > We discussed this before, and I followed your and Steffen's comment to
> > add netdevsim simulation code to satisfy the requirement.
>
> I didn't suggest netdevsim and always requested to upstream real driver.
> Netdevsim is Steffen's suggestion to overcome kernel rule of no adding
> code without in-kernel users.
>
> Thanks
>
> >
> > > Thanks
> > >
> >
> > > >
> > > > Signed-off-by: wangfe <wangfe@...gle.com>
> > > > ---
> > > > v6: https://lore.kernel.org/all/20241119220411.2961121-1-wangfe@google.com/
> > > > - Fix code style to follow reverse x-mas tree order declaration.
> > > > v5: https://lore.kernel.org/all/20241112192249.341515-1-wangfe@google.com/
> > > > - Add SA information only when XFRM interface ID is non-zero.
> > > > v4: https://lore.kernel.org/all/20241104233251.3387719-1-wangfe@google.com/
> > > > - Add offload flag check and only doing check when XFRM interface
> > > > ID is non-zero.
> > > > v3: https://lore.kernel.org/all/20240822200252.472298-1-wangfe@google.com/
> > > > - Add XFRM interface ID checking on netdevsim in the packet offload
> > > > mode.
> > > > v2:
> > > > - Add why HW offload requires the SA info to the commit message
> > > > v1: https://lore.kernel.org/all/20240812182317.1962756-1-wangfe@google.com/
> > > > ---
> > > > ---
> > > > drivers/net/netdevsim/ipsec.c | 32 ++++++++++++++++++++++++++++++-
> > > > drivers/net/netdevsim/netdevsim.h | 1 +
> > > > net/xfrm/xfrm_output.c | 22 +++++++++++++++++++++
> > > > 3 files changed, 54 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
> > > > index 88187dd4eb2d..5677b2acf9c0 100644
> > > > --- a/drivers/net/netdevsim/ipsec.c
> > > > +++ b/drivers/net/netdevsim/ipsec.c
> > > > @@ -153,7 +153,8 @@ static int nsim_ipsec_add_sa(struct xfrm_state *xs,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > - if (xs->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {
> > > > + if (xs->xso.type != XFRM_DEV_OFFLOAD_CRYPTO &&
> > > > + xs->xso.type != XFRM_DEV_OFFLOAD_PACKET) {
> > > > NL_SET_ERR_MSG_MOD(extack, "Unsupported ipsec offload type");
> > > > return -EINVAL;
> > > > }
> > > > @@ -169,6 +170,7 @@ static int nsim_ipsec_add_sa(struct xfrm_state *xs,
> > > > memset(&sa, 0, sizeof(sa));
> > > > sa.used = true;
> > > > sa.xs = xs;
> > > > + sa.if_id = xs->if_id;
> > > >
> > > > if (sa.xs->id.proto & IPPROTO_ESP)
> > > > sa.crypt = xs->ealg || xs->aead;
> > > > @@ -227,16 +229,31 @@ static bool nsim_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
> > > > return true;
> > > > }
> > > >
> > > > +static int nsim_ipsec_add_policy(struct xfrm_policy *policy,
> > > > + struct netlink_ext_ack *extack)
> > > > +{
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void nsim_ipsec_del_policy(struct xfrm_policy *policy)
> > > > +{
> > > > +}
> > > > +
> > > > static const struct xfrmdev_ops nsim_xfrmdev_ops = {
> > > > .xdo_dev_state_add = nsim_ipsec_add_sa,
> > > > .xdo_dev_state_delete = nsim_ipsec_del_sa,
> > > > .xdo_dev_offload_ok = nsim_ipsec_offload_ok,
> > > > +
> > > > + .xdo_dev_policy_add = nsim_ipsec_add_policy,
> > > > + .xdo_dev_policy_delete = nsim_ipsec_del_policy,
> > > > +
> > > > };
> > > >
> > > > bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> > > > {
> > > > struct sec_path *sp = skb_sec_path(skb);
> > > > struct nsim_ipsec *ipsec = &ns->ipsec;
> > > > + struct xfrm_offload *xo;
> > > > struct xfrm_state *xs;
> > > > struct nsim_sa *tsa;
> > > > u32 sa_idx;
> > > > @@ -275,6 +292,19 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> > > > return false;
> > > > }
> > > >
> > > > + if (xs->if_id) {
> > > > + if (xs->if_id != tsa->if_id) {
> > > > + netdev_err(ns->netdev, "unmatched if_id %d %d\n",
> > > > + xs->if_id, tsa->if_id);
> > > > + return false;
> > > > + }
> > > > + xo = xfrm_offload(skb);
> > > > + if (!xo || !(xo->flags & XFRM_XMIT)) {
> > > > + netdev_err(ns->netdev, "offload flag missing or wrong\n");
> > > > + return false;
> > > > + }
> > > > + }
> > > > +
> > > > ipsec->tx++;
> > > >
> > > > return true;
> > > > diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> > > > index bf02efa10956..4941b6e46d0a 100644
> > > > --- a/drivers/net/netdevsim/netdevsim.h
> > > > +++ b/drivers/net/netdevsim/netdevsim.h
> > > > @@ -41,6 +41,7 @@ struct nsim_sa {
> > > > __be32 ipaddr[4];
> > > > u32 key[4];
> > > > u32 salt;
> > > > + u32 if_id;
> > > > bool used;
> > > > bool crypt;
> > > > bool rx;
> > > > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > > > index e5722c95b8bb..3e9a1b17f37e 100644
> > > > --- a/net/xfrm/xfrm_output.c
> > > > +++ b/net/xfrm/xfrm_output.c
> > > > @@ -704,6 +704,8 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> > > > {
> > > > struct net *net = dev_net(skb_dst(skb)->dev);
> > > > struct xfrm_state *x = skb_dst(skb)->xfrm;
> > > > + struct xfrm_offload *xo;
> > > > + struct sec_path *sp;
> > > > int family;
> > > > int err;
> > > >
> > > > @@ -728,7 +730,27 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> > > > kfree_skb(skb);
> > > > return -EHOSTUNREACH;
> > > > }
> > > > + if (x->if_id) {
> > > > + sp = secpath_set(skb);
> > > > + if (!sp) {
> > > > + XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > > > + kfree_skb(skb);
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > > + sp->olen++;
> > > > + sp->xvec[sp->len++] = x;
> > > > + xfrm_state_hold(x);
> > > >
> > > > + xo = xfrm_offload(skb);
> > > > + if (!xo) {
> > > > + secpath_reset(skb);
> > > > + XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > > > + kfree_skb(skb);
> > > > + return -EINVAL;
> > > > + }
> > > > + xo->flags |= XFRM_XMIT;
> > > > + }
> > > > return xfrm_output_resume(sk, skb, 0);
> > > > }
> > > >
> > > > --
> > > > 2.47.0.371.ga323438b13-goog
> > > >
> > > >
Powered by blists - more mailing lists