diff mbox series

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

Message ID 20231023080611.19244-1-hbh25y@gmail.com (mailing list archive)
State Rejected
Delegated to: Netdev Maintainers
Headers show
Series net: tls: Fix possible NULL-pointer dereference in tls_decrypt_device() and tls_decrypt_sw() | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1363 this patch: 1363
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1388 this patch: 1388
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1388 this patch: 1388
netdev/checkpatch warning CHECK: Comparison to NULL could be written "!msg" CHECK: Comparison to NULL could be written "msg"
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Hangyu Hua Oct. 23, 2023, 8:06 a.m. UTC
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(-)

Comments

Sabrina Dubroca Oct. 23, 2023, 2:03 p.m. UTC | #1
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));
	}
[...]
}
Hangyu Hua Oct. 24, 2023, 2:17 a.m. UTC | #2
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));
> 	}
> [...]
> }
>
Sabrina Dubroca Oct. 25, 2023, 10:27 a.m. UTC | #3
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?
Jakub Kicinski Oct. 25, 2023, 2:14 p.m. UTC | #4
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?
Sabrina Dubroca Oct. 25, 2023, 9:20 p.m. UTC | #5
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?
Jakub Kicinski Oct. 25, 2023, 9:30 p.m. UTC | #6
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.
diff mbox series

Patch

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;
 	}