| Message ID | 20160927194644.GB15729@gallifrey (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
On Tue, Sep 27, 2016 at 04:46:44PM -0300, Marcelo Cerri wrote: > > Can you check if the problem occurs with this patch? In light of the fact that padlock-sha is the correct example to follow, you only need to add one line to the init_tfm fucntion to update the descsize based on that of the fallback. Thanks,
----- Original Message ----- > From: "Herbert Xu" <herbert@gondor.apana.org.au> > To: "Marcelo Cerri" <marcelo.cerri@canonical.com> > Cc: "Jan Stancek" <jstancek@redhat.com>, "rui y wang" <rui.y.wang@intel.com>, mhcerri@linux.vnet.ibm.com, > leosilva@linux.vnet.ibm.com, pfsmorigo@linux.vnet.ibm.com, linux-crypto@vger.kernel.org, > linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org > Sent: Wednesday, 28 September, 2016 4:45:49 AM > Subject: Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7 > > On Tue, Sep 27, 2016 at 04:46:44PM -0300, Marcelo Cerri wrote: > > > > Can you check if the problem occurs with this patch? > > In light of the fact that padlock-sha is the correct example > to follow, you only need to add one line to the init_tfm fucntion > to update the descsize based on that of the fallback. Thanks for clearing up how this works in padlock-sha, but we are not exactly in same situation with p8_ghash. p8_ghash_init_tfm() already updates descsize. Problem in original report is that without custom export/import/statesize p8_ghash_alg.statesize gets initialized by shash_prepare_alg() to alg->descsize: crash> p p8_ghash_alg.statesize $1 = 56 testmgr allocates space for export based on crypto_shash_statesize(), but shash_default_export() writes based on crypto_shash_descsize(): [ 8.297902] state: c0000004b873aa80, statesize: 56 [ 8.297932] shash_default_export memcpy c0000004b873aa80 c0000004b8607da0, len: 76 so I think we need either: 1) make sure p8_ghash_alg.descsize is correct before we register shash, this is what Marcelo's last patch is doing 2) provide custom export/import/statesize for p8_ghash_alg Regards, Jan > > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Jan, > > Can you check if the problem occurs with this patch? No issues in over-night test with this patch. > --- a/drivers/crypto/vmx/vmx.c > +++ b/drivers/crypto/vmx/vmx.c > @@ -28,6 +28,8 @@ > #include <asm/cputable.h> > #include <crypto/internal/hash.h> > > +int p8_ghash_fallback_descsize(void); > + > extern struct shash_alg p8_ghash_alg; > extern struct crypto_alg p8_aes_alg; > extern struct crypto_alg p8_aes_cbc_alg; > @@ -45,6 +47,7 @@ int __init p8_init(void) > { > int ret = 0; > struct crypto_alg **alg_it; > + int ghash_descsize; > > for (alg_it = algs; *alg_it; alg_it++) { > ret = crypto_register_alg(*alg_it); > @@ -59,6 +62,12 @@ int __init p8_init(void) > if (ret) > return ret; > > + ghash_descsize = p8_ghash_fallback_descsize(); > + if (ghash_descsize < 0) { > + printk(KERN_ERR "Cannot get descsize for p8_ghash fallback\n"); > + return ghash_descsize; > + } > + p8_ghash_alg.descsize += ghash_descsize; > ret = crypto_register_shash(&p8_ghash_alg); > if (ret) { > for (alg_it = algs; *alg_it; alg_it++) I'd suggest to move this inside vmx/ghash.c to a new function, so all p8_ghash related code is at single place. Then p8_init() would just call the new function: - ret = crypto_register_shash(&p8_ghash_alg); + ret = register_p8_ghash(); Regards, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Herbert, On Wed, Sep 28, 2016 at 10:45:49AM +0800, Herbert Xu wrote: > On Tue, Sep 27, 2016 at 04:46:44PM -0300, Marcelo Cerri wrote: > > > > Can you check if the problem occurs with this patch? > > In light of the fact that padlock-sha is the correct example > to follow, you only need to add one line to the init_tfm fucntion > to update the descsize based on that of the fallback. > Not sure if I'm missing something here but p8_ghash already does that: 56 static int p8_ghash_init_tfm(struct crypto_tfm *tfm) 57 { ... 83 shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx) 84 + crypto_shash_descsize(fallback); 85 86 return 0; 87 } I think the problem here is that crypto_ahash_statesize uses the statesize value (that is initialized with the descsize value from shash_alg) instead of using the value from the tfm that was updated. For padlock_sha1, it uses the value from alg->statesize since it defines import and export functions. It doesn't make much difference if it uses the value from descsize or statesize here, what really matter is that it uses the value defined in struct shash_alg and not from the tfm. The big difference between p8_ghash and padlock_sha1 is that padlock_sha1 defines alg->statesize as sizeof(struct sha1_state), which is the descsize value used by sha1_generic. This probably works but it's also wrong because the padlock_sha1 driver does not ensures that sha1_generic is always used. So, one solution is to hardcode ghash-generic as the fallback algorithm and update the descsize direct in its shash_alg structure. There's only one problem with that. ghash-generic desc type (struct ghash_desc_ctx) is not available for vmx_crypto at compile type, the type is defined directly in crypto/ghash-generic.c. That's the reason I added a function to get the fallback desc size at runtime in the patch I wrote as a prove of concept. -- Regards, Marcelo
On Wed, Sep 28, 2016 at 03:40:51AM -0400, Jan Stancek wrote: > > Thanks for clearing up how this works in padlock-sha, but > we are not exactly in same situation with p8_ghash. > > p8_ghash_init_tfm() already updates descsize. Problem in original report > is that without custom export/import/statesize p8_ghash_alg.statesize > gets initialized by shash_prepare_alg() to alg->descsize: Right. > so I think we need either: > 1) make sure p8_ghash_alg.descsize is correct before we register shash, > this is what Marcelo's last patch is doing This approach doesn't work because there is no guarantee that you'll get the same fallback the next time you allocate a tfm. So relying on the descsize being constant can only work if all implementations of the fallback use the same desc struct. > 2) provide custom export/import/statesize for p8_ghash_alg This works for padlock-sha because every implementation of SHA uses the same state data structure from sha.h. If we can make all implementations of ghash agree on the exported state then we can use the same approach. Otherwise we can go back to allocating just ghash-generic and also move its data structure into an exported header file. Cheers,
On Wed, Sep 28, 2016 at 09:28:44AM -0300, Marcelo Cerri wrote: > > The big difference between p8_ghash and padlock_sha1 is that > padlock_sha1 defines alg->statesize as sizeof(struct sha1_state), which > is the descsize value used by sha1_generic. This probably works but > it's also wrong because the padlock_sha1 driver does not ensures that > sha1_generic is always used. It should work because all our SHA implementations use the same export format. This is not necessarily the case for GHASH though. > So, one solution is to hardcode ghash-generic as the fallback algorithm > and update the descsize direct in its shash_alg structure. There's only > one problem with that. ghash-generic desc type (struct ghash_desc_ctx) > is not available for vmx_crypto at compile type, the type is defined > directly in crypto/ghash-generic.c. That's the reason I added a function > to get the fallback desc size at runtime in the patch I wrote as a prove > of concept. The problem with your patch is that there is no guarantee that you will get the same algorithm every time you allocate a fallback. Someone may have loaded a new module for example. So I think the safe approach is to stick with ghash-generic and expose its state data structure in a header file. Thanks,
Hi Hebert, On Wed, Sep 28, 2016 at 08:29:35PM +0800, Herbert Xu wrote: > On Wed, Sep 28, 2016 at 03:40:51AM -0400, Jan Stancek wrote: > > > > Thanks for clearing up how this works in padlock-sha, but > > we are not exactly in same situation with p8_ghash. > > > > p8_ghash_init_tfm() already updates descsize. Problem in original report > > is that without custom export/import/statesize p8_ghash_alg.statesize > > gets initialized by shash_prepare_alg() to alg->descsize: > > Right. > > > so I think we need either: > > 1) make sure p8_ghash_alg.descsize is correct before we register shash, > > this is what Marcelo's last patch is doing > > This approach doesn't work because there is no guarantee that > you'll get the same fallback the next time you allocate a tfm. > So relying on the descsize being constant can only work if all > implementations of the fallback use the same desc struct. The patch forces ghash-generic as the fallback. And I don't think that is a big problem if we decide to go by this path. > > > 2) provide custom export/import/statesize for p8_ghash_alg > > This works for padlock-sha because every implementation of SHA > uses the same state data structure from sha.h. If we can make > all implementations of ghash agree on the exported state then > we can use the same approach. That would be nice because it would allow p8_ghash to keep using a dynamic fallback, but I'm not that is viable. What do you think? > > Otherwise we can go back to allocating just ghash-generic and > also move its data structure into an exported header file. > That would make the fix much more simple and it wouldn't require to get the fallback descsize at runtime. > 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
On Wed, Sep 28, 2016 at 09:38:41AM -0300, Marcelo Cerri wrote: > > The patch forces ghash-generic as the fallback. And I don't think that > is a big problem if we decide to go by this path. Right it should work but could break for example if we ever decide to change the exported state structure for ghash and someone unloads the ghash-generic module and reloads a new one. > That would be nice because it would allow p8_ghash to keep using a > dynamic fallback, but I'm not that is viable. What do you think? We did it for SHA because it was desirable to have multiple fallbacks, i.e., a generic C version plus an assembly-optimised version. Not sure whether the same motiviation exists for GHASH. > > Otherwise we can go back to allocating just ghash-generic and > > also move its data structure into an exported header file. > > > > That would make the fix much more simple and it wouldn't require to get > the fallback descsize at runtime. This is the easiest fix so let's go with this now. If we ever care enough to have multiple fallbacks for GHASH we can always revisit this. The exported format is not exposed to user-space so it can always be changed. Cheers,
On Wed, Sep 28, 2016 at 08:44:52PM +0800, Herbert Xu wrote: > On Wed, Sep 28, 2016 at 09:38:41AM -0300, Marcelo Cerri wrote: > > > > The patch forces ghash-generic as the fallback. And I don't think that > > is a big problem if we decide to go by this path. > > Right it should work but could break for example if we ever decide > to change the exported state structure for ghash and someone unloads > the ghash-generic module and reloads a new one. > Great! If we check the descsize every time a fallback tfm is allocated that should be enough to prevent bigger problems such as memory corruptions. > > That would be nice because it would allow p8_ghash to keep using a > > dynamic fallback, but I'm not that is viable. What do you think? > > We did it for SHA because it was desirable to have multiple > fallbacks, i.e., a generic C version plus an assembly-optimised > version. > > Not sure whether the same motiviation exists for GHASH. > > > > Otherwise we can go back to allocating just ghash-generic and > > > also move its data structure into an exported header file. > > > > > > > That would make the fix much more simple and it wouldn't require to get > > the fallback descsize at runtime. > > This is the easiest fix so let's go with this now. If we ever > care enough to have multiple fallbacks for GHASH we can always > revisit this. The exported format is not exposed to user-space > so it can always be changed. Can I move ghash_desc_ctx to a header file under include/crypto/? Or do you do you prefer to do that? Maybe include/crypto/internal/hash.h or a new header file include/crypto/internal/ghash.h ? > > 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
On Wed, Sep 28, 2016 at 09:55:58AM -0300, Marcelo Cerri wrote: > > Great! If we check the descsize every time a fallback tfm is allocated > that should be enough to prevent bigger problems such as memory > corruptions. Yes a check wouldn't hurt. > Can I move ghash_desc_ctx to a header file under include/crypto/? Or do > you do you prefer to do that? No you can go ahead and do the move. > Maybe include/crypto/internal/hash.h or a new header file > include/crypto/internal/ghash.h ? Let's put it in include/crypto/ghash.h. Thanks,
Wed, Sep 28, 2016 at 08:33:18PM +0800, Herbert Xu wrote: > On Wed, Sep 28, 2016 at 09:28:44AM -0300, Marcelo Cerri wrote: > > > > The big difference between p8_ghash and padlock_sha1 is that > > padlock_sha1 defines alg->statesize as sizeof(struct sha1_state), which > > is the descsize value used by sha1_generic. This probably works but > > it's also wrong because the padlock_sha1 driver does not ensures that > > sha1_generic is always used. > > It should work because all our SHA implementations use the same > export format. This is not necessarily the case for GHASH though. > > > So, one solution is to hardcode ghash-generic as the fallback algorithm > > and update the descsize direct in its shash_alg structure. There's only > > one problem with that. ghash-generic desc type (struct ghash_desc_ctx) > > is not available for vmx_crypto at compile type, the type is defined > > directly in crypto/ghash-generic.c. That's the reason I added a function > > to get the fallback desc size at runtime in the patch I wrote as a prove > > of concept. > > The problem with your patch is that there is no guarantee that > you will get the same algorithm every time you allocate a fallback. > Someone may have loaded a new module for example. > > So I think the safe approach is to stick with ghash-generic and > expose its state data structure in a header file. Since generic is the only posible fallback for ppc, I think that's a good solution. :) > > 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 >
diff --git a/drivers/crypto/vmx/ghash.c b/drivers/crypto/vmx/ghash.c index 6c999cb0..033aba1 100644 --- a/drivers/crypto/vmx/ghash.c +++ b/drivers/crypto/vmx/ghash.c @@ -36,6 +36,8 @@ #define GHASH_DIGEST_SIZE (16) #define GHASH_KEY_LEN (16) +#define GHASH_FALLBACK_ALG "ghash-generic" + void gcm_init_p8(u128 htable[16], const u64 Xi[2]); void gcm_gmult_p8(u64 Xi[2], const u128 htable[16]); void gcm_ghash_p8(u64 Xi[2], const u128 htable[16], @@ -53,18 +55,26 @@ struct p8_ghash_desc_ctx { struct shash_desc fallback_desc; }; +int p8_ghash_fallback_descsize(void) +{ + int descsize; + struct crypto_shash *fallback; + fallback = crypto_alloc_shash(GHASH_FALLBACK_ALG, 0, + CRYPTO_ALG_NEED_FALLBACK); + if (IS_ERR(fallback)) { + return PTR_ERR(fallback); + } + descsize = crypto_shash_descsize(fallback); + crypto_free_shash(fallback); + return descsize; +} + static int p8_ghash_init_tfm(struct crypto_tfm *tfm) { - const char *alg; + const char *alg = GHASH_FALLBACK_ALG; struct crypto_shash *fallback; - struct crypto_shash *shash_tfm = __crypto_shash_cast(tfm); struct p8_ghash_ctx *ctx = crypto_tfm_ctx(tfm); - if (!(alg = crypto_tfm_alg_name(tfm))) { - printk(KERN_ERR "Failed to get algorithm name.\n"); - return -ENOENT; - } - fallback = crypto_alloc_shash(alg, 0, CRYPTO_ALG_NEED_FALLBACK); if (IS_ERR(fallback)) { printk(KERN_ERR @@ -79,10 +89,6 @@ static int p8_ghash_init_tfm(struct crypto_tfm *tfm) crypto_shash_get_flags((struct crypto_shash *) tfm)); ctx->fallback = fallback; - - shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx) - + crypto_shash_descsize(fallback); - return 0; } diff --git a/drivers/crypto/vmx/vmx.c b/drivers/crypto/vmx/vmx.c index 31a98dc..8a51149 100644 --- a/drivers/crypto/vmx/vmx.c +++ b/drivers/crypto/vmx/vmx.c @@ -28,6 +28,8 @@ #include <asm/cputable.h> #include <crypto/internal/hash.h> +int p8_ghash_fallback_descsize(void); + extern struct shash_alg p8_ghash_alg; extern struct crypto_alg p8_aes_alg; extern struct crypto_alg p8_aes_cbc_alg; @@ -45,6 +47,7 @@ int __init p8_init(void) { int ret = 0; struct crypto_alg **alg_it; + int ghash_descsize; for (alg_it = algs; *alg_it; alg_it++) { ret = crypto_register_alg(*alg_it); @@ -59,6 +62,12 @@ int __init p8_init(void) if (ret) return ret; + ghash_descsize = p8_ghash_fallback_descsize(); + if (ghash_descsize < 0) { + printk(KERN_ERR "Cannot get descsize for p8_ghash fallback\n"); + return ghash_descsize; + } + p8_ghash_alg.descsize += ghash_descsize; ret = crypto_register_shash(&p8_ghash_alg); if (ret) { for (alg_it = algs; *alg_it; alg_it++)