crypto: rng - fix crypto_rng_reset() refcounting when !CRYPTO_STATS
Subsystems (2)
PATCH 1/1 crypto: rng - fix crypto_rng_reset() refcounting when !CRYPTO_STATS
From: Eric Biggers <[email protected]>
crypto_stats_get() is a no-op when the kernel is compiled without CONFIG_CRYPTO_STATS, so pairing it with crypto_alg_put() unconditionally (as crypto_rng_reset() does) is wrong.
Fix this by moving the call to crypto_stats_get() to just before the actual algorithm operation which might need it. This makes it always paired with crypto_stats_rng_seed().
--- crypto/rng.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
crypto/rng.c +3 -7
@@ -34,22 +34,18 @@ int crypto_rng_reset(struct crypto_rng *tfm, const u8 *seed, unsigned int slen)u8 *buf =NULL ;int err; -crypto_stats_get (alg);if (!seed && slen) { buf =kmalloc (slen,GFP_KERNEL ); -if (!buf) { -crypto_alg_put (alg); +if (!buf)return -ENOMEM ; - } err =get_random_bytes_wait (buf, slen); -if (err) { -crypto_alg_put (alg); +if (err) goto out; - } seed = buf; } +crypto_stats_get (alg); err =crypto_rng_alg (tfm)->seed (tfm, seed, slen);crypto_stats_rng_seed (alg, err); out:
Signature · 1 line
2.31.0
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
crypto_stats_get()is a no-op when the kernel is compiled withoutCONFIG_CRYPTO_STATS, so pairing it withcrypto_alg_put()unconditionally (ascrypto_rng_reset()does) is wrong.
Presumably the intention was that _get() and _put() should always pair. It's really ugly and horrible that they don't. We could have predicted bug like this would happen and will continue to happen until the crypto_stats_get() is renamed.
regards,
dan carpenter
Quoted 8 lines · Dan Carpenter
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
crypto_stats_get()is a no-op when the kernel is compiled withoutCONFIG_CRYPTO_STATS, so pairing it withcrypto_alg_put()unconditionally (ascrypto_rng_reset()does) is wrong.
Quoted 5 lines
Presumably the intention was that
_get()and_put()should always pair. It's really ugly and horrible that they don't. We could have predicted bug like this would happen and will continue to happen until thecrypto_stats_get()is renamed.
Well, the crypto stats stuff has always been pretty broken, so I don't think people have looked at it too closely. Currently crypto_stats_get() pairs with one of the functions that tallies the statistics, such as crypto_stats_rng_seed() or crypto_stats_aead_encrypt(). What change are you suggesting, exactly? Maybe moving the conditional crypto_alg_put() into a new function crypto_stats_put() and moving it into the callers? Or do you think the functions should just be renamed to something like crypto_stats_begin() and crypto_stats_end_{rng_seed,aead_encrypt}()?
Another issue is that a lot of operations (such as the rng one in question here) don't actually need the get/put at all because they are never asynchronous. I didn't aim to address that in my patch though...
- Eric
Quoted 15 lines · Eric Biggers
Quoted 8 lines · Dan Carpenter
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
crypto_stats_get()is a no-op when the kernel is compiled withoutCONFIG_CRYPTO_STATS, so pairing it withcrypto_alg_put()unconditionally (ascrypto_rng_reset()does) is wrong.Quoted 5 lines
Presumably the intention was that
_get()and_put()should always pair. It's really ugly and horrible that they don't. We could have predicted bug like this would happen and will continue to happen until thecrypto_stats_get()is renamed.
Quoted 8 lines
Well, the crypto stats stuff has always been pretty broken, so I don't think people have looked at it too closely. Currently
crypto_stats_get()pairs with one of the functions that tallies the statistics, such ascrypto_stats_rng_seed()orcrypto_stats_aead_encrypt(). What change are you suggesting, exactly? Maybe moving the conditionalcrypto_alg_put()into a new functioncrypto_stats_put()and moving it into the callers? Or do you think the functions should just be renamed to something likecrypto_stats_begin()and crypto_stats_end_{rng_seed,aead_encrypt}()?
To be honest, I misread the crypto_alg_put() thinking that it was crypto_*stats*_put(). My favourite fix would be to introduce a crypto_stats_put() which is a mirror of crypto_stats_get() and ifdeffed out if we don't have CONFIG_CRYPTO_STATS.
regards,
dan carpenter
Quoted 25 lines · Dan Carpenter
Quoted 15 lines · Eric Biggers
Quoted 8 lines · Dan Carpenter
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
crypto_stats_get()is a no-op when the kernel is compiled withoutCONFIG_CRYPTO_STATS, so pairing it withcrypto_alg_put()unconditionally (ascrypto_rng_reset()does) is wrong.Quoted 5 lines
Presumably the intention was that
_get()and_put()should always pair. It's really ugly and horrible that they don't. We could have predicted bug like this would happen and will continue to happen until thecrypto_stats_get()is renamed.Quoted 8 lines
Well, the crypto stats stuff has always been pretty broken, so I don't think people have looked at it too closely. Currently
crypto_stats_get()pairs with one of the functions that tallies the statistics, such ascrypto_stats_rng_seed()orcrypto_stats_aead_encrypt(). What change are you suggesting, exactly? Maybe moving the conditionalcrypto_alg_put()into a new functioncrypto_stats_put()and moving it into the callers? Or do you think the functions should just be renamed to something likecrypto_stats_begin()and crypto_stats_end_{rng_seed,aead_encrypt}()?
Quoted 5 lines
To be honest, I misread the
crypto_alg_put()thinking that it was crypto_*stats*_put(). My favourite fix would be to introduce acrypto_stats_put()which is a mirror ofcrypto_stats_get()and ifdeffed out if we don't haveCONFIG_CRYPTO_STATS.
I agree it will be better.
I can work on adding crypto_stats_put() if you want.
Regards
Herbert Xu
maintainerQuoted 10 lines · Eric Biggers
From: Eric Biggers <[email protected]>
crypto_stats_get()is a no-op when the kernel is compiled withoutCONFIG_CRYPTO_STATS, so pairing it withcrypto_alg_put()unconditionally (ascrypto_rng_reset()does) is wrong.Fix this by moving the call to
crypto_stats_get()to just before the actual algorithm operation which might need it. This makes it always paired withcrypto_stats_rng_seed().
Quoted 6 lines
Fixes eed74b3eba9e ("crypto: rng - Fix a refcounting bug in crypto_rng_reset()")Signed-off-by Eric Biggers <[email protected]>---
crypto/rng.c| 10 +++-------1 file changed, 3 insertions(+), 7 deletions(-)
Patch applied. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt