Re: [PATCH][next] staging: ccree: fix memory leaks in cc_ivgen_init

2018-01-21 Thread Gilad Ben-Yossef 
On Fri, Jan 12, 2018 at 6:10 PM, Colin King  wrote:
> From: Colin Ian King 
>
> The current error exit path in function cc_ivgen_init via label
> 'out' free's resources from the drvdata->ivgen_handle context.
> However, drvdata->ivgen_handle has not been assigned to the
> context ivgen_ctx at this point, so the resources are not freed.
> Fix this by setting drvdata->ivgen_handle to ivgen_ctx as early
> as possible so that the clean up error exit return path can free
> the resources.
>
> Detected by CoveritScan, CID#1463795 ("Resource leak")
>
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/ccree/cc_ivgen.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/ccree/cc_ivgen.c 
> b/drivers/staging/ccree/cc_ivgen.c
> index 25a3131a93ce..c47f419b277b 100644
> --- a/drivers/staging/ccree/cc_ivgen.c
> +++ b/drivers/staging/ccree/cc_ivgen.c
> @@ -178,6 +178,8 @@ int cc_ivgen_init(struct cc_drvdata *drvdata)
> if (!ivgen_ctx)
> return -ENOMEM;
>
> +   drvdata->ivgen_handle = ivgen_ctx;
> +
> /* Allocate pool's header for initial enc. key/IV */
> ivgen_ctx->pool_meta = dma_alloc_coherent(device, CC_IVPOOL_META_SIZE,
>   _ctx->pool_meta_dma,
> @@ -196,8 +198,6 @@ int cc_ivgen_init(struct cc_drvdata *drvdata)
> goto out;
> }
>
> -   drvdata->ivgen_handle = ivgen_ctx;
> -
> return cc_init_iv_sram(drvdata);
>
>  out:
> --
> 2.15.1
>

Good catch!

Acked-by: Gilad Ben-Yossef 

Thanks,
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/7] crypto: ccree: add ablkcipher support

2018-01-21 Thread Gilad Ben-Yossef 
Hi Corentin,


On Thu, Jan 11, 2018 at 12:01 PM, Corentin Labbe
 wrote:
> On Thu, Jan 11, 2018 at 09:17:10AM +, Gilad Ben-Yossef wrote:
>> Add CryptoCell ablkcipher support
>>
>
> Hello
>
> I have some minor comments:
>
> ablkcipher is deprecated, so you need to use skcipher instead.

Fixed in v2.

>
>> Signed-off-by: Gilad Ben-Yossef 
>> ---
>>  drivers/crypto/ccree/Makefile|2 +-
>>  drivers/crypto/ccree/cc_buffer_mgr.c |  125 
>>  drivers/crypto/ccree/cc_buffer_mgr.h |   10 +
>>  drivers/crypto/ccree/cc_cipher.c | 1167 
>> ++
>>  drivers/crypto/ccree/cc_cipher.h |   74 +++
>>  drivers/crypto/ccree/cc_driver.c |   11 +
>>  drivers/crypto/ccree/cc_driver.h |2 +
>>  7 files changed, 1390 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/crypto/ccree/cc_cipher.c
>>  create mode 100644 drivers/crypto/ccree/cc_cipher.h
>>
> [...]
>> +
>> +struct tdes_keys {
>> + u8  key1[DES_KEY_SIZE];
>> + u8  key2[DES_KEY_SIZE];
>> + u8  key3[DES_KEY_SIZE];
>> +};
>> +
>> +static const u8 zero_buff[] = {  0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
>> + 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
>> + 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
>> + 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0};
>> +
>
> This constant is used nowhere.

Fixed in v2.

>
>> +/* The function verifies that tdes keys are not weak.*/
>> +static int cc_verify_3des_keys(const u8 *key, unsigned int keylen)
>> +{
>> + struct tdes_keys *tdes_key = (struct tdes_keys *)key;
>> +
>> + /* verify key1 != key2 and key3 != key2*/
>> + if ((memcmp((u8 *)tdes_key->key1, (u8 *)tdes_key->key2,
>> + sizeof(tdes_key->key1)) == 0) ||
>> + (memcmp((u8 *)tdes_key->key3, (u8 *)tdes_key->key2,
>> + sizeof(tdes_key->key3)) == 0)) {
>> + return -ENOEXEC;
>> + }
>> +
>> + return 0;
>> +}
>
> All driver testing 3des key also use des_ekey()

