netdev - Re: [PATCH net] tls: fix NULL pointer dereference on poll

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]

Message-ID: <20180612053749.GA16853@lst.de>
Date:   Tue, 12 Jun 2018 07:37:50 +0200
From:   Christoph Hellwig <hch@....de>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     davem@...emloft.net, davejwatson@...com, netdev@...r.kernel.org,
        ast@...nel.org, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH net] tls: fix NULL pointer dereference on poll

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