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 <[email protected]>
---
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
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
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?
--
Sabrina
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?
--
Sabrina
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.