Well, the weak key test which is part of des_ekey() are not needed
AFAIK for 3des
as per RFC2451 and the HW does 3des key expansion so that function is not useful
here.

>
> [...]
>> +static void cc_cipher_complete(struct device *dev, void *cc_req, int err)
>> +{
>> + struct ablkcipher_request *areq = (struct ablkcipher_request *)cc_req;
>> + struct scatterlist *dst = areq->dst;
>> + struct scatterlist *src = areq->src;
>> + struct blkcipher_req_ctx *req_ctx = ablkcipher_request_ctx(areq);
>> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
>> + unsigned int ivsize = crypto_ablkcipher_ivsize(tfm);
>> + struct ablkcipher_request *req = (struct ablkcipher_request *)areq;
>> +
>> + cc_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst);
>> + kfree(req_ctx->iv);
>
> kzfree for all stuff with IV/key

Fixed in v2.
>
> [...]
>> +
>> +#ifdef CRYPTO_TFM_REQ_HW_KEY
>> +
>> +static inline bool cc_is_hw_key(struct crypto_tfm *tfm)
>> +{
>> + return (crypto_tfm_get_flags(tfm) & CRYPTO_TFM_REQ_HW_KEY);
>> +}
>> +
>> +#else
>> +
>> +struct arm_hw_key_info {
>> + int hw_key1;
>> + int hw_key2;
>> +};
>> +
>> +static inline bool cc_is_hw_key(struct crypto_tfm *tfm)
>> +{
>> + return false;
>> +}
>> +
>> +#endif /* CRYPTO_TFM_REQ_HW_KEY */
>
> I see nowhere any use/documentation of CRYPTO_TFM_REQ_HW_KEY, so a cleaning 
> could be done

You are right. It's a badly implemented stub function. I'll drop the
ifdef as it is useless right now.

Many thanks for the review!

Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 3/7] crypto: ccree: add ablkcipher support

2018-01-21 Thread Gilad Ben-Yossef 
On Thu, Jan 11, 2018 at 12:03 PM, Stephan Mueller  wrote:
> Am Donnerstag, 11. Januar 2018, 10:17:10 CET schrieb Gilad Ben-Yossef:
>
> Hi Gilad,
>
>> + // verify weak keys
>> + if (ctx_p->flow_mode == S_DIN_to_DES) {
>> + if (!des_ekey(tmp, key) &&
>> + (crypto_tfm_get_flags(tfm) & CRYPTO_TFM_REQ_WEAK_KEY)) {
>> + tfm->crt_flags |= CRYPTO_TFM_RES_WEAK_KEY;
>> + dev_dbg(dev, "weak DES key");
>> + return -EINVAL;
>> + }
>> + }
>> + if (ctx_p->cipher_mode == DRV_CIPHER_XTS &&
>> + xts_check_key(tfm, key, keylen)) {
>> + dev_dbg(dev, "weak XTS key");
>> + return -EINVAL;
>> + }
>> + if (ctx_p->flow_mode == S_DIN_to_DES &&
>> + keylen == DES3_EDE_KEY_SIZE &&
>> + cc_verify_3des_keys(key, keylen)) {
>> + dev_dbg(dev, "weak 3DES key");
>> + return -EINVAL;
>> + }
>
> For the DES key, wouldn't it make sense to use __des3_ede_setkey?
>
> Note, I would plan to release a patch for review to change that function to
> disallow key1 == key2 or key1 == key3 or key2 == key3 in FIPS mode.

I took your advise and did just that in v2.

Thanks for the review!

Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [RFC crypto v3 0/9] Chelsio Inline TLS

2018-01-21 Thread Sabrina Dubroca 
2017-12-20, 17:03:02 +0530, Atul Gupta wrote:
> RFC series for Chelsio Inline TLS driver (chtls.ko)
> 
> Driver use the ULP infrastructure to register chtls as Inline TLS ULP.

I don't think drivers should be registering their own ULP. TLS
offloading should be transparent to userspace, whatever HW ends up
being used. If each driver implements its own ULP, the application has
to be aware of what HW and what driver it's running on.

I think this offload should rely on a generic infrastructure, not
build its own private interface. Look at the current kTLS code, the
proposal for an offload infrastructure [0] from Mellanox, and see how
you can fit your driver into that, and extend what's missing.

[0] https://patchwork.ozlabs.org/patch/849984/


[...]
> Atul Gupta (9):
>   chtls: structure and macro definiton
>   cxgb4: Inline TLS FW Interface
>   cxgb4: LLD driver changes to enable TLS
>   chcr: Key Macro
>   chtls: Key program
>   chtls: CPL handler definition
>   chtls: Inline crypto request Tx/Rx
>   chtls: Register the ULP
>   Makefile Kconfig

That patchset is split so that each patch touches a separate set of
files, and the description of the contents of each patch is very
limited.  Can you try to group your changes by feature instead?  That
should help you come up with descriptive commit messages as well.


Thanks,

-- 
Sabrina

Re: Getting the ccree driver out of staging

2018-01-21 Thread Gilad Ben-Yossef 
On Thu, Jan 11, 2018 at 10:42 AM, Dan Carpenter
 wrote:
> Here are my remaining Smatch warnings:
>
> drivers/staging/ccree/cc_driver.c:219 init_cc_resources()
> error: '%pa' can only be followed by one of [dp]
>
> drivers/staging/ccree/cc_driver.c
>217
>218  if (rc) {
>219  dev_err(dev, "Failed in dma_set_mask, mask=%par\n",
>   ^
> This 'r' is is treated as a 'p'.  Not sure what was intended.
>
>220  _mask);
>221  return rc;
>222  }
>223
>
>
> drivers/staging/ccree/cc_buffer_mgr.c:1067 cc_aead_chain_data()
> warn: inconsistent indenting
>
> drivers/staging/ccree/cc_buffer_mgr.c
>   1064  if (src_mapped_nents > LLI_MAX_NUM_OF_DATA_ENTRIES) {
>   1065  dev_err(dev, "Too many fragments. current %d max 
> %d\n",
>   1066  src_mapped_nents, 
> LLI_MAX_NUM_OF_DATA_ENTRIES);
>   1067  return -ENOMEM;
> ^^
>   1068  }
>
> drivers/staging/ccree/cc_cipher.c:373 cc_cipher_setkey()
> warn: inconsistent indenting
>
> drivers/staging/ccree/cc_cipher.c
>369  dma_sync_single_for_device(dev, ctx_p->user.key_dma_addr,
>370 max_key_buf_size, DMA_TO_DEVICE);
>371  ctx_p->keylen = keylen;
>372
>373   dev_dbg(dev, "return safely");
> ^
> One extra space.
>
>374  return 0;
>375  }
>

Will fix in v2.

Thanks!
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [RFT PATCH] crypto: arm64 - implement SM3 secure hash using special instructions

2018-01-21 Thread Gilad Ben-Yossef 
On Tue, Jan 16, 2018 at 10:29 AM, Ard Biesheuvel
 wrote:
> Implement the Chinese SM3 secure hash algorithm using the new
> special instructions that have been introduced as an optional
> extension in ARMv8.2.
>
> Signed-off-by: Ard Biesheuvel 
> ---

Looks good to me.

Reviewed-by: Gilad Ben-Yossef 


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH] crypto: AF_ALG - inline IV support

2018-01-21 Thread Stephan Müller 
Hi Herbert,

I tried to summarize the use cases of the AIO support at [1].

The use case covering the inline IV support is documented in section [2]. It 
naturally would depend on this patch to be accepted. What is your take on this 
use case?

What is your take on the issue outlined at [3]? Should the affected drivers be 
updated?

Thanks
Stephan

[1] https://github.com/smuellerDD/libkcapi/commit/
29132075b8f045f3f92367c0190add81ccf5da11

[2] https://github.com/smuellerDD/libkcapi/commit/
29132075b8f045f3f92367c0190add81ccf5da11#diff-
dce7a00cbc610df94f0dc27eb769d01dR644

[3] https://github.com/smuellerDD/libkcapi/commit/
29132075b8f045f3f92367c0190add81ccf5da11#diff-
dce7a00cbc610df94f0dc27eb769d01dR575