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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 18 Aug 2022 16:34:54 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     "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 Thu, Aug 18, 2022 at 12:24:51PM +0200, Steffen Klassert wrote:
> 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.

If device supports transformation of the packet, this packet
won't be counted as XFRM anymore. I'm not sure that we need
to perform XFRM specific checks.

> 
> >  
> > 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.

I will fix, thanks.

>