| From 3961402d18b278631a80fb3abc77bb17a78c7b6c Mon Sep 17 00:00:00 2001 |
| From: John Fastabend <john.fastabend@gmail.com> |
| Date: Sat, 11 Jan 2020 06:12:04 +0000 |
| Subject: [PATCH] bpf: Sockmap/tls, tls_sw can create a plaintext buf > encrypt |
| buf |
| |
| commit d468e4775c1c351616947ba0cccc43273963b9b5 upstream. |
| |
| It is possible to build a plaintext buffer using push helper that is larger |
| than the allocated encrypt buffer. When this record is pushed to crypto |
| layers this can result in a NULL pointer dereference because the crypto |
| API expects the encrypt buffer is large enough to fit the plaintext |
| buffer. Kernel splat below. |
| |
| To resolve catch the cases this can happen and split the buffer into two |
| records to send individually. Unfortunately, there is still one case to |
| handle where the split creates a zero sized buffer. In this case we merge |
| the buffers and unmark the split. This happens when apply is zero and user |
| pushed data beyond encrypt buffer. This fixes the original case as well |
| because the split allocated an encrypt buffer larger than the plaintext |
| buffer and the merge simply moves the pointers around so we now have |
| a reference to the new (larger) encrypt buffer. |
| |
| Perhaps its not ideal but it seems the best solution for a fixes branch |
| and avoids handling these two cases, (a) apply that needs split and (b) |
| non apply case. The are edge cases anyways so optimizing them seems not |
| necessary unless someone wants later in next branches. |
| |
| [ 306.719107] BUG: kernel NULL pointer dereference, address: 0000000000000008 |
| [...] |
| [ 306.747260] RIP: 0010:scatterwalk_copychunks+0x12f/0x1b0 |
| [...] |
| [ 306.770350] Call Trace: |
| [ 306.770956] scatterwalk_map_and_copy+0x6c/0x80 |
| [ 306.772026] gcm_enc_copy_hash+0x4b/0x50 |
| [ 306.772925] gcm_hash_crypt_remain_continue+0xef/0x110 |
| [ 306.774138] gcm_hash_crypt_continue+0xa1/0xb0 |
| [ 306.775103] ? gcm_hash_crypt_continue+0xa1/0xb0 |
| [ 306.776103] gcm_hash_assoc_remain_continue+0x94/0xa0 |
| [ 306.777170] gcm_hash_assoc_continue+0x9d/0xb0 |
| [ 306.778239] gcm_hash_init_continue+0x8f/0xa0 |
| [ 306.779121] gcm_hash+0x73/0x80 |
| [ 306.779762] gcm_encrypt_continue+0x6d/0x80 |
| [ 306.780582] crypto_gcm_encrypt+0xcb/0xe0 |
| [ 306.781474] crypto_aead_encrypt+0x1f/0x30 |
| [ 306.782353] tls_push_record+0x3b9/0xb20 [tls] |
| [ 306.783314] ? sk_psock_msg_verdict+0x199/0x300 |
| [ 306.784287] bpf_exec_tx_verdict+0x3f2/0x680 [tls] |
| [ 306.785357] tls_sw_sendmsg+0x4a3/0x6a0 [tls] |
| |
| test_sockmap test signature to trigger bug, |
| |
| [TEST]: (1, 1, 1, sendmsg, pass,redir,start 1,end 2,pop (1,2),ktls,): |
| |
| Fixes: d3b18ad31f93d ("tls: add bpf support to sk_msg handling") |
| Signed-off-by: John Fastabend <john.fastabend@gmail.com> |
| Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> |
| Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com> |
| Cc: stable@vger.kernel.org |
| Link: https://lore.kernel.org/bpf/20200111061206.8028-7-john.fastabend@gmail.com |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c |
| index 506cd1091967..f7c92df2e266 100644 |
| --- a/net/tls/tls_sw.c |
| +++ b/net/tls/tls_sw.c |
| @@ -677,12 +677,32 @@ static int tls_push_record(struct sock *sk, int flags, |
| |
| split_point = msg_pl->apply_bytes; |
| split = split_point && split_point < msg_pl->sg.size; |
| + if (unlikely((!split && |
| + msg_pl->sg.size + |
| + prot->overhead_size > msg_en->sg.size) || |
| + (split && |
| + split_point + |
| + prot->overhead_size > msg_en->sg.size))) { |
| + split = true; |
| + split_point = msg_en->sg.size; |
| + } |
| if (split) { |
| rc = tls_split_open_record(sk, rec, &tmp, msg_pl, msg_en, |
| split_point, prot->overhead_size, |
| &orig_end); |
| if (rc < 0) |
| return rc; |
| + /* This can happen if above tls_split_open_record allocates |
| + * a single large encryption buffer instead of two smaller |
| + * ones. In this case adjust pointers and continue without |
| + * split. |
| + */ |
| + if (!msg_pl->sg.size) { |
| + tls_merge_open_record(sk, rec, tmp, orig_end); |
| + msg_pl = &rec->msg_plaintext; |
| + msg_en = &rec->msg_encrypted; |
| + split = false; |
| + } |
| sk_msg_trim(sk, msg_en, msg_pl->sg.size + |
| prot->overhead_size); |
| } |
| -- |
| 2.7.4 |
| |