linux-kernel - Re: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6bfd5012-a22b-a70e-c052-21f2d47d364f@oracle.com>
Date:   Fri, 1 Dec 2017 11:23:50 -0800
From:   Shannon Nelson <shannon.nelson@...cle.com>
To:     yossefe@...lanox.com, "David S. Miller" <davem@...emloft.net>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     borisp@...lanox.com, kliteyn@...lanox.com, yossiku@...lanox.com
Subject: Re: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

On 11/28/2017 1:49 AM, yossefe@...lanox.com wrote:
> From: Yossef Efraim <yossefe@...lanox.com>
> 
> This patch adds ESN support to IPsec device offload.
> Adding new xfrm device operation to synchronize device ESN.
> 
> Signed-off-by: Yossef Efraim <yossefe@...lanox.com>
> ---
>   include/linux/netdevice.h |  1 +
>   include/net/xfrm.h        | 12 ++++++++++++
>   net/xfrm/xfrm_device.c    |  4 ++--
>   net/xfrm/xfrm_replay.c    |  2 ++
>   4 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7de7656..d4e9198 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -825,6 +825,7 @@ struct xfrmdev_ops {
>   	void	(*xdo_dev_state_free) (struct xfrm_state *x);
>   	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
>   				       struct xfrm_state *x);
> +	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
>   };
>   #endif
>   
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index dc28a98..372bfcb 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1863,6 +1863,14 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>   		       struct xfrm_user_offload *xuo);
>   bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x);
>   
> +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
> +{
> +	struct xfrm_state_offload *xso = &x->xso;
> +
> +	if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn)
> +		xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);

If there isn't an implementation for xdo_dev_state_advance_esn, should 
this even have been called?  See my comment below about checking the 
XFRM_STATE_ESN.

What is the driver expected to do with this?  I would guess that maybe 
the hardware doing the offload needs to know when the ESN is advanced so 
that it can change the ICV calculation accordingly, but that only is 
useful for hardware that knows about ESN.

> +}
> +
>   static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
>   {
>   	struct xfrm_state *x = dst->xfrm;
> @@ -1920,6 +1928,10 @@ static inline bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x
>   	return false;
>   }
>   
> +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
> +{
> +}
> +
>   static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
>   {
>   	return false;
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index dc68d9c..fc7e1e44 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -65,8 +65,8 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>   	if (!x->type_offload)
>   		return -EINVAL;
>   
> -	/* We don't yet support UDP encapsulation, TFC padding and ESN. */
> -	if (x->encap || x->tfcpad || (x->props.flags & XFRM_STATE_ESN))
> +	/* We don't yet support UDP encapsulation and TFC padding. */
> +	if (x->encap || x->tfcpad)
>   		return -EINVAL;

Maybe we should check to see that xdo_dev_state_advance_esn is 
implemented before allowing XFRM_STATE_ESN?

sln

>   
>   	dev = dev_get_by_index(net, xuo->ifindex);
> diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
> index 0250181..1d38c6a 100644
> --- a/net/xfrm/xfrm_replay.c
> +++ b/net/xfrm/xfrm_replay.c
> @@ -551,6 +551,8 @@ static void xfrm_replay_advance_esn(struct xfrm_state *x, __be32 net_seq)
>   			bitnr = replay_esn->replay_window - (diff - pos);
>   	}
>   
> +	xfrm_dev_state_advance_esn(x);
> +
>   	nr = bitnr >> 5;
>   	bitnr = bitnr & 0x1F;
>   	replay_esn->bmp[nr] |= (1U << bitnr);
>