| 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: <20241105073248.GC311159@unreal>
Date: Tue, 5 Nov 2024 09:32:48 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Feng Wang <wangfe@...gle.com>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com,
antony.antony@...unet.com
Subject: Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet
On Mon, Nov 04, 2024 at 03:32:51PM -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>
Something wrong with your git setup, lore shows only one patch.
https://lore.kernel.org/all/20241104233251.3387719-1-wangfe@google.com/
> ---
> 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 | 24 +++++++++++++++++++++++-
> drivers/net/netdevsim/netdevsim.h | 1 +
> net/xfrm/xfrm_output.c | 21 +++++++++++++++++++++
> 3 files changed, 45 insertions(+), 1 deletion(-)
Don't you need to update current drivers who support packet offload with
if(x->if_id) check in their SA/policy validator to return an error to
the user? They don't support that flow.
>
> diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
> index f0d58092e7e9..1ce7447dd269 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)
> @@ -272,6 +288,12 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> return false;
> }
>
> + if (xs->if_id != tsa->if_id) {
if_id exists in policy and state, but you are checking only state.
> + netdev_err(ns->netdev, "unmatched if_id %d %d\n",
> + xs->if_id, tsa->if_id);
> + return false;
> + }
It is worth to add check for having XFRM_XMIT flag here.
> +
> 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;
> }
I think that code below should be set under "if (x->if_id)".
Thanks
> + 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.199.ga7371fff76-goog
>
>
Powered by blists - more mailing lists