A long time ago (in 2.6.9 and 2.4.28), crypto_free_tfm() started zeroing
"alg->cra_ctxsize" bytes before freeing a crypto_tfm:
| commit 94ab49d18f69a816561ae199e05daab709ba912e (from full-history-linux)
| Author: David S. Miller <[email protected]>
| Date: Tue Sep 14 08:21:40 2004 -0700
|
| [CRYPTO]: Zero out tfm before freeing in crypto_free_tfm().
|
| Based upon discussions with Ulrich Kuehn
| ([email protected])
|
| Signed-off-by: James Morris <[email protected]>
| Signed-off-by: David S. Miller <[email protected]>
|
| diff --git a/crypto/api.c b/crypto/api.c
| index 6f0e625..394169a 100644
| --- a/crypto/api.c
| +++ b/crypto/api.c
| @@ -155,8 +155,12 @@ out:
|
| void crypto_free_tfm(struct crypto_tfm *tfm)
| {
| + struct crypto_alg *alg = tfm->__crt_alg;
| + int size = sizeof(*tfm) + alg->cra_ctxsize;
| +
| crypto_exit_ops(tfm);
| - crypto_alg_put(tfm->__crt_alg);
| + crypto_alg_put(alg);
| + memset(tfm, 0, size);
| kfree(tfm);
| }
However, in the mean time, the allocation mechanism for crypto_tfm objects has
been changed twice, by:
1. commit fbdae9f3e7fb57c07cb0d973f113eb25da2e8ff2 ("[CRYPTO] Ensure cit_iv
is aligned correctly"), which replaced "alg->cra_ctxsize" by
"crypto_ctxsize(alg, flags)" in crypto_alloc_tfm(),
2. commit 7b0bac64cd5b74d6f1147524c26216de13a501fd ("crypto: api - Rebirth of
crypto_alloc_tfm"), which introduced the alternative crypto_create_tfm(),
where the memory requirements are based on
"frontend->extsize(alg, frontend)" instead of "alg->cra_ctxsize".
Fortunately (for all current transforms under crypto/), it seems that
"crypto_ctxsize(alg, flags)" and "frontend->extsize(alg, frontend)" are always
at least as large as "alg->cra_ctxsize". But still,
(a) this may leak key information in the few cases where the actual key size
is larger than "alg->cra_ctxsize",
(b) this may change in the future, causing memory corruption.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
On Wed, Feb 04, 2009 at 04:09:04PM +0100, Geert Uytterhoeven wrote:
>
> However, in the mean time, the allocation mechanism for crypto_tfm objects has
> been changed twice, by:
> 1. commit fbdae9f3e7fb57c07cb0d973f113eb25da2e8ff2 ("[CRYPTO] Ensure cit_iv
> is aligned correctly"), which replaced "alg->cra_ctxsize" by
> "crypto_ctxsize(alg, flags)" in crypto_alloc_tfm(),
> 2. commit 7b0bac64cd5b74d6f1147524c26216de13a501fd ("crypto: api - Rebirth of
> crypto_alloc_tfm"), which introduced the alternative crypto_create_tfm(),
> where the memory requirements are based on
> "frontend->extsize(alg, frontend)" instead of "alg->cra_ctxsize".
Good catch. In fact we've been freeing the wrong pointer with
shash all along. I wonder how it avoided crashing.
I'm going to add these to commits to fix them.
commit 412e87ae5d852bc3d836f475c19d954b3324363d
Author: Herbert Xu <[email protected]>
Date: Thu Feb 5 16:51:25 2009 +1100
crypto: shash - Fix tfm destruction
We were freeing an offset into the slab object instead of the
start. This patch fixes it by calling crypto_destroy_tfm which
allows the correct address to be given.
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index cd16d6e..d797e11 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -222,7 +222,7 @@ static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)
static inline void crypto_free_shash(struct crypto_shash *tfm)
{
- crypto_free_tfm(crypto_shash_tfm(tfm));
+ crypto_destroy_tfm(tfm, crypto_shash_tfm(tfm));
}
static inline unsigned int crypto_shash_alignmask(
commit 7b2cd92adc5430b0c1adeb120971852b4ea1ab08
Author: Herbert Xu <[email protected]>
Date: Thu Feb 5 16:48:24 2009 +1100
crypto: api - Fix zeroing on free
Geert Uytterhoeven pointed out that we're not zeroing all the
memory when freeing a transform. This patch fixes it by calling
ksize to ensure that we zero everything in sight.
Reported-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/api.c b/crypto/api.c
index 9975a7b..efe77df 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -557,34 +557,34 @@ err:
return ERR_PTR(err);
}
EXPORT_SYMBOL_GPL(crypto_alloc_tfm);
-
+
/*
- * crypto_free_tfm - Free crypto transform
+ * crypto_destroy_tfm - Free crypto transform
+ * @mem: Start of tfm slab
* @tfm: Transform to free
*
- * crypto_free_tfm() frees up the transform and any associated resources,
+ * This function frees up the transform and any associated resources,
* then drops the refcount on the associated algorithm.
*/
-void crypto_free_tfm(struct crypto_tfm *tfm)
+void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm)
{
struct crypto_alg *alg;
int size;
- if (unlikely(!tfm))
+ if (unlikely(!mem))
return;
alg = tfm->__crt_alg;
- size = sizeof(*tfm) + alg->cra_ctxsize;
+ size = ksize(mem);
if (!tfm->exit && alg->cra_exit)
alg->cra_exit(tfm);
crypto_exit_ops(tfm);
crypto_mod_put(alg);
- memset(tfm, 0, size);
- kfree(tfm);
+ memset(mem, 0, size);
+ kfree(mem);
}
-
-EXPORT_SYMBOL_GPL(crypto_free_tfm);
+EXPORT_SYMBOL_GPL(crypto_destroy_tfm);
int crypto_has_alg(const char *name, u32 type, u32 mask)
{
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 3bacd71..1f2e902 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -552,7 +552,12 @@ struct crypto_tfm *crypto_alloc_tfm(const char *alg_name,
const struct crypto_type *frontend,
u32 type, u32 mask);
struct crypto_tfm *crypto_alloc_base(const char *alg_name, u32 type, u32 mask);
-void crypto_free_tfm(struct crypto_tfm *tfm);
+void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm);
+
+static inline void crypto_free_tfm(struct crypto_tfm *tfm)
+{
+ return crypto_destroy_tfm(tfm, tfm);
+}
int alg_test(const char *driver, const char *alg, u32 type, u32 mask);
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, 5 Feb 2009, Herbert Xu wrote:
> On Wed, Feb 04, 2009 at 04:09:04PM +0100, Geert Uytterhoeven wrote:
> > However, in the mean time, the allocation mechanism for crypto_tfm objects has
> > been changed twice, by:
> > 1. commit fbdae9f3e7fb57c07cb0d973f113eb25da2e8ff2 ("[CRYPTO] Ensure cit_iv
> > is aligned correctly"), which replaced "alg->cra_ctxsize" by
> > "crypto_ctxsize(alg, flags)" in crypto_alloc_tfm(),
> > 2. commit 7b0bac64cd5b74d6f1147524c26216de13a501fd ("crypto: api - Rebirth of
> > crypto_alloc_tfm"), which introduced the alternative crypto_create_tfm(),
> > where the memory requirements are based on
> > "frontend->extsize(alg, frontend)" instead of "alg->cra_ctxsize".
>
> Good catch. In fact we've been freeing the wrong pointer with
> shash all along. I wonder how it avoided crashing.
It doesn't crash because crypto_shash_type.tfmsize =
offsetof(struct crypto_shash, base) = zero.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
On Thu, Feb 05, 2009 at 12:03:20PM +0100, Geert Uytterhoeven wrote:
>
> It doesn't crash because crypto_shash_type.tfmsize =
> offsetof(struct crypto_shash, base) = zero.
Ah yes, I took out all the members from crypto_shash so it was
harmless :)
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt