| 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: <CADsK2K_Lu1HUyutODQceKWkGgjEYVwowYY4qDm20qyohOh_2DA@mail.gmail.com>
Date: Tue, 19 Nov 2024 11:15:16 -0800
From: Feng Wang <wangfe@...gle.com>
To: Leon Romanovsky <leonro@...dia.com>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com,
antony.antony@...unet.com
Subject: Re: [PATCH] xfrm: add SA information to the offloaded packet
Hi Leon,
Sorry about the format issue, I thought I used the correct format.
Now I try to do it again, if it still has an issue, please let me
know.
I will create a new patch based on your comments.
Thanks,
Feng
On Sat, Nov 16, 2024 at 10:22 PM Leon Romanovsky <leonro@...dia.com> wrote:
>
> On Tue, Nov 12, 2024 at 11:22:49AM -0800, 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 used in setups
> > with multiple XFRM interfaces where source/destination addresses alone
> > can't pinpoint the right policy.
> >
> > Enable packet offload mode on netdevsim and add code to check the XFRM
> > interface ID.
> >
> > Signed-off-by: wangfe <wangfe@...gle.com>
> > ---
> > 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 | 21 ++++++++++++++++++++
> > 3 files changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
> > index f0d58092e7e9..afd2005bf7a8 100644
> > --- a/drivers/net/netdevsim/ipsec.c
> > +++ b/drivers/net/netdevsim/ipsec.c
> > @@ -149,7 +149,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;
> > }
> > @@ -165,6 +166,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;
> > @@ -224,10 +226,24 @@ 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)
> > @@ -237,6 +253,7 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> > struct xfrm_state *xs;
> > struct nsim_sa *tsa;
> > u32 sa_idx;
> > + struct xfrm_offload *xo;
> >
> > /* do we even need to check this packet? */
> > if (!sp)
> > @@ -272,6 +289,19 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> > return false;
> > }
> >
> > + if (xs->if_id) {
>
> If you are checking this, you will need to make sure that XFRM_XMIT is
> set when if_id != 0. It is not how it is implemented now.
>
I will change the code to add the SA only when if_id is non-zero,
then it will make sense.
> > + 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..a12588e7b060 100644
> > --- a/net/xfrm/xfrm_output.c
> > +++ b/net/xfrm/xfrm_output.c
> > @@ -706,6 +706,8 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> > struct xfrm_state *x = skb_dst(skb)->xfrm;
> > int family;
> > int err;
> > + struct xfrm_offload *xo;
> > + struct sec_path *sp;
> >
> > family = (x->xso.type != XFRM_DEV_OFFLOAD_PACKET) ? x->outer_mode.family
> > : skb_dst(skb)->ops->family;
> > @@ -728,6 +730,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> > kfree_skb(skb);
> > return -EHOSTUNREACH;
> > }
> > + 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);
>
> Not an expert and just looked how XFRM_XMIT is assigned in other places,
> and simple skb_ext_add(skb, SKB_EXT_SEC_PATH) is used, why is it
> different here?
>
> The additional issue is that you are adding extension and flag to all
> offloaded packets, which makes me wonder what to do with this check in
> validate_xmit_xfrm():
>
Set the XFRM_XMIT flag makes validate_xmit_xfrm returns immediately
without need to do any further check.
> 136 /* The packet was sent to HW IPsec packet offload engine,
> 137 * but to wrong device. Drop the packet, so it won't skip
> 138 * XFRM stack.
> 139 */
> 140 if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET && x->xso.dev != dev) {
> 141 kfree_skb(skb);
> 142 dev_core_stats_tx_dropped_inc(dev);
> 143 return NULL;
> 144 }
>
> Also because you are adding it to all packets, you are adding extra
> overhead for all users, even these who didn't ask this if_id thing.
>
> And again, you should block creation of SAs with if_id != 0 for already
> existing in-kernel IPsec implementations.
>
> Thanks
>
I will add SA information only when id is non-zero, thus no overhead
for other uses, and no need to block creating of SAs with if_id is 0,
> > +
> > + 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.277.g8800431eea-goog
> >
> >
Powered by blists - more mailing lists