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

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

Message-ID: <33d0faa0-6f05-16fe-61a7-f106005f16e0@iogearbox.net>
Date:   Tue, 12 Jun 2018 12:43:20 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Christoph Hellwig <hch@....de>
Cc:     davem@...emloft.net, davejwatson@...com, netdev@...r.kernel.org,
        ast@...nel.org
Subject: Re: [PATCH net] tls: fix NULL pointer dereference on poll

On 06/12/2018 07:37 AM, Christoph Hellwig wrote:
>> 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.

Generally, agree with you. It could at minimum also be a __ro_after_init
candidate, at least the TLSV4 ops which wouldn't change. In v6 case though
it could be loaded as a module after TLS was initialized.

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

Yeah, patch doesn't change it, but reason is that TLS relies on kernel's
stream parser to determine TLS message boundary on ingress, so once a full
message got received only then we want to signal this to the user space
application. Latter skb is then held in ctx->recv_pkt via stream parser.

Thanks,
Daniel