netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: davem@davemloft.net, davejwatson@fb.com, netdev@vger.kernel.org,
	ast@kernel.org, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH net] tls: fix NULL pointer dereference on poll
Date: Tue, 12 Jun 2018 07:37:50 +0200	[thread overview]
Message-ID: <20180612053749.GA16853@lst.de> (raw)
In-Reply-To: <20180611212204.24002-1-daniel@iogearbox.net>

> 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?

  parent reply	other threads:[~2018-06-12  5:29 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-11 21:22 [PATCH net] tls: fix NULL pointer dereference on poll Daniel Borkmann
2018-06-11 22:27 ` Dave Watson
2018-06-11 23:30 ` David Miller
2018-06-12  5:37 ` Christoph Hellwig [this message]
2018-06-12 10:43   ` Daniel Borkmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180612053749.GA16853@lst.de \
    --to=hch@lst.de \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davejwatson@fb.com \
    --cc=davem@davemloft.net \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).