| Message ID | 1466601840-18486-7-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
On Wed, Jun 22, 2016 at 04:23:39PM +0300, Tero Kristo wrote: > The statesize is used to determine the maximum size for saved ahash > context. In some cases, this can be much larger than what is currently > allocated for it, for example omap-sham driver uses a buffer size of > PAGE_SIZE. Increase the statesize to accommodate this. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> Nack. The exported state is supposed to consist of the actual hash state, plus at most one block worth of unhashed data. It's limited so that we can store it on the stack. So no I'm not taking this patch.
On 24/06/16 13:32, Herbert Xu wrote: > On Wed, Jun 22, 2016 at 04:23:39PM +0300, Tero Kristo wrote: >> The statesize is used to determine the maximum size for saved ahash >> context. In some cases, this can be much larger than what is currently >> allocated for it, for example omap-sham driver uses a buffer size of >> PAGE_SIZE. Increase the statesize to accommodate this. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> > > Nack. The exported state is supposed to consist of the actual > hash state, plus at most one block worth of unhashed data. It's > limited so that we can store it on the stack. > > So no I'm not taking this patch. > Ok, I think I need to allocate the storage space locally then within the driver. Would it be ok to call kmalloc / free in the export / import implementation of the driver? The size of the unhashed buffer in omap-sham is unfortunately rather large. -Tero -- 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
On Mon, Jun 27, 2016 at 07:58:43AM +0300, Tero Kristo wrote: > > Ok, I think I need to allocate the storage space locally then within > the driver. Would it be ok to call kmalloc / free in the export / > import implementation of the driver? The size of the unhashed buffer > in omap-sham is unfortunately rather large. The allocation should usually be done from the request_alloc function, i.e., you set the reqsize and the user does the allocation for you. Cheers,
On 27/06/16 08:00, Herbert Xu wrote: > On Mon, Jun 27, 2016 at 07:58:43AM +0300, Tero Kristo wrote: >> >> Ok, I think I need to allocate the storage space locally then within >> the driver. Would it be ok to call kmalloc / free in the export / >> import implementation of the driver? The size of the unhashed buffer >> in omap-sham is unfortunately rather large. > > The allocation should usually be done from the request_alloc > function, i.e., you set the reqsize and the user does the allocation > for you. I need some clarification on this, afaik request_alloc related functionality only works per-request basis. The export / import functionality however is supposed to work across multiple requests. The test code for example does this: ret = crypto_ahash_export(req, state); ... ahash_request_free(req); req = ahash_request_alloc(tfm, GFP_KERNEL); ... ret = crypto_ahash_import(req, state); ... which means if I attempt to allocate extra space for the export buffer within the first request, it is not available at the import time anymore. Is there any limitation how many simultaneous exports can be done from a driver? I was wondering if I can allocate a single export buffer for the whole driver. -Tero -- 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
On Mon, Jul 04, 2016 at 12:17:02PM +0300, Tero Kristo wrote: > > I need some clarification on this, afaik request_alloc related > functionality only works per-request basis. The export / import > functionality however is supposed to work across multiple requests. > The test code for example does this: Why are you trying to allocate memory in export/import at all? The preferred approach is to unconditionally allocate the memory in ahash_request_alloc. Cheers,
On 04/07/16 12:19, Herbert Xu wrote: > On Mon, Jul 04, 2016 at 12:17:02PM +0300, Tero Kristo wrote: >> >> I need some clarification on this, afaik request_alloc related >> functionality only works per-request basis. The export / import >> functionality however is supposed to work across multiple requests. >> The test code for example does this: > > Why are you trying to allocate memory in export/import at all? > The preferred approach is to unconditionally allocate the memory > in ahash_request_alloc. The driver allocates a largish buffer for copying the input data from the sgs into a sequential space. If I don't save the contents of this buffer anywhere, the export/import doesn't work as expected it seems. Actually I am now wondering why the driver allocates this sequential buffer at all, DMA should be possible to use over the sgs just fine, at least some other crypto drivers are doing this. -Tero -- 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
On Mon, Jul 04, 2016 at 12:27:00PM +0300, Tero Kristo wrote: > > The driver allocates a largish buffer for copying the input data > from the sgs into a sequential space. If I don't save the contents > of this buffer anywhere, the export/import doesn't work as expected > it seems. > > Actually I am now wondering why the driver allocates this sequential > buffer at all, DMA should be possible to use over the sgs just fine, > at least some other crypto drivers are doing this. Indeed this is totally broken. No hash driver should be holding data without actually hashing it, unless of course if there is less than a block of data. So either fix the driver to hash the data as they come in, or make it use a software fallback for update. Cheers,
diff --git a/crypto/ahash.c b/crypto/ahash.c index 3887a98..375bbd7 100644 --- a/crypto/ahash.c +++ b/crypto/ahash.c @@ -536,7 +536,7 @@ static int ahash_prepare_alg(struct ahash_alg *alg) struct crypto_alg *base = &alg->halg.base; if (alg->halg.digestsize > PAGE_SIZE / 8 || - alg->halg.statesize > PAGE_SIZE / 8 || + alg->halg.statesize > PAGE_SIZE * 2 || alg->halg.statesize == 0) return -EINVAL;
The statesize is used to determine the maximum size for saved ahash context. In some cases, this can be much larger than what is currently allocated for it, for example omap-sham driver uses a buffer size of PAGE_SIZE. Increase the statesize to accommodate this. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- crypto/ahash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)