netdev - RE: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion

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>] [day] [month] [year] [list]

Message-ID: <0101015f595dc570-9cae58d5-fe75-4457-9bfb-2420b9ed7c1d-000000@us-west-2.amazonses.com>
Date:   Thu, 26 Oct 2017 15:47:18 +0000
From:   Aviv Heller <aviv@...vh.com>
To:     Aviv Heller <aviv@...vh.com>,
        Steffen Klassert <steffen.klassert@...unet.com>
Cc:     netdev-owner@...r.kernel.org 
        <netdev-owner@...r.kernel.org>,
        avivh@...lanox.com <avivh@...lanox.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Boris Pismenny <borisp@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Yevgeny Kliteynik <kliteyn@...lanox.com>,
        netdev@...r.kernel.org <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to
 occur after insertion

-----Original message-----
> From: Aviv Heller
> Sent: Thursday, October 26 2017, 5:55 pm
> To: Steffen Klassert
> Cc: netdev-owner@...r.kernel.org; avivh@...lanox.com; Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik; netdev@...r.kernel.org
> Subject: RE: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion
> 
> -----Original message-----
> > From: Steffen Klassert
> > Sent: Thursday, October 26 2017, 9:16 am
> > To: Aviv Heller
> > Cc: netdev-owner@...r.kernel.org; avivh@...lanox.com; Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik; netdev@...r.kernel.org
> > Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion
> > 
> > On Wed, Oct 25, 2017 at 01:09:44PM +0000, Aviv Heller wrote:
> > > -----Original message-----
> > > > From: Steffen Klassert
> > > > Sent: Wednesday, October 25 2017, 10:22 am
> > > > To: avivh@...lanox.com
> > > > Cc: Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik; netdev@...r.kernel.org
> > > > Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion
> > > > 
> > > > On Tue, Oct 24, 2017 at 06:10:30PM +0300, avivh@...lanox.com wrote:
> > > > > From: Aviv Heller <avivh@...lanox.com>
> > > > > 
> > > > > Adding the state to the offload device prior to replay init in
> > > > > xfrm_state_construct() will result in NULL dereference if a matching
> > > > > ESP packet is received in between.
> > > > > 
> > > > > Adding it after insertion also has the benefit of the driver not having
> > > > > to check whether a state with the same match criteria already exists,
> > > > > but forces us to use an atomic type for the offload_handle, to make
> > > > > certain a stack-read/driver-write race won't result in reading corrupt
> > > > > data.
> > > > 
> > > > No, this will add multiple atomic operations to the packet path,
> > > > even in the non offloaded case.
> > > > 
> > > > I think the problem is that we set XFRM_STATE_VALID to early.
> > > > This was not a problem before we had offloading because
> > > > it was not possible to lookup this state before we inserted
> > > > it into the SADB. Now that the driver holds a subset of states
> > > > too, we need to make sure the state is fully initialized
> > > > before we mark it as valid.
> > > > 
> > > > The patch below should do it, in combination with your patch 1/3.
> > > > 
> > > > Could you please test this?
> > > > 
> > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > > > index b997f13..96eb263 100644
> > > > --- a/net/xfrm/xfrm_user.c
> > > > +++ b/net/xfrm/xfrm_user.c
> > > > @@ -587,10 +587,6 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
> > > >  	if (attrs[XFRMA_OUTPUT_MARK])
> > > >  		x->props.output_mark = nla_get_u32(attrs[XFRMA_OUTPUT_MARK]);
> > > >  
> > > > -	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
> > > > -	if (err)
> > > > -		goto error;
> > > > -
> > > >  	if (attrs[XFRMA_SEC_CTX]) {
> > > >  		err = security_xfrm_state_alloc(x,
> > > >  						nla_data(attrs[XFRMA_SEC_CTX]));
> > > > @@ -620,6 +616,10 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
> > > >  	/* override default values from above */
> > > >  	xfrm_update_ae_params(x, attrs, 0);
> > > >  
> > > > +	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
> > > > +	if (err)
> > > > +		goto error;
> > > > +
> > > >  	return x;
> > > >  
> > > >  error:
> > > > 
> > > 
> > > Hi Steffen,
> > > 
> > > This patch does not work, due to:
> > > 	if (!x->type_offload)
> > > 		return -EINVAL;
> > > 
> > > test in xfrm_dev_state_add().
> > 
> > There is certainly a way arround that :)
> > The easiest I can think of would be to propagate XFRM_STATE_VALID
> > only after the state is inserted into the SADBs. I.e. move the
> > setting of XFRM_STATE_VALID out of __xfrm_init_state() and let the
> > callers do it.
> 
> This does seem like the easiest solution, if we don't move state addition to occur after insertion.
> I'm waiting for our regression results (probably on Monday) for the patch below, and would appreciate your comments:
> 

Please ignore the last patch, I understood you wrong.
Will reimplement and submit.