netdev - Re: [PATCH xfrm-next v2 4/6] xfrm: add TX datapath support for IPsec full offload mode

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]

Date:   Thu, 18 Aug 2022 12:24:51 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Leon Romanovsky <leon@...nel.org>
CC:     Leon Romanovsky <leonro@...dia.com>,
        "David S . Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        <netdev@...r.kernel.org>, Raed Salem <raeds@...dia.com>,
        ipsec-devel <devel@...ux-ipsec.org>
Subject: Re: [PATCH xfrm-next v2 4/6] xfrm: add TX datapath support for IPsec
 full offload mode

On Tue, Aug 16, 2022 at 11:59:25AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@...dia.com>
> 
> In IPsec full mode, the device is going to encrypt and encapsulate
> packets that are associated with offloaded policy. After successful
> policy lookup to indicate if packets should be offloaded or not,
> the stack forwards packets to the device to do the magic.
> 
> Signed-off-by: Raed Salem <raeds@...dia.com>
> Signed-off-by: Huy Nguyen <huyn@...dia.com>
> Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> ---
>  net/xfrm/xfrm_device.c | 13 +++++++++++++
>  net/xfrm/xfrm_output.c | 20 ++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 1cc482e9c87d..db5ebd36f68c 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c

> @@ -366,6 +376,9 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
>  	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
>  	struct net_device *dev = x->xso.dev;
>  
> +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL)
> +		goto ok;

You skip the PMTU checks here. I've seen that you check
the packet length against the device MTU in one of your
mlx5 patches, but that does not help if the PMTU is below.

>  
> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index 555ab35cd119..27a8dac9ca7d 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -719,6 +719,26 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
>  		break;
>  	}
>  
> +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL) {
> +		struct dst_entry *dst = skb_dst_pop(skb);
> +
> +		if (!dst || !xfrm_dev_offload_ok(skb, x)) {
> +			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> +			kfree_skb(skb);
> +			return -EHOSTUNREACH;
> +		}
> +
> +		skb_dst_set(skb, dst);
> +		err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
> +		if (unlikely(err != 1))
> +			return err;
> +
> +		if (!skb_dst(skb)->xfrm)
> +			return dst_output(net, skb->sk, skb);
> +
> +		return 0;

You leak skb here. Also, this skb needs another tfm because
skb_dst(skb)->xfrm is set.