From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [PATCH net] tls: fix NULL pointer dereference on poll Date: Tue, 12 Jun 2018 07:37:50 +0200 Message-ID: <20180612053749.GA16853@lst.de> References: <20180611212204.24002-1-daniel@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: davem@davemloft.net, davejwatson@fb.com, netdev@vger.kernel.org, ast@kernel.org, Christoph Hellwig To: Daniel Borkmann Return-path: Received: from verein.lst.de ([213.95.11.211]:34014 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932286AbeFLF3r (ORCPT ); Tue, 12 Jun 2018 01:29:47 -0400 Content-Disposition: inline In-Reply-To: <20180611212204.24002-1-daniel@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: > Looks like the recent conversion from poll to poll_mask callback started > in 152524231023 ("net: add support for ->poll_mask in proto_ops") missed > to eventually convert kTLS, too: TCP's ->poll was converted over to the > ->poll_mask in commit 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask") > and therefore kTLS wrongly saved the ->poll old one which is now NULL. Looks like this TLS code was added in the same cycle. > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c > index 301f224..a127d61 100644 > --- a/net/tls/tls_main.c > +++ b/net/tls/tls_main.c > @@ -712,7 +712,7 @@ static int __init tls_register(void) > build_protos(tls_prots[TLSV4], &tcp_prot); > > tls_sw_proto_ops = inet_stream_ops; > - tls_sw_proto_ops.poll = tls_sw_poll; > + tls_sw_proto_ops.poll_mask = tls_sw_poll_mask; > tls_sw_proto_ops.splice_read = tls_sw_splice_read; Not new in this patch, but copying ops vectors is a very bad idea, not only because your new instance can't be marked const and you thus open up exploit vectors. I would suggest to clean this up eventually. > +__poll_t tls_sw_poll_mask(struct socket *sock, __poll_t events) > { > struct sock *sk = sock->sk; > struct tls_context *tls_ctx = tls_get_ctx(sk); > struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx); > + __poll_t mask; > > + /* Grab EPOLLOUT and EPOLLHUP from the underlying socket */ > + mask = ctx->sk_poll_mask(sock, events); > > + /* Clear EPOLLIN bits, and set based on recv_pkt */ > + mask &= ~(EPOLLIN | EPOLLRDNORM); > if (ctx->recv_pkt) > + mask |= EPOLLIN | EPOLLRDNORM; > > + return mask; So you call the underlying protocol method on the struct sock of the TLS code? Again not reall new in this patch, but how is this even supposed to work?