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

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

Date:   Wed, 25 Oct 2017 09:22:14 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     <avivh@...lanox.com>
CC:     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>
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: