linux-kernel - [PATCH v2] crypto: AF_ALG - race-free access of encryption flag

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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Nov 2017 22:33:09 +0100
From:   Stephan Müller <smueller@...onox.de>
To:     herbert@...dor.apana.org.au
Cc:     Eric Biggers <ebiggers3@...il.com>,
        syzbot 
        <bot+3401d9494b9380f7244bcc7fec49680878fccba6@...kaller.appspotmail.com>,
        davem@...emloft.net, linux-crypto@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: [PATCH v2] crypto: AF_ALG - race-free access of encryption flag

Hi Herbert,

I verified the correctnes of the patch with Eric's test program.
Without the patch, the issue is present. With the patch, the kernel
happily lives ever after.

Changes v2: change the submission into a proper patch

Ciao
Stephan

---8<---

The function af_alg_get_rsgl may sleep to wait for additional data. In
this case, the socket lock may be dropped. This allows user space to
change values in the socket data structure. Hence, all variables of the
context that are needed before and after the af_alg_get_rsgl must be
copied into a local variable.

This issue applies to the ctx->enc variable only. Therefore, this
value is copied into a local variable that is used for all operations
before and after the potential sleep and lock release. This implies that
any changes on this variable while the kernel waits for data will only
be picked up in another recvmsg operation.

Reported-by: syzbot <syzkaller@...glegroups.com>
Cc: <stable@...r.kernel.org> # v4.14+
Signed-off-by: Stephan Mueller <smueller@...onox.de>
---
 crypto/algif_aead.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 7d2d162666e5..4ba13c0b97ca 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -110,6 +110,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
 	size_t outlen = 0;		/* [out] RX bufs produced by kernel */
 	size_t usedpages = 0;		/* [in]  RX bufs to be used from user */
 	size_t processed = 0;		/* [in]  TX bufs to be consumed */
+	bool enc = ctx->enc;		/* prevent race if sock lock dropped */
 
 	/*
 	 * Data length provided by caller via sendmsg/sendpage that has not
@@ -137,7 +138,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
 	 * buffer provides the tag which is consumed resulting in only the
 	 * plaintext without a buffer for the tag returned to the caller.
 	 */
-	if (ctx->enc)
+	if (enc)
 		outlen = used + as;
 	else
 		outlen = used - as;
@@ -211,7 +212,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
 	/* Use the RX SGL as source (and destination) for crypto op. */
 	rsgl_src = areq->first_rsgl.sgl.sg;
 
-	if (ctx->enc) {
+	if (enc) {
 		/*
 		 * Encryption operation - The in-place cipher operation is
 		 * achieved by the following operation:
@@ -288,7 +289,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
 		aead_request_set_callback(&areq->cra_u.aead_req,
 					  CRYPTO_TFM_REQ_MAY_BACKLOG,
 					  af_alg_async_cb, areq);
-		err = ctx->enc ? crypto_aead_encrypt(&areq->cra_u.aead_req) :
+		err = enc ? crypto_aead_encrypt(&areq->cra_u.aead_req) :
 				 crypto_aead_decrypt(&areq->cra_u.aead_req);
 
 		/* AIO operation in progress */
@@ -305,7 +306,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
 		aead_request_set_callback(&areq->cra_u.aead_req,
 					  CRYPTO_TFM_REQ_MAY_BACKLOG,
 					  crypto_req_done, &ctx->wait);
-		err = crypto_wait_req(ctx->enc ?
+		err = crypto_wait_req(enc ?
 				crypto_aead_encrypt(&areq->cra_u.aead_req) :
 				crypto_aead_decrypt(&areq->cra_u.aead_req),
 				&ctx->wait);
-- 
2.14.3