[PATCH] net: tls: Fix possible NULL-pointer dereference in tls_decrypt_device() and tls_decrypt_sw()

[PATCH] net: tls: Fix possible NULL-pointer dereference in tls_decrypt_device() and tls_decrypt_sw()

[Hangyu Hua]

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.

Fixes: dd47ed3620e6 ("tls: rx: factor SW handling out of tls_rx_one_record()")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
 net/tls/tls_sw.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index e9d1e83a859d..411bf148f707 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1612,7 +1612,11 @@ tls_decrypt_sw(struct sock *sk, struct tls_context *tls_ctx,
 	struct strp_msg *rxm;
 	int pad, err;
 
-	err = tls_decrypt_sg(sk, &msg->msg_iter, NULL, darg);
+	if (msg == NULL)
+		err = tls_decrypt_sg(sk, NULL, NULL, darg);
+	else
+		err = tls_decrypt_sg(sk, &msg->msg_iter, NULL, darg);
+
 	if (err < 0) {
 		if (err == -EBADMSG)
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSDECRYPTERROR);
@@ -1686,7 +1690,8 @@ tls_decrypt_device(struct sock *sk, struct msghdr *msg,
 		off = rxm->offset + prot->prepend_size;
 		len = rxm->full_len - prot->overhead_size;
 
-		err = skb_copy_datagram_msg(darg->skb, off, msg, len);
+		if (msg != NULL)
+			err = skb_copy_datagram_msg(darg->skb, off, msg, len);
 		if (err)
 			return err;
 	}
-- 
2.34.1

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

[Sabrina Dubroca]

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

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

[Hangyu Hua]

On 23/10/2023 22:03, Sabrina Dubroca wrote:
> 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.

My bad. I only checked &msg->msg_iter's address in tls_decrypt_sw and 
found it was wrong. Do I need to make a new patch to fix the harmless 
bogus pointer?

> 
> 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));
> 	}
> [...]
> }
> 

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

[Sabrina Dubroca]

2023-10-24, 10:17:08 +0800, Hangyu Hua wrote:
> On 23/10/2023 22:03, Sabrina Dubroca wrote:
> > 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.
> 
> My bad. I only checked &msg->msg_iter's address in tls_decrypt_sw and found
> it was wrong. Do I need to make a new patch to fix the harmless bogus
> pointer?

I don't think that's necessary, but maybe it would avoid people trying
to "fix" this code in the future. Jakub, WDYT?

-- 
Sabrina

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

[Jakub Kicinski]

On Wed, 25 Oct 2023 12:27:05 +0200 Sabrina Dubroca wrote:
> > My bad. I only checked &msg->msg_iter's address in tls_decrypt_sw and found
> > it was wrong. Do I need to make a new patch to fix the harmless bogus
> > pointer?  
> 
> I don't think that's necessary, but maybe it would avoid people trying
> to "fix" this code in the future. Jakub, WDYT?

No strong feelings, but personally I find checks for conditions which
cannot happen decrease the readability. Maybe a comment is better?

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

[Sabrina Dubroca]

2023-10-25, 07:14:08 -0700, Jakub Kicinski wrote:
> On Wed, 25 Oct 2023 12:27:05 +0200 Sabrina Dubroca wrote:
> > > My bad. I only checked &msg->msg_iter's address in tls_decrypt_sw and found
> > > it was wrong. Do I need to make a new patch to fix the harmless bogus
> > > pointer?  
> > 
> > I don't think that's necessary, but maybe it would avoid people trying
> > to "fix" this code in the future. Jakub, WDYT?
> 
> No strong feelings, but personally I find checks for conditions which
> cannot happen decrease the readability. Maybe a comment is better?

There's already a comment above tls_decrypt_sg that (pretty much) says
out_iov is only used in zero-copy mode.

 *          [...]            The input parameter 'darg->zc' indicates if
 * zero-copy mode needs to be tried or not. With zero-copy mode, either
 * out_iov or out_sg must be non-NULL.

Do we need another just above the call to tls_decrypt_sg?

-- 
Sabrina

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

[Jakub Kicinski]

On Wed, 25 Oct 2023 23:20:23 +0200 Sabrina Dubroca wrote:
> There's already a comment above tls_decrypt_sg that (pretty much) says
> out_iov is only used in zero-copy mode.
> 
>  *          [...]            The input parameter 'darg->zc' indicates if
>  * zero-copy mode needs to be tried or not. With zero-copy mode, either
>  * out_iov or out_sg must be non-NULL.
> 
> Do we need another just above the call to tls_decrypt_sg?

Sounds good. Right next to a line of code that people will try to
modify when whatever static checker they have tells them this is
buggy :S  Call site of tls_decrypt_sg() seems like a good bet.