netdev - Re: [PATCH net-next 1/3] net/tls: use RCU protection on icsk->icsk_ulp

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:   Mon, 19 Aug 2019 15:23:31 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Boris Pismenny <borisp@...lanox.com>,
        John Fastabend <john.fastabend@...il.com>,
        Dave Watson <davejwatson@...com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net/tls: use RCU protection on
 icsk->icsk_ulp_data

On Thu, 2019-08-15 at 14:32 -0700, Jakub Kicinski wrote:
> On Thu, 15 Aug 2019 18:00:42 +0200, Davide Caratti wrote:
> > From: Jakub Kicinski <jakub.kicinski@...ronome.com>
> > 
> > We need to make sure context does not get freed while diag
> > code is interrogating it. Free struct tls_context with
> > kfree_rcu().
> > 
> > We add the __rcu annotation directly in icsk, and cast it
> > away in the datapath accessor. Presumably all ULPs will
> > do a similar thing.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>

hello Jakub,

> > @@ -251,14 +251,31 @@ static void tls_write_space(struct sock *sk)
> >  	ctx->sk_write_space(sk);
> >  }
> >  
> > -void tls_ctx_free(struct tls_context *ctx)
> > +/**
> > + * tls_ctx_free() - free TLS ULP context
> > + * @sk:  socket to with @ctx is attached
> > + * @ctx: TLS context structure
> > + *
> > + * Free TLS context. If @sk is %NULL caller guarantees that the socket
> > + * to which @ctx was attached has no outstanding references.
> > + */
> > +void tls_ctx_free(struct sock *sk, struct tls_context *ctx)
> >  {
> > +	struct inet_connection_sock *icsk;
> > +
> >  	if (!ctx)
> >  		return;
> >  
> >  	memzero_explicit(&ctx->crypto_send, sizeof(ctx->crypto_send));
> >  	memzero_explicit(&ctx->crypto_recv, sizeof(ctx->crypto_recv));
> > -	kfree(ctx);
> > +
> > +	if (sk) {
> > +		icsk = inet_csk(sk);
> > +		rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
> 
> Now that we kind of want to set the icsk_ulp_data to NULL under the
> callback_lock I think we should let the callers do it.

Ok, I will fix this in series v2.

> > 
> > @@ -649,8 +666,8 @@ static void tls_hw_sk_destruct(struct sock *sk)
> >  
> >  	ctx->sk_destruct(sk);
> >  	/* Free ctx */
> > -	tls_ctx_free(ctx);
> > -	icsk->icsk_ulp_data = NULL;
> > +	tls_ctx_free(sk, ctx);
> > +	rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
> 
> Let's reorder the assignment before the free.

Ok, I will fix this in series v2.

thanks!
-- 
davide