linux-kernel - Re: [PATCH] net: tls: Fix possible NULL-pointer dereference in tls_decrypt_device() and tls_decrypt

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: <ZTZ9H4aDB45RzrFD@hog>
Date:   Mon, 23 Oct 2023 16:03:11 +0200
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Hangyu Hua <hbh25y@...il.com>
Cc:     borisp@...dia.com, john.fastabend@...il.com, kuba@...nel.org,
        davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: tls: Fix possible NULL-pointer dereference in
 tls_decrypt_device() and tls_decrypt_sw()

2023-10-23, 16:06:11 +0800, Hangyu Hua wrote:
> tls_rx_one_record can be called in tls_sw_splice_read and tls_sw_read_sock
> with msg being NULL. This may lead to null pointer dereferences in
> tls_decrypt_device and tls_decrypt_sw.
> 
> Fix this by adding a check.

Have you actually hit this NULL dereference? I don't see how it can
happen.

darg->zc is 0 in both cases, so tls_decrypt_device doesn't call
skb_copy_datagram_msg.

tls_decrypt_sw will call tls_decrypt_sg with out_iov = &msg->msg_iter
(a bogus pointer but no NULL deref yet), and darg->zc is still
0. tls_decrypt_sg skips the use of out_iov/out_sg and allocates
clear_skb, and the next place where it would use out_iov is skipped
because we have clear_skb.

Relevant parts of tls_decrypt_sg:

static int tls_decrypt_sg(struct sock *sk, struct iov_iter *out_iov,
			  struct scatterlist *out_sg,
			  struct tls_decrypt_arg *darg)
{
[...]
	if (darg->zc && (out_iov || out_sg)) {
		clear_skb = NULL;
[...]
	} else {
		darg->zc = false;

		clear_skb = tls_alloc_clrtxt_skb(sk, skb, rxm->full_len);
[...]
	}

[...]
	if (err < 0)
		goto exit_free;

	if (clear_skb) {
		sg_init_table(sgout, n_sgout);
		sg_set_buf(&sgout[0], dctx->aad, prot->aad_size);

		err = skb_to_sgvec(clear_skb, &sgout[1], prot->prepend_size,
				   data_len + prot->tail_size);
		if (err < 0)
			goto exit_free;
	} else if (out_iov) {
[...]
	} else if (out_sg) {
		memcpy(sgout, out_sg, n_sgout * sizeof(*sgout));
	}
[...]
}

-- 
Sabrina