netdev - Re: [PATCH ipsec-next 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]

Message-ID: <20220518074914.GP680067@gauss3.secunet.de>
Date:   Wed, 18 May 2022 09:49:14 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Leon Romanovsky <leon@...nel.org>
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 ipsec-next 4/6] xfrm: add TX datapath support for IPsec
 full offload mode

On Mon, May 16, 2022 at 08:44:58AM +0300, Leon Romanovsky wrote:
> On Fri, May 13, 2022 at 04:56:58PM +0200, Steffen Klassert wrote:
> > On Tue, May 10, 2022 at 01:36:55PM +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_output.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > > index d4935b3b9983..2599f3dbac08 100644
> > > --- a/net/xfrm/xfrm_output.c
> > > +++ b/net/xfrm/xfrm_output.c
> > > @@ -718,6 +718,25 @@ 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_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > > +			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;
> > > +	}
> > > +
> > 
> > How do we know that we send the packet really to a device that
> > supports this type of offload? For crypto offload, we check that
> > in xfrm_dev_offload_ok() and I think something similar is required
> > here too.
> 
> I think that function is needed to make sure that we will have SW
> fallback. It is not needed in full offload, anything that is not
> supported/wrong should be dropped by HW.

Yes, but only if the final output device really supports this kind
of offload. How can we be sure that this is the case? Packets can be
rerouted etc. We need to make sure that the outgoing device supports
full offload, and I think this check is missing somewhere.