netdev - [PATCH net-next] tls: Fix socket mem accounting error under async encryption

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20180925105617.22543-1-vakul.garg@nxp.com>
Date:   Tue, 25 Sep 2018 16:26:17 +0530
From:   Vakul Garg <vakul.garg@....com>
To:     netdev@...r.kernel.org
Cc:     borisp@...lanox.com, aviadye@...lanox.com, davejwatson@...com,
        davem@...emloft.net, doronrk@...com,
        Vakul Garg <vakul.garg@....com>
Subject: [PATCH net-next] tls: Fix socket mem accounting error under async encryption

Current async encryption implementation sometimes showed up socket
memory accounting error during socket close. This results in kernel
warning calltrace. The root cause of the problem is that socket var
sk_forward_alloc gets corrupted due to access in sk_mem_charge()
and sk_mem_uncharge() being invoked from multiple concurrent contexts
in multicore processor. The apis sk_mem_charge() and sk_mem_uncharge()
are called from functions alloc_plaintext_sg(), free_sg() etc. It is
required that memory accounting apis are called under a socket lock.

The plaintext sg data sent for encryption is freed using free_sg() in
tls_encryption_done(). It is wrong to call free_sg() from this function.
This is because this function may run in irq context. We cannot acquire
socket lock in this function.

We remove calling of function free_sg() for plaintext data from
tls_encryption_done() and defer freeing up of plaintext data to the time
when the record is picked up from tx_list and transmitted/freed. When
tls_tx_records() gets called, socket is already locked and thus there is
no concurrent access problem.

Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
Signed-off-by: Vakul Garg <vakul.garg@....com>
---
 net/tls/tls_sw.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index bf03f32aa983..406d3bb98818 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -353,6 +353,9 @@ int tls_tx_records(struct sock *sk, int flags)
 		 * Remove the head of tx_list
 		 */
 		list_del(&rec->list);
+		free_sg(sk, rec->sg_plaintext_data,
+			&rec->sg_plaintext_num_elem, &rec->sg_plaintext_size);
+
 		kfree(rec);
 	}

@@ -371,6 +374,10 @@ int tls_tx_records(struct sock *sk, int flags)
 				goto tx_err;

 			list_del(&rec->list);
+			free_sg(sk, rec->sg_plaintext_data,
+				&rec->sg_plaintext_num_elem,
+				&rec->sg_plaintext_size);
+
 			kfree(rec);
 		} else {
 			break;
@@ -399,8 +406,6 @@ static void tls_encrypt_done(struct crypto_async_request *req, int err)
 	rec->sg_encrypted_data[0].offset -= tls_ctx->tx.prepend_size;
 	rec->sg_encrypted_data[0].length += tls_ctx->tx.prepend_size;

-	free_sg(sk, rec->sg_plaintext_data,
-		&rec->sg_plaintext_num_elem, &rec->sg_plaintext_size);

 	/* Free the record if error is previously set on socket */
 	if (err || sk->sk_err) {
@@ -523,9 +528,6 @@ static int tls_push_record(struct sock *sk, int flags,
 	if (rc == -EINPROGRESS)
 		return -EINPROGRESS;

-	free_sg(sk, rec->sg_plaintext_data, &rec->sg_plaintext_num_elem,
-		&rec->sg_plaintext_size);
-
 	if (rc < 0) {
 		tls_err_abort(sk, EBADMSG);
 		return rc;
@@ -1566,6 +1568,11 @@ void tls_sw_free_resources_tx(struct sock *sk)

 		rec = list_first_entry(&ctx->tx_list,
 				       struct tls_rec, list);
+
+		free_sg(sk, rec->sg_plaintext_data,
+			&rec->sg_plaintext_num_elem,
+			&rec->sg_plaintext_size);
+
 		list_del(&rec->list);
 		kfree(rec);
 	}
@@ -1575,6 +1582,10 @@ void tls_sw_free_resources_tx(struct sock *sk)
 			&rec->sg_encrypted_num_elem,
 			&rec->sg_encrypted_size);

+		free_sg(sk, rec->sg_plaintext_data,
+			&rec->sg_plaintext_num_elem,
+			&rec->sg_plaintext_size);
+
 		list_del(&rec->list);
 		kfree(rec);
 	}
-- 
2.13.6