Commit d468e477 authored Jan 11, 2020 by John Fastabend Committed by Daniel Borkmann Jan 15, 2020

bpf: Sockmap/tls, tls_sw can create a plaintext buf > encrypt buf



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: d3b18ad3 ("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

parent cf21e9ba

net/tls/tls_sw.c

+20 −0

Original line number	Diff line number	Diff line
		@@ -682,12 +682,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);
		}

Admin message