* [PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm
@ 2019-12-06 2:35 Herbert Xu
2019-12-06 2:36 ` [PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Herbert Xu @ 2019-12-06 2:35 UTC (permalink / raw)
To: Linux Crypto Mailing List
As it stands we only enforce descsize limits when an algorithm is
registered. However, as descsize is dynamic and may be set at
init_tfm time this is not enough. This is why hmac has its own
descsize check.
This series adds descsize limit enforcement at init_tfm time so
that the API takes over the responsibility of checking descsize
after the algorithm's init_tfm has completed.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize 2019-12-06 2:35 [PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu @ 2019-12-06 2:36 ` Herbert Xu 2019-12-06 21:54 ` Eric Biggers 2019-12-06 2:36 ` [PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface Herbert Xu ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Herbert Xu @ 2019-12-06 2:36 UTC (permalink / raw) To: Linux Crypto Mailing List The shash interface supports a dynamic descsize field because of the presence of fallbacks (it's just padlock-sha actually, perhaps we can remove it one day). As it is the API does not verify the setting of descsize at all. It is up to the individual algorithms to ensure that descsize does not exceed the specified maximum value of HASH_MAX_DESCSIZE (going above would cause stack corruption). In order to allow the API to impose this limit directly, this patch adds init_tfm/exit_tfm hooks to the shash_alg structure. We can then verify the descsize setting in the API directly. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/shash.c | 25 +++++++++++++++++++++++++ include/crypto/hash.h | 14 ++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/crypto/shash.c b/crypto/shash.c index e83c5124f6eb..40628712feec 100644 --- a/crypto/shash.c +++ b/crypto/shash.c @@ -386,15 +386,40 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm) return 0; } +static void crypto_shash_exit_tfm(struct crypto_tfm *tfm) +{ + struct crypto_shash *hash = __crypto_shash_cast(tfm); + struct shash_alg *alg = crypto_shash_alg(hash); + + alg->exit_tfm(hash); +} + static int crypto_shash_init_tfm(struct crypto_tfm *tfm) { struct crypto_shash *hash = __crypto_shash_cast(tfm); struct shash_alg *alg = crypto_shash_alg(hash); + int err; hash->descsize = alg->descsize; shash_set_needkey(hash, alg); + if (alg->exit_tfm) + tfm->exit = crypto_shash_exit_tfm; + + if (!alg->init_tfm) + return 0; + + err = alg->init_tfm(hash); + if (err) + return err; + + if (hash->descsize > HASH_MAX_DESCSIZE) { + if (alg->exit_tfm) + alg->exit_tfm(hash); + return -EINVAL; + } + return 0; } diff --git a/include/crypto/hash.h b/include/crypto/hash.h index fe7f73bad1e2..ba1435b38266 100644 --- a/include/crypto/hash.h +++ b/include/crypto/hash.h @@ -169,6 +169,18 @@ struct shash_desc { * @export: see struct ahash_alg * @import: see struct ahash_alg * @setkey: see struct ahash_alg + * @init_tfm: Initialize the cryptographic transformation object. + * This function is used to initialize the cryptographic + * transformation object. This function is called only + * once at the instantiation time, right after the + * transformation context was allocated. In case the + * cryptographic hardware has some special requirements + * which need to be handled by software, this function + * shall check for the precise requirement of the + * transformation and put any software fallbacks in place. + * @exit_tfm: Deinitialize the cryptographic transformation object. + * This is a counterpart to @init_tfm, used to remove + * various changes set in @init_tfm. * @digestsize: see struct ahash_alg * @statesize: see struct ahash_alg * @descsize: Size of the operational state for the message digest. This state @@ -189,6 +201,8 @@ struct shash_alg { int (*import)(struct shash_desc *desc, const void *in); int (*setkey)(struct crypto_shash *tfm, const u8 *key, unsigned int keylen); + int (*init_tfm)(struct crypto_shash *tfm); + void (*exit_tfm)(struct crypto_shash *tfm); unsigned int descsize; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize 2019-12-06 2:36 ` [PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu @ 2019-12-06 21:54 ` Eric Biggers 0 siblings, 0 replies; 14+ messages in thread From: Eric Biggers @ 2019-12-06 21:54 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Fri, Dec 06, 2019 at 10:36:18AM +0800, Herbert Xu wrote: > The shash interface supports a dynamic descsize field because of > the presence of fallbacks (it's just padlock-sha actually, perhaps > we can remove it one day). As it is the API does not verify the > setting of descsize at all. It is up to the individual algorithms > to ensure that descsize does not exceed the specified maximum value > of HASH_MAX_DESCSIZE (going above would cause stack corruption). > > In order to allow the API to impose this limit directly, this patch > adds init_tfm/exit_tfm hooks to the shash_alg structure. We can > then verify the descsize setting in the API directly. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > --- > > crypto/shash.c | 25 +++++++++++++++++++++++++ > include/crypto/hash.h | 14 ++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/crypto/shash.c b/crypto/shash.c > index e83c5124f6eb..40628712feec 100644 > --- a/crypto/shash.c > +++ b/crypto/shash.c > @@ -386,15 +386,40 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm) > return 0; > } > > +static void crypto_shash_exit_tfm(struct crypto_tfm *tfm) > +{ > + struct crypto_shash *hash = __crypto_shash_cast(tfm); > + struct shash_alg *alg = crypto_shash_alg(hash); > + > + alg->exit_tfm(hash); > +} > + > static int crypto_shash_init_tfm(struct crypto_tfm *tfm) > { > struct crypto_shash *hash = __crypto_shash_cast(tfm); > struct shash_alg *alg = crypto_shash_alg(hash); > + int err; > > hash->descsize = alg->descsize; > > shash_set_needkey(hash, alg); > > + if (alg->exit_tfm) > + tfm->exit = crypto_shash_exit_tfm; > + > + if (!alg->init_tfm) > + return 0; > + > + err = alg->init_tfm(hash); > + if (err) > + return err; > + > + if (hash->descsize > HASH_MAX_DESCSIZE) { Use WARN_ON_ONCE() here? If HASH_MAX_DESCSIZE is too low for some case, it's a bug that needs to be fixed. > + * @init_tfm: Initialize the cryptographic transformation object. > + * This function is used to initialize the cryptographic > + * transformation object. This function is called only > + * once at the instantiation time, right after the > + * transformation context was allocated. In case the > + * cryptographic hardware has some special requirements > + * which need to be handled by software, this function > + * shall check for the precise requirement of the > + * transformation and put any software fallbacks in place. The second sentence can be removed, since it's redundant with the first. - Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface 2019-12-06 2:35 [PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 2019-12-06 2:36 ` [PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu @ 2019-12-06 2:36 ` Herbert Xu 2019-12-06 2:36 ` [PATCH 3/3] crypto: hmac " Herbert Xu 2019-12-08 5:42 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 3 siblings, 0 replies; 14+ messages in thread From: Herbert Xu @ 2019-12-06 2:36 UTC (permalink / raw) To: Linux Crypto Mailing List This patch switches padlock-sha over to the new init_tfm/exit_tfm interface as opposed to cra_init/cra_exit. This way the shash API can make sure that descsize does not exceed the maximum. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- drivers/crypto/padlock-sha.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/drivers/crypto/padlock-sha.c b/drivers/crypto/padlock-sha.c index ddf1b549fdca..68a06487a665 100644 --- a/drivers/crypto/padlock-sha.c +++ b/drivers/crypto/padlock-sha.c @@ -190,13 +190,11 @@ static int padlock_sha256_final(struct shash_desc *desc, u8 *out) return padlock_sha256_finup(desc, buf, 0, out); } -static int padlock_cra_init(struct crypto_tfm *tfm) +static int padlock_cra_init(struct crypto_shash *hash) { - struct crypto_shash *hash = __crypto_shash_cast(tfm); - const char *fallback_driver_name = crypto_tfm_alg_name(tfm); - struct padlock_sha_ctx *ctx = crypto_tfm_ctx(tfm); + const char *fallback_driver_name = crypto_shash_alg_name(hash); + struct padlock_sha_ctx *ctx = crypto_shash_ctx(hash); struct crypto_shash *fallback_tfm; - int err = -ENOMEM; /* Allocate a fallback and abort if it failed. */ fallback_tfm = crypto_alloc_shash(fallback_driver_name, 0, @@ -204,21 +202,17 @@ static int padlock_cra_init(struct crypto_tfm *tfm) if (IS_ERR(fallback_tfm)) { printk(KERN_WARNING PFX "Fallback driver '%s' could not be loaded!\n", fallback_driver_name); - err = PTR_ERR(fallback_tfm); - goto out; + return PTR_ERR(fallback_tfm); } ctx->fallback = fallback_tfm; hash->descsize += crypto_shash_descsize(fallback_tfm); return 0; - -out: - return err; } -static void padlock_cra_exit(struct crypto_tfm *tfm) +static void padlock_cra_exit(struct crypto_shash *hash) { - struct padlock_sha_ctx *ctx = crypto_tfm_ctx(tfm); + struct padlock_sha_ctx *ctx = crypto_shash_ctx(hash); crypto_free_shash(ctx->fallback); } @@ -231,6 +225,8 @@ static struct shash_alg sha1_alg = { .final = padlock_sha1_final, .export = padlock_sha_export, .import = padlock_sha_import, + .init_tfm = padlock_cra_init, + .exit_tfm = padlock_cra_exit, .descsize = sizeof(struct padlock_sha_desc), .statesize = sizeof(struct sha1_state), .base = { @@ -241,8 +237,6 @@ static struct shash_alg sha1_alg = { .cra_blocksize = SHA1_BLOCK_SIZE, .cra_ctxsize = sizeof(struct padlock_sha_ctx), .cra_module = THIS_MODULE, - .cra_init = padlock_cra_init, - .cra_exit = padlock_cra_exit, } }; @@ -254,6 +248,8 @@ static struct shash_alg sha256_alg = { .final = padlock_sha256_final, .export = padlock_sha_export, .import = padlock_sha_import, + .init_tfm = padlock_cra_init, + .exit_tfm = padlock_cra_exit, .descsize = sizeof(struct padlock_sha_desc), .statesize = sizeof(struct sha256_state), .base = { @@ -264,8 +260,6 @@ static struct shash_alg sha256_alg = { .cra_blocksize = SHA256_BLOCK_SIZE, .cra_ctxsize = sizeof(struct padlock_sha_ctx), .cra_module = THIS_MODULE, - .cra_init = padlock_cra_init, - .cra_exit = padlock_cra_exit, } }; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] crypto: hmac - Use init_tfm/exit_tfm interface 2019-12-06 2:35 [PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 2019-12-06 2:36 ` [PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu 2019-12-06 2:36 ` [PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface Herbert Xu @ 2019-12-06 2:36 ` Herbert Xu 2019-12-06 21:58 ` Eric Biggers 2019-12-08 5:42 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 3 siblings, 1 reply; 14+ messages in thread From: Herbert Xu @ 2019-12-06 2:36 UTC (permalink / raw) To: Linux Crypto Mailing List This patch switches hmac over to the new init_tfm/exit_tfm interface as opposed to cra_init/cra_exit. This way the shash API can make sure that descsize does not exceed the maximum. This patch also adds the API helper shash_alg_instance. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/hmac.c | 20 +++++++------------- include/crypto/internal/hash.h | 6 ++++++ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crypto/hmac.c b/crypto/hmac.c index 8b2a212eb0ad..00a7abd7275d 100644 --- a/crypto/hmac.c +++ b/crypto/hmac.c @@ -138,12 +138,11 @@ static int hmac_finup(struct shash_desc *pdesc, const u8 *data, crypto_shash_finup(desc, out, ds, out); } -static int hmac_init_tfm(struct crypto_tfm *tfm) +static int hmac_init_tfm(struct crypto_shash *parent) { - struct crypto_shash *parent = __crypto_shash_cast(tfm); struct crypto_shash *hash; - struct crypto_instance *inst = (void *)tfm->__crt_alg; - struct crypto_shash_spawn *spawn = crypto_instance_ctx(inst); + struct shash_instance *inst = shash_alg_instance(parent); + struct crypto_shash_spawn *spawn = shash_instance_ctx(inst); struct hmac_ctx *ctx = hmac_ctx(parent); hash = crypto_spawn_shash(spawn); @@ -152,18 +151,14 @@ static int hmac_init_tfm(struct crypto_tfm *tfm) parent->descsize = sizeof(struct shash_desc) + crypto_shash_descsize(hash); - if (WARN_ON(parent->descsize > HASH_MAX_DESCSIZE)) { - crypto_free_shash(hash); - return -EINVAL; - } ctx->hash = hash; return 0; } -static void hmac_exit_tfm(struct crypto_tfm *tfm) +static void hmac_exit_tfm(struct crypto_shash *parent) { - struct hmac_ctx *ctx = hmac_ctx(__crypto_shash_cast(tfm)); + struct hmac_ctx *ctx = hmac_ctx(parent); crypto_free_shash(ctx->hash); } @@ -217,9 +212,6 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) inst->alg.base.cra_ctxsize = sizeof(struct hmac_ctx) + ALIGN(ss * 2, crypto_tfm_ctx_alignment()); - inst->alg.base.cra_init = hmac_init_tfm; - inst->alg.base.cra_exit = hmac_exit_tfm; - inst->alg.init = hmac_init; inst->alg.update = hmac_update; inst->alg.final = hmac_final; @@ -227,6 +219,8 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) inst->alg.export = hmac_export; inst->alg.import = hmac_import; inst->alg.setkey = hmac_setkey; + inst->alg.init_tfm = hmac_init_tfm; + inst->alg.exit_tfm = hmac_exit_tfm; err = shash_register_instance(tmpl, inst); if (err) { diff --git a/include/crypto/internal/hash.h b/include/crypto/internal/hash.h index bfc9db7b100d..7bf4181c7abb 100644 --- a/include/crypto/internal/hash.h +++ b/include/crypto/internal/hash.h @@ -214,6 +214,12 @@ static inline struct shash_instance *shash_instance( struct shash_instance, alg); } +static inline struct shash_instance *shash_alg_instance( + struct crypto_shash *shash) +{ + return shash_instance(crypto_tfm_alg_instance(&shash->base)); +} + static inline void *shash_instance_ctx(struct shash_instance *inst) { return crypto_instance_ctx(shash_crypto_instance(inst)); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] crypto: hmac - Use init_tfm/exit_tfm interface 2019-12-06 2:36 ` [PATCH 3/3] crypto: hmac " Herbert Xu @ 2019-12-06 21:58 ` Eric Biggers 0 siblings, 0 replies; 14+ messages in thread From: Eric Biggers @ 2019-12-06 21:58 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Fri, Dec 06, 2019 at 10:36:21AM +0800, Herbert Xu wrote: > diff --git a/include/crypto/internal/hash.h b/include/crypto/internal/hash.h > index bfc9db7b100d..7bf4181c7abb 100644 > --- a/include/crypto/internal/hash.h > +++ b/include/crypto/internal/hash.h > @@ -214,6 +214,12 @@ static inline struct shash_instance *shash_instance( > struct shash_instance, alg); > } > > +static inline struct shash_instance *shash_alg_instance( > + struct crypto_shash *shash) > +{ > + return shash_instance(crypto_tfm_alg_instance(&shash->base)); > +} > + > static inline void *shash_instance_ctx(struct shash_instance *inst) > { > return crypto_instance_ctx(shash_crypto_instance(inst)); Please run checkpatch: ERROR: trailing whitespace #86: FILE: include/crypto/internal/hash.h:223: +static inline struct shash_instance *shash_alg_instance(^I$ - Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm 2019-12-06 2:35 [PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu ` (2 preceding siblings ...) 2019-12-06 2:36 ` [PATCH 3/3] crypto: hmac " Herbert Xu @ 2019-12-08 5:42 ` Herbert Xu 2019-12-08 5:42 ` [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu ` (3 more replies) 3 siblings, 4 replies; 14+ messages in thread From: Herbert Xu @ 2019-12-08 5:42 UTC (permalink / raw) To: Linux Crypto Mailing List; +Cc: Eric Biggers As it stands we only enforce descsize limits when an algorithm is registered. However, as descsize is dynamic and may be set at init_tfm time this is not enough. This is why hmac has its own descsize check. This series adds descsize limit enforcement at init_tfm time so that the API takes over the responsibility of checking descsize after the algorithm's init_tfm has completed. v2 addresses the issues raised during review, including adding a WARN_ON_ONCE to crypto_shash_init_tfm. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 14+ messages in thread
* [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize 2019-12-08 5:42 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu @ 2019-12-08 5:42 ` Herbert Xu 2019-12-11 3:30 ` Eric Biggers 2019-12-08 5:42 ` [v2 PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface Herbert Xu ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Herbert Xu @ 2019-12-08 5:42 UTC (permalink / raw) To: Linux Crypto Mailing List, Eric Biggers The shash interface supports a dynamic descsize field because of the presence of fallbacks (it's just padlock-sha actually, perhaps we can remove it one day). As it is the API does not verify the setting of descsize at all. It is up to the individual algorithms to ensure that descsize does not exceed the specified maximum value of HASH_MAX_DESCSIZE (going above would cause stack corruption). In order to allow the API to impose this limit directly, this patch adds init_tfm/exit_tfm hooks to the shash_alg structure. We can then verify the descsize setting in the API directly. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/shash.c | 25 +++++++++++++++++++++++++ include/crypto/hash.h | 13 +++++++++++++ 2 files changed, 38 insertions(+) diff --git a/crypto/shash.c b/crypto/shash.c index e83c5124f6eb..63a7ea368eb1 100644 --- a/crypto/shash.c +++ b/crypto/shash.c @@ -386,15 +386,40 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm) return 0; } +static void crypto_shash_exit_tfm(struct crypto_tfm *tfm) +{ + struct crypto_shash *hash = __crypto_shash_cast(tfm); + struct shash_alg *alg = crypto_shash_alg(hash); + + alg->exit_tfm(hash); +} + static int crypto_shash_init_tfm(struct crypto_tfm *tfm) { struct crypto_shash *hash = __crypto_shash_cast(tfm); struct shash_alg *alg = crypto_shash_alg(hash); + int err; hash->descsize = alg->descsize; shash_set_needkey(hash, alg); + if (alg->exit_tfm) + tfm->exit = crypto_shash_exit_tfm; + + if (!alg->init_tfm) + return 0; + + err = alg->init_tfm(hash); + if (err) + return err; + + if (WARN_ON_ONCE(hash->descsize > HASH_MAX_DESCSIZE)) { + if (alg->exit_tfm) + alg->exit_tfm(hash); + return -EINVAL; + } + return 0; } diff --git a/include/crypto/hash.h b/include/crypto/hash.h index fe7f73bad1e2..cee446c59497 100644 --- a/include/crypto/hash.h +++ b/include/crypto/hash.h @@ -169,6 +169,17 @@ struct shash_desc { * @export: see struct ahash_alg * @import: see struct ahash_alg * @setkey: see struct ahash_alg + * @init_tfm: Initialize the cryptographic transformation object. + * This function is called only once at the instantiation + * time, right after the transformation context was + * allocated. In case the cryptographic hardware has + * some special requirements which need to be handled + * by software, this function shall check for the precise + * requirement of the transformation and put any software + * fallbacks in place. + * @exit_tfm: Deinitialize the cryptographic transformation object. + * This is a counterpart to @init_tfm, used to remove + * various changes set in @init_tfm. * @digestsize: see struct ahash_alg * @statesize: see struct ahash_alg * @descsize: Size of the operational state for the message digest. This state @@ -189,6 +200,8 @@ struct shash_alg { int (*import)(struct shash_desc *desc, const void *in); int (*setkey)(struct crypto_shash *tfm, const u8 *key, unsigned int keylen); + int (*init_tfm)(struct crypto_shash *tfm); + void (*exit_tfm)(struct crypto_shash *tfm); unsigned int descsize; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize 2019-12-08 5:42 ` [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu @ 2019-12-11 3:30 ` Eric Biggers 0 siblings, 0 replies; 14+ messages in thread From: Eric Biggers @ 2019-12-11 3:30 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Sun, Dec 08, 2019 at 01:42:51PM +0800, Herbert Xu wrote: > The shash interface supports a dynamic descsize field because of > the presence of fallbacks (it's just padlock-sha actually, perhaps > we can remove it one day). As it is the API does not verify the > setting of descsize at all. It is up to the individual algorithms > to ensure that descsize does not exceed the specified maximum value > of HASH_MAX_DESCSIZE (going above would cause stack corruption). > > In order to allow the API to impose this limit directly, this patch > adds init_tfm/exit_tfm hooks to the shash_alg structure. We can > then verify the descsize setting in the API directly. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > --- > > crypto/shash.c | 25 +++++++++++++++++++++++++ > include/crypto/hash.h | 13 +++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/crypto/shash.c b/crypto/shash.c > index e83c5124f6eb..63a7ea368eb1 100644 > --- a/crypto/shash.c > +++ b/crypto/shash.c > @@ -386,15 +386,40 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm) > return 0; > } > > +static void crypto_shash_exit_tfm(struct crypto_tfm *tfm) > +{ > + struct crypto_shash *hash = __crypto_shash_cast(tfm); > + struct shash_alg *alg = crypto_shash_alg(hash); > + > + alg->exit_tfm(hash); > +} > + > static int crypto_shash_init_tfm(struct crypto_tfm *tfm) > { > struct crypto_shash *hash = __crypto_shash_cast(tfm); > struct shash_alg *alg = crypto_shash_alg(hash); > + int err; > > hash->descsize = alg->descsize; > > shash_set_needkey(hash, alg); > > + if (alg->exit_tfm) > + tfm->exit = crypto_shash_exit_tfm; > + > + if (!alg->init_tfm) > + return 0; > + > + err = alg->init_tfm(hash); > + if (err) > + return err; > + > + if (WARN_ON_ONCE(hash->descsize > HASH_MAX_DESCSIZE)) { > + if (alg->exit_tfm) > + alg->exit_tfm(hash); > + return -EINVAL; > + } Nit: it would be helpful to have a comment just above the WARN_ON_ONCE() like: /* ->init_tfm() may have increased the descsize. */ - Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* [v2 PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface 2019-12-08 5:42 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 2019-12-08 5:42 ` [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu @ 2019-12-08 5:42 ` Herbert Xu 2019-12-11 3:31 ` Eric Biggers 2019-12-08 5:42 ` [v2 PATCH 3/3] crypto: hmac " Herbert Xu 2019-12-11 3:32 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Eric Biggers 3 siblings, 1 reply; 14+ messages in thread From: Herbert Xu @ 2019-12-08 5:42 UTC (permalink / raw) To: Linux Crypto Mailing List, Eric Biggers This patch switches padlock-sha over to the new init_tfm/exit_tfm interface as opposed to cra_init/cra_exit. This way the shash API can make sure that descsize does not exceed the maximum. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- drivers/crypto/padlock-sha.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/drivers/crypto/padlock-sha.c b/drivers/crypto/padlock-sha.c index ddf1b549fdca..68a06487a665 100644 --- a/drivers/crypto/padlock-sha.c +++ b/drivers/crypto/padlock-sha.c @@ -190,13 +190,11 @@ static int padlock_sha256_final(struct shash_desc *desc, u8 *out) return padlock_sha256_finup(desc, buf, 0, out); } -static int padlock_cra_init(struct crypto_tfm *tfm) +static int padlock_cra_init(struct crypto_shash *hash) { - struct crypto_shash *hash = __crypto_shash_cast(tfm); - const char *fallback_driver_name = crypto_tfm_alg_name(tfm); - struct padlock_sha_ctx *ctx = crypto_tfm_ctx(tfm); + const char *fallback_driver_name = crypto_shash_alg_name(hash); + struct padlock_sha_ctx *ctx = crypto_shash_ctx(hash); struct crypto_shash *fallback_tfm; - int err = -ENOMEM; /* Allocate a fallback and abort if it failed. */ fallback_tfm = crypto_alloc_shash(fallback_driver_name, 0, @@ -204,21 +202,17 @@ static int padlock_cra_init(struct crypto_tfm *tfm) if (IS_ERR(fallback_tfm)) { printk(KERN_WARNING PFX "Fallback driver '%s' could not be loaded!\n", fallback_driver_name); - err = PTR_ERR(fallback_tfm); - goto out; + return PTR_ERR(fallback_tfm); } ctx->fallback = fallback_tfm; hash->descsize += crypto_shash_descsize(fallback_tfm); return 0; - -out: - return err; } -static void padlock_cra_exit(struct crypto_tfm *tfm) +static void padlock_cra_exit(struct crypto_shash *hash) { - struct padlock_sha_ctx *ctx = crypto_tfm_ctx(tfm); + struct padlock_sha_ctx *ctx = crypto_shash_ctx(hash); crypto_free_shash(ctx->fallback); } @@ -231,6 +225,8 @@ static struct shash_alg sha1_alg = { .final = padlock_sha1_final, .export = padlock_sha_export, .import = padlock_sha_import, + .init_tfm = padlock_cra_init, + .exit_tfm = padlock_cra_exit, .descsize = sizeof(struct padlock_sha_desc), .statesize = sizeof(struct sha1_state), .base = { @@ -241,8 +237,6 @@ static struct shash_alg sha1_alg = { .cra_blocksize = SHA1_BLOCK_SIZE, .cra_ctxsize = sizeof(struct padlock_sha_ctx), .cra_module = THIS_MODULE, - .cra_init = padlock_cra_init, - .cra_exit = padlock_cra_exit, } }; @@ -254,6 +248,8 @@ static struct shash_alg sha256_alg = { .final = padlock_sha256_final, .export = padlock_sha_export, .import = padlock_sha_import, + .init_tfm = padlock_cra_init, + .exit_tfm = padlock_cra_exit, .descsize = sizeof(struct padlock_sha_desc), .statesize = sizeof(struct sha256_state), .base = { @@ -264,8 +260,6 @@ static struct shash_alg sha256_alg = { .cra_blocksize = SHA256_BLOCK_SIZE, .cra_ctxsize = sizeof(struct padlock_sha_ctx), .cra_module = THIS_MODULE, - .cra_init = padlock_cra_init, - .cra_exit = padlock_cra_exit, } }; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [v2 PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface 2019-12-08 5:42 ` [v2 PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface Herbert Xu @ 2019-12-11 3:31 ` Eric Biggers 0 siblings, 0 replies; 14+ messages in thread From: Eric Biggers @ 2019-12-11 3:31 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Sun, Dec 08, 2019 at 01:42:52PM +0800, Herbert Xu wrote: > @@ -231,6 +225,8 @@ static struct shash_alg sha1_alg = { > .final = padlock_sha1_final, > .export = padlock_sha_export, > .import = padlock_sha_import, > + .init_tfm = padlock_cra_init, > + .exit_tfm = padlock_cra_exit, > .descsize = sizeof(struct padlock_sha_desc), > .statesize = sizeof(struct sha1_state), Nit: these should be renamed to padlock_sha_init_tfm() and padlock_sha_exit_tfm(). - Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* [v2 PATCH 3/3] crypto: hmac - Use init_tfm/exit_tfm interface 2019-12-08 5:42 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 2019-12-08 5:42 ` [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu 2019-12-08 5:42 ` [v2 PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface Herbert Xu @ 2019-12-08 5:42 ` Herbert Xu 2019-12-11 3:32 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Eric Biggers 3 siblings, 0 replies; 14+ messages in thread From: Herbert Xu @ 2019-12-08 5:42 UTC (permalink / raw) To: Linux Crypto Mailing List, Eric Biggers This patch switches hmac over to the new init_tfm/exit_tfm interface as opposed to cra_init/cra_exit. This way the shash API can make sure that descsize does not exceed the maximum. This patch also adds the API helper shash_alg_instance. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/hmac.c | 20 +++++++------------- include/crypto/internal/hash.h | 6 ++++++ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crypto/hmac.c b/crypto/hmac.c index 8b2a212eb0ad..00a7abd7275d 100644 --- a/crypto/hmac.c +++ b/crypto/hmac.c @@ -138,12 +138,11 @@ static int hmac_finup(struct shash_desc *pdesc, const u8 *data, crypto_shash_finup(desc, out, ds, out); } -static int hmac_init_tfm(struct crypto_tfm *tfm) +static int hmac_init_tfm(struct crypto_shash *parent) { - struct crypto_shash *parent = __crypto_shash_cast(tfm); struct crypto_shash *hash; - struct crypto_instance *inst = (void *)tfm->__crt_alg; - struct crypto_shash_spawn *spawn = crypto_instance_ctx(inst); + struct shash_instance *inst = shash_alg_instance(parent); + struct crypto_shash_spawn *spawn = shash_instance_ctx(inst); struct hmac_ctx *ctx = hmac_ctx(parent); hash = crypto_spawn_shash(spawn); @@ -152,18 +151,14 @@ static int hmac_init_tfm(struct crypto_tfm *tfm) parent->descsize = sizeof(struct shash_desc) + crypto_shash_descsize(hash); - if (WARN_ON(parent->descsize > HASH_MAX_DESCSIZE)) { - crypto_free_shash(hash); - return -EINVAL; - } ctx->hash = hash; return 0; } -static void hmac_exit_tfm(struct crypto_tfm *tfm) +static void hmac_exit_tfm(struct crypto_shash *parent) { - struct hmac_ctx *ctx = hmac_ctx(__crypto_shash_cast(tfm)); + struct hmac_ctx *ctx = hmac_ctx(parent); crypto_free_shash(ctx->hash); } @@ -217,9 +212,6 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) inst->alg.base.cra_ctxsize = sizeof(struct hmac_ctx) + ALIGN(ss * 2, crypto_tfm_ctx_alignment()); - inst->alg.base.cra_init = hmac_init_tfm; - inst->alg.base.cra_exit = hmac_exit_tfm; - inst->alg.init = hmac_init; inst->alg.update = hmac_update; inst->alg.final = hmac_final; @@ -227,6 +219,8 @@ static int hmac_create(struct crypto_template *tmpl, struct rtattr **tb) inst->alg.export = hmac_export; inst->alg.import = hmac_import; inst->alg.setkey = hmac_setkey; + inst->alg.init_tfm = hmac_init_tfm; + inst->alg.exit_tfm = hmac_exit_tfm; err = shash_register_instance(tmpl, inst); if (err) { diff --git a/include/crypto/internal/hash.h b/include/crypto/internal/hash.h index bfc9db7b100d..80e4b75c3771 100644 --- a/include/crypto/internal/hash.h +++ b/include/crypto/internal/hash.h @@ -214,6 +214,12 @@ static inline struct shash_instance *shash_instance( struct shash_instance, alg); } +static inline struct shash_instance *shash_alg_instance( + struct crypto_shash *shash) +{ + return shash_instance(crypto_tfm_alg_instance(&shash->base)); +} + static inline void *shash_instance_ctx(struct shash_instance *inst) { return crypto_instance_ctx(shash_crypto_instance(inst)); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm 2019-12-08 5:42 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu ` (2 preceding siblings ...) 2019-12-08 5:42 ` [v2 PATCH 3/3] crypto: hmac " Herbert Xu @ 2019-12-11 3:32 ` Eric Biggers 2019-12-11 8:30 ` Herbert Xu 3 siblings, 1 reply; 14+ messages in thread From: Eric Biggers @ 2019-12-11 3:32 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Sun, Dec 08, 2019 at 01:42:29PM +0800, Herbert Xu wrote: > As it stands we only enforce descsize limits when an algorithm is > registered. However, as descsize is dynamic and may be set at > init_tfm time this is not enough. This is why hmac has its own > descsize check. > > This series adds descsize limit enforcement at init_tfm time so > that the API takes over the responsibility of checking descsize > after the algorithm's init_tfm has completed. > > v2 addresses the issues raised during review, including adding > a WARN_ON_ONCE to crypto_shash_init_tfm. > > Thanks, I left some nits on patches 1 and 2, but not too important. Feel free to add: Reviewed-by: Eric Biggers <ebiggers@google.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm 2019-12-11 3:32 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Eric Biggers @ 2019-12-11 8:30 ` Herbert Xu 0 siblings, 0 replies; 14+ messages in thread From: Herbert Xu @ 2019-12-11 8:30 UTC (permalink / raw) To: Eric Biggers; +Cc: Linux Crypto Mailing List On Tue, Dec 10, 2019 at 07:32:11PM -0800, Eric Biggers wrote: > > I left some nits on patches 1 and 2, but not too important. Feel free to add: > > Reviewed-by: Eric Biggers <ebiggers@google.com> Thanks. I've added your suggestions as well as your review tag. -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-12-11 8:30 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-06 2:35 [PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 2019-12-06 2:36 ` [PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu 2019-12-06 21:54 ` Eric Biggers 2019-12-06 2:36 ` [PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface Herbert Xu 2019-12-06 2:36 ` [PATCH 3/3] crypto: hmac " Herbert Xu 2019-12-06 21:58 ` Eric Biggers 2019-12-08 5:42 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Herbert Xu 2019-12-08 5:42 ` [v2 PATCH 1/3] crypto: shash - Add init_tfm/exit_tfm and verify descsize Herbert Xu 2019-12-11 3:30 ` Eric Biggers 2019-12-08 5:42 ` [v2 PATCH 2/3] crypto: padlock-sha - Use init_tfm/exit_tfm interface Herbert Xu 2019-12-11 3:31 ` Eric Biggers 2019-12-08 5:42 ` [v2 PATCH 3/3] crypto: hmac " Herbert Xu 2019-12-11 3:32 ` [v2 PATCH 0/3] crypto: shash - Enforce descsize limit in init_tfm Eric Biggers 2019-12-11 8:30 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).