linux-kernel - Re: KASAN: use-after-free Read in skcipher

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

Date:   Sun, 4 Nov 2018 12:18:33 +0100
From:   LABBE Corentin <clabbe@...libre.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     davem@...emloft.net, herbert@...dor.apana.org.au,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com,
        syzbot <syzbot+6939a606a5305e9e9799@...kaller.appspotmail.com>
Subject: Re: KASAN: use-after-free Read in skcipher_recvmsg

On Sat, Nov 03, 2018 at 03:35:04PM -0700, Eric Biggers wrote:
> [+clabbe@...libre.com]
> 
> Hi Corentin, I think this is a bug in the new crypto statistics feature.  In the
> skcipher_decrypt case the code is (but this applies elsewhere too!):
> 
> static inline void crypto_stat_skcipher_decrypt(struct skcipher_request *req,
> 						int ret, struct crypto_alg *alg)
> {
> #ifdef CONFIG_CRYPTO_STATS
> 	if (ret && ret != -EINPROGRESS && ret != -EBUSY) {
> 		atomic_inc(&alg->cipher_err_cnt);
> 	} else {
> 		atomic_inc(&alg->decrypt_cnt);
> 		atomic64_add(req->cryptlen, &alg->decrypt_tlen);
> 	}
> #endif
> }
> 
> static inline int crypto_skcipher_decrypt(struct skcipher_request *req)
> {
> 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> 	int ret;
> 
> 	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
> 		ret = -ENOKEY;
> 	else
> 		ret = tfm->decrypt(req);
> 	crypto_stat_skcipher_decrypt(req, ret, tfm->base.__crt_alg);
> 	return ret;
> }
> 
> The bug is the request may be issued asynchronously (as indicated by EINPROGRESS
> or EBUSY) being returned, and the stats are updated afterwards.  But by that
> time, the request's completion function may have already run, and the request
> structure may have already been freed.
> 
> In theory, I think the algorithm could have even been unregistered as well.
> Therefore, it's only safe to update the stats either *before* calling
> tfm->decrypt(), or afterwards if the error code was not EINPROGRESS or EBUSY.

Hello

I can store "len" and alg for later use, this will fix a part of the problem.

For the fact that algorithm could be unregistred, I think it cannot happen since at least the tfm running this crypto_skcipher_decrypt/othersamefunction still exists and that it is(should be) impossible to unregister an alg with still existing tfm which uses it.
But that needs to be verified.

Regards