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