Re: [GIT] Networking

[Herbert Xu]

On Tue, Jul 11, 2017 at 01:31:14PM -0700, David Miller wrote:
>
> Acked-by: David S. Miller 
> 
> Looks good, is this going via my tree or your's?

I'll push it along.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [GIT] Networking

[David Miller]

From: Herbert Xu 
Date: Mon, 10 Jul 2017 22:00:48 +0800

> crypto: af_alg - Avoid sock_graft call warning
> 
> The newly added sock_graft warning triggers in af_alg_accept.
> It's harmless as we're essentially doing sock->sk = sock->sk.
> 
> The sock_graft call is actually redundant because all the work
> it does is subsumed by sock_init_data.  However, it was added
> to placate SELinux as it uses it to initialise its internal state.
> 
> This patch avoisd the warning by making the SELinux call directly.
> 
> Reported-by: Linus Torvalds 
> Signed-off-by: Herbert Xu 

Acked-by: David S. Miller 

Looks good, is this going via my tree or your's?

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Eric Biggers]

On Tue, Jul 11, 2017 at 11:53:11AM -0700, Dave Watson wrote:
> On 07/11/17 08:29 AM, Steffen Klassert wrote:
> > Sorry for replying to old mail...
> > > +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> > > +{
> > 
> > ...
> > 
> > > +
> > > + if (!sw_ctx->aead_send) {
> > > + sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> > > + if (IS_ERR(sw_ctx->aead_send)) {
> > > + rc = PTR_ERR(sw_ctx->aead_send);
> > > + sw_ctx->aead_send = NULL;
> > > + goto free_rec_seq;
> > > + }
> > > + }
> > > +
> > 
> > When I look on how you allocate the aead transformation, it seems
> > that you should either register an asynchronous callback with
> > aead_request_set_callback(), or request for a synchronous algorithm.
> > 
> > Otherwise you will crash on an asynchronous crypto return, no?
> 
> The intention is for it to be synchronous, and gather directly from
> userspace buffers.  It looks like calling
> crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC) is the correct way
> to request synchronous algorithms only?
> 

Yes, that means the CRYPTO_ALG_ASYNC bit is required to be 0, i.e. the algorithm
must be synchronous.  Currently it's requesting either a synchronous or
asynchronous algorithm, and it will crash if it gets an async one.

Also I think even with a synchronous algorithm, tls_do_encryption() still needs
to call aead_request_set_callback(), passing NULL for the callback and data, so
that the request flags are initialized.

Eric

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Dave Watson]

On 07/11/17 08:29 AM, Steffen Klassert wrote:
> Sorry for replying to old mail...
> > +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> > +{
> 
> ...
> 
> > +
> > +   if (!sw_ctx->aead_send) {
> > +   sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> > +   if (IS_ERR(sw_ctx->aead_send)) {
> > +   rc = PTR_ERR(sw_ctx->aead_send);
> > +   sw_ctx->aead_send = NULL;
> > +   goto free_rec_seq;
> > +   }
> > +   }
> > +
> 
> When I look on how you allocate the aead transformation, it seems
> that you should either register an asynchronous callback with
> aead_request_set_callback(), or request for a synchronous algorithm.
> 
> Otherwise you will crash on an asynchronous crypto return, no?

The intention is for it to be synchronous, and gather directly from
userspace buffers.  It looks like calling
crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC) is the correct way
to request synchronous algorithms only?

> Also, it seems that you have your scatterlists on a per crypto
> transformation base istead of per crypto request. Is this intentional?

We hold the socket lock and only one crypto op can happen at a time,
so we reuse the scatterlists.

Re: [PATCH 00/10] Fix alignment issues in staging/ccree

[Greg KH]

On Sun, Jul 02, 2017 at 01:25:45AM +0200, Simon Sandström wrote:
> Fixes a total of 195 alignment issues in staging/ccree reported by
> checkpatch.pl. Adds a few "line over 80 characters" warnings as a
> result of the realignments, but I could try to get rid of them in the
> same patchset if needed.

Not all of these applied, some did, if you could rebase the remaining
against my staging-testing branch at the moment, and resend, that would
be great.

thanks,

greg k-h

[PATCH] staging: ccree: move FIPS support to kernel infrastructure

[Gilad Ben-Yossef]

The ccree driver had its own FIPS support, complete with
a test harness comparable to crypto testmgr and an
implementation which disables crypto functionality on
FIPS test error detection, either in Linux or from TEE.

This patch removes the duplication, while reimplementing
the handling of TEE reported FIPS errors according to the
kernel policy of inducing a panic in such an event.

Signed-off-by: Gilad Ben-Yossef 
---

Note: this patch is based on top of Tyler Olivieri patch
entitled "staging: ccree: fix switch case indentation".

 drivers/staging/ccree/Kconfig   |9 -
 drivers/staging/ccree/Makefile  |2 +-
 drivers/staging/ccree/ssi_aead.c|6 -
 drivers/staging/ccree/ssi_cipher.c  |   30 +-
 drivers/staging/ccree/ssi_driver.c  |8 +-
 drivers/staging/ccree/ssi_driver.h  |1 -
 drivers/staging/ccree/ssi_fips.c|  121 ++-
 drivers/staging/ccree/ssi_fips.h|   58 +-
 drivers/staging/ccree/ssi_fips_data.h   |  306 --
 drivers/staging/ccree/ssi_fips_ext.c|   92 --
 drivers/staging/ccree/ssi_fips_ll.c | 1620 ---
 drivers/staging/ccree/ssi_fips_local.c  |  357 ---
 drivers/staging/ccree/ssi_fips_local.h  |   67 --
 drivers/staging/ccree/ssi_hash.c|   21 -
 drivers/staging/ccree/ssi_request_mgr.c |2 -
 15 files changed, 130 insertions(+), 2570 deletions(-)
 delete mode 100644 drivers/staging/ccree/ssi_fips_data.h
 delete mode 100644 drivers/staging/ccree/ssi_fips_ext.c
 delete mode 100644 drivers/staging/ccree/ssi_fips_ll.c
 delete mode 100644 drivers/staging/ccree/ssi_fips_local.c
 delete mode 100644 drivers/staging/ccree/ssi_fips_local.h

diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig
index 36a87c6..0b3092b 100644
--- a/drivers/staging/ccree/Kconfig
+++ b/drivers/staging/ccree/Kconfig
@@ -23,12 +23,3 @@ config CRYPTO_DEV_CCREE
  Choose this if you wish to use hardware acceleration of
  cryptographic operations on the system REE.
  If unsure say Y.
-
-config CCREE_FIPS_SUPPORT
-   bool "Turn on CryptoCell 7XX REE FIPS mode support"
-   depends on CRYPTO_DEV_CCREE
-   default n
-   help
- Say 'Y' to enable support for FIPS compliant mode by the
- CCREE driver.
- If unsure say N.
diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile
index 318c2b3..ae702f3 100644
--- a/drivers/staging/ccree/Makefile
+++ b/drivers/staging/ccree/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o
 ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o 
ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o
-ccree-$(CCREE_FIPS_SUPPORT) += ssi_fips.o ssi_fips_ll.o ssi_fips_ext.o 
ssi_fips_local.o
+ccree-$(CONFIG_CRYPTO_FIPS) += ssi_fips.o
diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c
index 1168161..3a10c31 100644
--- a/drivers/staging/ccree/ssi_aead.c
+++ b/drivers/staging/ccree/ssi_aead.c
@@ -36,7 +36,6 @@
 #include "ssi_hash.h"
 #include "ssi_sysfs.h"
 #include "ssi_sram_mgr.h"
-#include "ssi_fips_local.h"
 
 #define template_aead  template_u.aead
 
@@ -146,8 +145,6 @@ static int ssi_aead_init(struct crypto_aead *tfm)
container_of(alg, struct ssi_crypto_alg, aead_alg);
SSI_LOG_DEBUG("Initializing context @%p for %s\n", ctx, 
crypto_tfm_alg_name(&(tfm->base)));
 
-   CHECK_AND_RETURN_UPON_FIPS_ERROR();
-
/* Initialize modes in instance */
ctx->cipher_mode = ssi_alg->cipher_mode;
ctx->flow_mode = ssi_alg->flow_mode;
@@ -538,7 +535,6 @@ ssi_aead_setkey(struct crypto_aead *tfm, const u8 *key, 
unsigned int keylen)
SSI_LOG_DEBUG("Setting key in context @%p for %s. key=%p keylen=%u\n",
ctx, crypto_tfm_alg_name(crypto_aead_tfm(tfm)), key, keylen);
 
-   CHECK_AND_RETURN_UPON_FIPS_ERROR();
/* STAT_PHASE_0: Init and sanity checks */
 
if (ctx->auth_mode != DRV_HASH_NULL) { /* authenc() alg. */
@@ -654,7 +650,6 @@ static int ssi_aead_setauthsize(
 {
struct ssi_aead_ctx *ctx = crypto_aead_ctx(authenc);
 
-   CHECK_AND_RETURN_UPON_FIPS_ERROR();
/* Unsupported auth. sizes */
if ((authsize == 0) ||
(authsize > crypto_aead_maxauthsize(authenc))) {
@@ -1946,7 +1941,6 @@ static int ssi_aead_process(struct aead_request *req, 
enum drv_crypto_direction
SSI_LOG_DEBUG("%s context=%p req=%p iv=%p src=%p src_ofs=%d dst=%p 
dst_ofs=%d cryptolen=%d\n",
((direct == DRV_CRYPTO_DIRECTION_ENCRYPT) ? "Encrypt" : 
"Decrypt"), ctx, req, req->iv,
sg_virt(req->src), req->src->offset, sg_virt(req->dst), 
req->dst->offset, req->cryptlen);
-   CHECK_AND_RETURN_UPON_FIPS_ERROR();
 
/* STAT_PHASE_0: Init and sanity checks */
 
diff --git a/drivers/staging/ccree/ssi_cipher.c 
b/drivers/staging/ccree/ssi_cipher.c
index 4ef0c9b..af16bd0 100644

Re: [PATCH 1/2] staging: ccree: remove unnecessary cast on kmalloc

[Gilad Ben-Yossef]

On Sun, Jul 9, 2017 at 8:43 AM, Gustavo A. R. Silva
 wrote:
> The assignment operator implicitly converts a void pointer to the type of the
> pointer it is assigned to.
>
> This issue was detected using Coccinelle and the following semantic patch:
>
> @@
> expression * e;
> expression arg1, arg2;
> type T;
> @@
>
> - e=(T*)
> + e=
> kmalloc(arg1, arg2);
>
> Signed-off-by: Gustavo A. R. Silva 

For both patches:

Acked-by: Gilad Ben-Yossef 

> ---
>  drivers/staging/ccree/ssi_buffer_mgr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c 
> b/drivers/staging/ccree/ssi_buffer_mgr.c
> index b35871e..18a8694 100644
> --- a/drivers/staging/ccree/ssi_buffer_mgr.c
> +++ b/drivers/staging/ccree/ssi_buffer_mgr.c
> @@ -1725,8 +1725,7 @@ int ssi_buffer_mgr_init(struct ssi_drvdata *drvdata)
> struct buff_mgr_handle *buff_mgr_handle;
> struct device *dev = >plat_dev->dev;
>
> -   buff_mgr_handle = (struct buff_mgr_handle *)
> -   kmalloc(sizeof(struct buff_mgr_handle), GFP_KERNEL);
> +   buff_mgr_handle = kmalloc(sizeof(struct buff_mgr_handle), GFP_KERNEL);
> if (!buff_mgr_handle)
> return -ENOMEM;
>
> --
> 2.5.0
>

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 0/5] staging: ccree: fix checkpatch errors

[Gilad Ben-Yossef]

Tyler,


On Tue, Jul 11, 2017 at 4:38 PM, Gilad Ben-Yossef  wrote:
> On Mon, Jul 10, 2017 at 12:10 AM,   wrote:
>> From: Tyler Olivieri 
>>
>> This patchset fixes several checkpatch errors and warnings in /staging/ccree:

You've messed Greg's email address, so my ACK bounced.

The content is good, but I don't know if Greg saw the patch set.
You might want to resend it.

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 v2] staging: ccree: Use __func__ instead of function name

[Gilad Ben-Yossef]

Hello Karthik ,

Thank you for the patch.

On Thu, Jun 29, 2017 at 8:08 PM,   wrote:
> From: Karthik Tummala 
>
> Fixed following checkpatch.pl warning:
> WARNING: Prefer using '"%s...", __func__' to using
> the function's name, in a string
>
> It is prefered to use '%s & __func__' instead of function
> name for logging.
>
> Signed-off-by: Karthik Tummala 
> ---
> Changes for v2:
> v1 was a patch series, which consisted of two patches in which
> second one was already submitted by Gilad Ben-Yossef, so dropped
> that one.
>
> Patch generated on staging-testing as suggested by Greg-K H.
> ---
>  drivers/staging/ccree/ssi_aead.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_aead.c 
> b/drivers/staging/ccree/ssi_aead.c
> index 1fc0b05..1168161 100644
> --- a/drivers/staging/ccree/ssi_aead.c
> +++ b/drivers/staging/ccree/ssi_aead.c
> @@ -1886,7 +1886,7 @@ static int config_gcm_context(struct aead_request *req)
> (req->cryptlen - ctx->authsize);
> __be32 counter = cpu_to_be32(2);
>
> -   SSI_LOG_DEBUG("config_gcm_context() cryptlen = %d, req->assoclen = %d 
> ctx->authsize = %d\n", cryptlen, req->assoclen, ctx->authsize);
> +   SSI_LOG_DEBUG("%s() cryptlen = %d, req->assoclen = %d ctx->authsize = 
> %d\n", __func__, cryptlen, req->assoclen, ctx->authsize);
>
> memset(req_ctx->hkey, 0, AES_BLOCK_SIZE);
>
> @@ -2198,7 +2198,7 @@ static int ssi_rfc4106_gcm_setkey(struct crypto_aead 
> *tfm, const u8 *key, unsign
> struct ssi_aead_ctx *ctx = crypto_aead_ctx(tfm);
> int rc = 0;
>
> -   SSI_LOG_DEBUG("ssi_rfc4106_gcm_setkey()  keylen %d, key %p\n", 
> keylen, key);
> +   SSI_LOG_DEBUG("%s()  keylen %d, key %p\n", __func__, keylen, key);
>
> if (keylen < 4)
> return -EINVAL;
> @@ -2216,7 +2216,7 @@ static int ssi_rfc4543_gcm_setkey(struct crypto_aead 
> *tfm, const u8 *key, unsign
> struct ssi_aead_ctx *ctx = crypto_aead_ctx(tfm);
> int rc = 0;
>
> -   SSI_LOG_DEBUG("ssi_rfc4543_gcm_setkey()  keylen %d, key %p\n", 
> keylen, key);
> +   SSI_LOG_DEBUG("%s()  keylen %d, key %p\n", __func__, keylen, key);
>
> if (keylen < 4)
> return -EINVAL;
> --
> 1.9.1
>

Acked-by: Gilad Ben-Yossef 

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 0/5] staging: ccree: fix checkpatch errors

[Gilad Ben-Yossef]

On Mon, Jul 10, 2017 at 12:10 AM,   wrote:
> From: Tyler Olivieri 
>
> This patchset fixes several checkpatch errors and warnings in /staging/ccree:
>
> ERROR: that open brace { should be on the previous line
> ERROR: open brace '{' following function declarations go on the next line
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> ERROR: do not use assignment in if condition
> ERROR: switch and case should be at the same indent
> WARNING: Statements terminations use 1 semicolon
>
> This is also a submission for the eudyptula challenge.
>
> Tyler Olivieri (5):
>   staging: ccree: remove redudant semicolons
>   staging: ccree: fix placement of curly braces
>   staging: ccree: remove assignement in conditional
>   staging: ccree: export symbol immediately following function
>   staging: ccree: fix switch case indentation
>
>  drivers/staging/ccree/ssi_buffer_mgr.c | 14 ++
>  drivers/staging/ccree/ssi_cipher.c |  6 ++-
>  drivers/staging/ccree/ssi_driver.c |  5 +-
>  drivers/staging/ccree/ssi_fips.c   |  2 -
>  drivers/staging/ccree/ssi_fips_ll.c| 85 
> +++---
>  drivers/staging/ccree/ssi_hash.c   | 33 +++--
>  drivers/staging/ccree/ssi_sysfs.c  |  3 +-
>  7 files changed, 57 insertions(+), 91 deletions(-)
>
> --
> 2.9.4
>


Looks good to me.

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] Stagingdriver cctree: Fix for checkpatch warning

[Gilad Ben-Yossef]

Hello Philip,

Thank your patch.

Your patch subject line is not descriptive and not formatted well.

A better subject would be something like:

staging: ccree: move comment to fit coding style

Thanks,
Gilad

On Fri, Jun 30, 2017 at 7:32 AM,   wrote:
> From: Bincy K Philip 
>
> Trivial fix for Line over 80 characters
>
> Moved the comment to top of the definition
>
> Signed-off-by: Bincy K Philip 
> ---
>  drivers/staging/ccree/cc_hw_queue_defs.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/ccree/cc_hw_queue_defs.h 
> b/drivers/staging/ccree/cc_hw_queue_defs.h
> index aaa56c8..a18e6c9 100644
> --- a/drivers/staging/ccree/cc_hw_queue_defs.h
> +++ b/drivers/staging/ccree/cc_hw_queue_defs.h
> @@ -27,7 +27,8 @@
>  
> **/
>
>  #define HW_DESC_SIZE_WORDS 6
> -#define HW_QUEUE_SLOTS_MAX  15 /* Max. available slots in HW 
> queue */
> +/* Define max. available slots in HW queue */
> +#define HW_QUEUE_SLOTS_MAX  15
>
>  #define CC_REG_NAME(word, name) DX_DSCRPTR_QUEUE_WORD ## word ## _ ## name
>
> --
> 1.8.3.1
>



-- 
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 00/10] Fix alignment issues in staging/ccree

[Gilad Ben-Yossef]

On Mon, Jul 3, 2017 at 3:28 PM, Simon Sandström  wrote:
> On Mon, Jul 03, 2017 at 10:19:31AM +0300, Gilad Ben-Yossef wrote:
>> but for the few cases where its a complex expression that can be
>> broken down like this one:
>>
>> WARNING: line over 80 characters
>> #93: FILE: drivers/staging/ccree/ssi_buffer_mgr.c:437:
>> + (AES_BLOCK_SIZE + areq_ctx->ccm_hdr_size),
>>
>> It would be great if you fix those.
>
> Do you mean to fix them by just breaking the line after the ternary
> operator? Is that OK according to the code style rules?

Yes, it is.

>
> If not, then the two warnings in ssi_aead_handle_config_buf can be
> fixed by simply having a local variable:
>
> const unsigned int buflen = AES_BLOCK_SIZE + areq_ctx->ccm_hdr_size;
>
> but I'm not sure if the same can be done in
> ssi_buffer_mgr_map_hash_request_update for (update_data_len - *curr_buf_cnt)
> as it's not always the case that update_data_len is greater than
> *curr_buf_cnt. Even if it's safe to always calculate it I'm not
> entierly sure what to call the variable.

No need not. I simply meant to break this:

 (update_data_len - *curr_buf_cnt)

To this:

 (update_data_len -
   *curr_buf_cnt)

Assuming it made sense.

> Other then those two functions I don't think that there are any more
> lines that can be fixed without renaming functions / variables or
> by rewriting them to decrease the indentation depth, as you wrote.

No, not in this patch.

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: HELP writing crypto driver for HW with blocksize hash

[Gilad Ben-Yossef]

On Tue, Jul 11, 2017 at 1:50 PM, Kamil Konieczny
 wrote:
> On 11.07.2017 12:30, Gilad Ben-Yossef wrote:
>> On Tue, Jul 11, 2017 at 10:52 AM, Kamil Konieczny
>>  wrote:
>>>
>>>
>>> I am writing crypto driver for hash MD5/SHA1/SHA256 on Exynos 4412,
>>> and I am facing some (minor?) difficulties. [...]
>
>>> So there is no [...] final method.
>>>
>>> It must be feeded with at least one message byte to produce hash.
>>>
>>> One more constrain is to process data in constant-sized chunks,
>>> here it is 64 bytes, the same as for AES block,
>>> i.e. it cannot stop and export state while in middle of block,
>>> example - if we feed 16 bytes, we must then feed 48 bytes
>>> to stop, but ideally we should feed it always 64 bytes. [...]
>>>
>>> Any suggestions ?
>>>
>>> Can i keep some bytes unfeeded from ahash_request
>>> and return -EINPROGRESS ?
>>> Should i set timer and copy rest bytes after some timeout,
>>> where no more requests are incoming ? Or not ? cause it is
>>> async mode ?
>>> can i wait for more requests for processing waiting one ?
>> Your two constraints are actually inter-related -
>>
>> If you can only feed the HW a constant size chunk, than indeed need to
>> keey bytes fed
>> to the driver the are below the chunk size in a software buffer, but
>> than you need the final()
>> method to feed these bytes (padded as needed) to the HW if they are
>> the last bytes
>>
>> Gilad
>
> HW will done padding
>
> I want to avoid memcpy iff possible for async hash

Why? you're talking about a copy of a single cache line at most.
That hardly seem worth the trouble.

Gilad

>
> --
> Best regards,
> Kamil Konieczny
> Samsung R Institute Poland



-- 
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: HELP writing crypto driver for HW with blocksize hash

[Kamil Konieczny]

On 11.07.2017 12:30, Gilad Ben-Yossef wrote:
> On Tue, Jul 11, 2017 at 10:52 AM, Kamil Konieczny
>  wrote:
>>
>>
>> I am writing crypto driver for hash MD5/SHA1/SHA256 on Exynos 4412,
>> and I am facing some (minor?) difficulties. [...]

>> So there is no [...] final method.
>>
>> It must be feeded with at least one message byte to produce hash.
>>
>> One more constrain is to process data in constant-sized chunks,
>> here it is 64 bytes, the same as for AES block,
>> i.e. it cannot stop and export state while in middle of block,
>> example - if we feed 16 bytes, we must then feed 48 bytes
>> to stop, but ideally we should feed it always 64 bytes. [...]
>>
>> Any suggestions ?
>>
>> Can i keep some bytes unfeeded from ahash_request
>> and return -EINPROGRESS ?
>> Should i set timer and copy rest bytes after some timeout,
>> where no more requests are incoming ? Or not ? cause it is
>> async mode ?
>> can i wait for more requests for processing waiting one ?
> Your two constraints are actually inter-related -
> 
> If you can only feed the HW a constant size chunk, than indeed need to
> keey bytes fed
> to the driver the are below the chunk size in a software buffer, but
> than you need the final()
> method to feed these bytes (padded as needed) to the HW if they are
> the last bytes
> 
> Gilad

HW will done padding

I want to avoid memcpy iff possible for async hash

-- 
Best regards,
Kamil Konieczny
Samsung R Institute Poland

Re: HELP writing crypto driver for HW with blocksize hash

[Gilad Ben-Yossef]

On Tue, Jul 11, 2017 at 10:52 AM, Kamil Konieczny
 wrote:
>
> Hi,
>
> I am writing crypto driver for hash MD5/SHA1/SHA256 on Exynos 4412,
> and I am facing some (minor?) difficulties.
>
> In old days, hadware (HW) can only do basic hash block operation,
> so at the end it needed to finalize hash, and driver need to
> write some bits into buffer to get message hash. Time passes,
> and hardware (HW) was designed with improved cappabilities,
> so it can add itself bits after message and calculate hash,
> it can stop then export state, import and resume,
> but ... it can not process null-length (or zero-length) ending.
> So there is no more final method.
>
> It must be feeded with at least one message byte to produce hash.
>
> One more constrain is to process data in constant-sized chunks,
> here it is 64 bytes, the same as for AES block,
> i.e. it cannot stop and export state while in middle of block,
> example - if we feed 16 bytes, we must then feed 48 bytes
> to stop, but ideally we should feed it always 64 bytes.


>
> Some crypto drivers with similar problem(s):
>
> omap-sham.c - no final and blocksize needed,
> broadcom bcm/spu.c - no final,
> ccp/ccp-crypto-sha.c - no final,
> nx/nx-sha256.c - blocksize needed,
>
> One more thing - in algorithm description for methods:
> final, finup, update, digest, export, import
> there is note that finup is for those hardware
> that cannot do final, but again,
>
> it looks like crypto framework is ignoring that and every finup
> is translated into "update, final".
>
> HW driver will do opposite - it will translate final into finup.
>
> From this follows that for every such HW crypto drivers authors
> duplicate code for keeping at least blocksize cache of message.
>
> One more point - use of block size in algo structure.
> It is for informing framework about HW limitation,
> but seems to be ignored again...
>
> Any suggestions ?
>
> Can i keep some bytes unfeeded from ahash_request
> and return -EINPROGRESS ?
> Should i set timer and copy rest bytes after some timeout,
> where no more requests are incoming ? Or not ? cause it is
> async mode ?
> can i wait for more requests for processing waiting one ?
>

Your two constraints are actually inter-related -

If you can only feed the HW a constant size chunk, than indeed need to
keey bytes fed
to the driver the are below the chunk size in a software buffer, but
than you need the final()
method to feed these bytes (padded as needed) to the HW if they are
the last bytes

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

[PATCH] crypto: brcm - remove BCM_PDC_MBOX dependency in Kconfig

[Raveendra Padasalagi]

SPU driver is dependent on generic MAILBOX API's to
communicate with underlying DMA engine driver.

So this patch removes BCM_PDC_MBOX "depends on" for SPU driver
in Kconfig and adds MAILBOX as dependent module.

Fixes: 9d12ba86f818 ("crypto: brcm - Add Broadcom SPU driver")
Signed-off-by: Raveendra Padasalagi 
Reviewed-by: Ray Jui 
Reviewed-by: Scott Branden 
Cc: sta...@vger.kernel.org
---
 drivers/crypto/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index fb1e60f..778fc1b 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -629,7 +629,7 @@ source "drivers/crypto/virtio/Kconfig"
 config CRYPTO_DEV_BCM_SPU
tristate "Broadcom symmetric crypto/hash acceleration support"
depends on ARCH_BCM_IPROC
-   depends on BCM_PDC_MBOX
+   depends on MAILBOX
default m
select CRYPTO_DES
select CRYPTO_MD5
-- 
1.9.1

HELP writing crypto driver for HW with blocksize hash

[Kamil Konieczny]

Hi,

I am writing crypto driver for hash MD5/SHA1/SHA256 on Exynos 4412,
and I am facing some (minor?) difficulties.

In old days, hadware (HW) can only do basic hash block operation,
so at the end it needed to finalize hash, and driver need to
write some bits into buffer to get message hash. Time passes,
and hardware (HW) was designed with improved cappabilities,
so it can add itself bits after message and calculate hash,
it can stop then export state, import and resume,
but ... it can not process null-length (or zero-length) ending.
So there is no more final method.

It must be feeded with at least one message byte to produce hash.

One more constrain is to process data in constant-sized chunks,
here it is 64 bytes, the same as for AES block,
i.e. it cannot stop and export state while in middle of block,
example - if we feed 16 bytes, we must then feed 48 bytes
to stop, but ideally we should feed it always 64 bytes.

Some crypto drivers with similar problem(s):

omap-sham.c - no final and blocksize needed,
broadcom bcm/spu.c - no final,
ccp/ccp-crypto-sha.c - no final,
nx/nx-sha256.c - blocksize needed,

One more thing - in algorithm description for methods:
final, finup, update, digest, export, import
there is note that finup is for those hardware 
that cannot do final, but again,

it looks like crypto framework is ignoring that and every finup
is translated into "update, final".

HW driver will do opposite - it will translate final into finup.

>From this follows that for every such HW crypto drivers authors
duplicate code for keeping at least blocksize cache of message.

One more point - use of block size in algo structure.
It is for informing framework about HW limitation, 
but seems to be ignored again...

Any suggestions ?

Can i keep some bytes unfeeded from ahash_request 
and return -EINPROGRESS ?
Should i set timer and copy rest bytes after some timeout,
where no more requests are incoming ? Or not ? cause it is 
async mode ? 
can i wait for more requests for processing waiting one ?

-- 
Best regards,
Kamil Konieczny
Samsung R Institute Poland

[PATCH 2/2] crypto: hwrng remember rng chosen by user

[Harald Freudenberger]

When a user chooses a rng source via sysfs attribute
this rng should be sticky, even when other sources
with better quality to register. This patch introduces
a simple way to remember the user's choice. This is
reflected by a new sysfs attribute file 'rng_selected'
which shows if the current rng has been chosen by
userspace. The new attribute file shows '1' for user
selected rng and '0' otherwise.

Signed-off-by: Harald Freudenberger 
Reviewed-by: PrasannaKumar Muralidharan 
---
 drivers/char/hw_random/core.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index e9dda16..9701ac7 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,6 +28,8 @@
 #define RNG_MODULE_NAME"hw_random"
 
 static struct hwrng *current_rng;
+/* the current rng has been explicitly chosen by user via sysfs */
+static int cur_rng_set_by_user;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
list_for_each_entry(rng, _list, list) {
if (sysfs_streq(rng->name, buf)) {
err = 0;
+   cur_rng_set_by_user = 1;
if (rng != current_rng)
err = set_current_rng(rng);
break;
@@ -352,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device 
*dev,
return strlen(buf);
 }
 
+static ssize_t hwrng_attr_selected_show(struct device *dev,
+   struct device_attribute *attr,
+   char *buf)
+{
+   return snprintf(buf, PAGE_SIZE, "%d\n", cur_rng_set_by_user);
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
   hwrng_attr_current_show,
   hwrng_attr_current_store);
 static DEVICE_ATTR(rng_available, S_IRUGO,
   hwrng_attr_available_show,
   NULL);
+static DEVICE_ATTR(rng_selected, S_IRUGO,
+  hwrng_attr_selected_show,
+  NULL);
 
 static struct attribute *rng_dev_attrs[] = {
_attr_rng_current.attr,
_attr_rng_available.attr,
+   _attr_rng_selected.attr,
NULL
 };
 
@@ -444,10 +458,12 @@ int hwrng_register(struct hwrng *rng)
 
old_rng = current_rng;
err = 0;
-   if (!old_rng || (rng->quality > old_rng->quality)) {
+   if (!old_rng ||
+   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
/*
 * Set new rng as current as the new rng source
-* provides better entropy quality.
+* provides better entropy quality and was not
+* chosen by userspace.
 */
err = set_current_rng(rng);
if (err)
@@ -479,6 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   cur_rng_set_by_user = 0;
/* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
struct hwrng *new_rng;
-- 
2.7.4

[PATCH 1/2] crypto: hwrng use rng source with best quality

[Harald Freudenberger]

This patch rewoks the hwrng to always use the
rng source with best entropy quality.

On registation and unregistration the hwrng now
tries to choose the best (= highest quality value)
rng source. The handling of the internal list
of registered rng sources is now always sorted
by quality and the top most rng chosen.

Signed-off-by: Harald Freudenberger 
Reviewed-by: PrasannaKumar Muralidharan 
---
 drivers/char/hw_random/core.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..e9dda16 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -29,6 +29,7 @@
 
 static struct hwrng *current_rng;
 static struct task_struct *hwrng_fill;
+/* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
 /* Protects rng_list and current_rng */
 static DEFINE_MUTEX(rng_mutex);
@@ -417,6 +418,7 @@ int hwrng_register(struct hwrng *rng)
 {
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
+   struct list_head *rng_list_ptr;
 
if (!rng->name || (!rng->data_read && !rng->read))
goto out;
@@ -432,14 +434,25 @@ int hwrng_register(struct hwrng *rng)
init_completion(>cleanup_done);
complete(>cleanup_done);
 
+   /* rng_list is sorted by decreasing quality */
+   list_for_each(rng_list_ptr, _list) {
+   tmp = list_entry(rng_list_ptr, struct hwrng, list);
+   if (tmp->quality < rng->quality)
+   break;
+   }
+   list_add_tail(>list, rng_list_ptr);
+
old_rng = current_rng;
err = 0;
-   if (!old_rng) {
+   if (!old_rng || (rng->quality > old_rng->quality)) {
+   /*
+* Set new rng as current as the new rng source
+* provides better entropy quality.
+*/
err = set_current_rng(rng);
if (err)
goto out_unlock;
}
-   list_add_tail(>list, _list);
 
if (old_rng && !rng->init) {
/*
@@ -466,12 +479,12 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   /* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
-   struct hwrng *tail;
-
-   tail = list_entry(rng_list.prev, struct hwrng, list);
+   struct hwrng *new_rng;
 
-   set_current_rng(tail);
+   new_rng = list_entry(rng_list.next, struct hwrng, list);
+   set_current_rng(new_rng);
}
}
 
-- 
2.7.4

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Steffen Klassert]

Sorry for replying to old mail...

On Wed, Jun 14, 2017 at 11:37:39AM -0700, Dave Watson wrote:
> +static int tls_do_encryption(struct tls_context *tls_ctx,
> +  struct tls_sw_context *ctx, size_t data_len,
> +  gfp_t flags)
> +{
> + unsigned int req_size = sizeof(struct aead_request) +
> + crypto_aead_reqsize(ctx->aead_send);
> + struct aead_request *aead_req;
> + int rc;
> +
> + aead_req = kmalloc(req_size, flags);
> + if (!aead_req)
> + return -ENOMEM;
> +
> + ctx->sg_encrypted_data[0].offset += tls_ctx->prepend_size;
> + ctx->sg_encrypted_data[0].length -= tls_ctx->prepend_size;
> +
> + aead_request_set_tfm(aead_req, ctx->aead_send);
> + aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
> + aead_request_set_crypt(aead_req, ctx->sg_aead_in, ctx->sg_aead_out,
> +data_len, tls_ctx->iv);
> + rc = crypto_aead_encrypt(aead_req);
> +
> + ctx->sg_encrypted_data[0].offset -= tls_ctx->prepend_size;
> + ctx->sg_encrypted_data[0].length += tls_ctx->prepend_size;
> +
> + kfree(aead_req);
> + return rc;
> +}

...

> +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> +{

...

> +
> + if (!sw_ctx->aead_send) {
> + sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> + if (IS_ERR(sw_ctx->aead_send)) {
> + rc = PTR_ERR(sw_ctx->aead_send);
> + sw_ctx->aead_send = NULL;
> + goto free_rec_seq;
> + }
> + }
> +

When I look on how you allocate the aead transformation, it seems
that you should either register an asynchronous callback with
aead_request_set_callback(), or request for a synchronous algorithm.

Otherwise you will crash on an asynchronous crypto return, no?

Also, it seems that you have your scatterlists on a per crypto
transformation base istead of per crypto request. Is this intentional?

[PATCH] crypto: caam - free qman_fq after kill_fq

[Xulin Sun]

kill_fq removes a complete frame queue, it needs to free the qman_fq
in the last. Else kmemleak will report the below warning:

unreferenced object 0x800073085c80 (size 128):
  comm "cryptomgr_test", pid 199, jiffies 4294937850 (age 67.840s)
  hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 a0 80 7e 00 00 80 ff ff
00 00 00 00 00 00 00 00 04 00 04 00 5c 01 00 00
  backtrace:
[] create_object+0xf8/0x258
[] kmemleak_alloc+0x58/0xa0
[] kmem_cache_alloc_trace+0x2c8/0x358
[] create_caam_req_fq+0x40/0x170
[] caam_drv_ctx_update+0x54/0x248
[] aead_setkey+0x154/0x300
[] setkey+0x50/0xf0
[] __test_aead+0x5ec/0x1028
[] test_aead+0x44/0xc8
[] alg_test_aead+0x58/0xd0
[] alg_test+0x14c/0x308
[] cryptomgr_test+0x50/0x58
[] kthread+0xdc/0xf0
[] ret_from_fork+0x10/0x50

And check where the function kill_fq() is called to remove
the additional kfree to qman_fq.

Signed-off-by: Xulin Sun 
---
 drivers/crypto/caam/qi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c
index 1990ed4..c4b9173 100644
--- a/drivers/crypto/caam/qi.c
+++ b/drivers/crypto/caam/qi.c
@@ -277,6 +277,7 @@ static int kill_fq(struct device *qidev, struct qman_fq *fq)
dev_err(qidev, "OOS of FQID: %u failed\n", fq->fqid);
 
qman_destroy_fq(fq);
+   kfree(fq);
 
return ret;
 }
@@ -511,7 +512,6 @@ int caam_qi_shutdown(struct device *qidev)
 
if (kill_fq(qidev, per_cpu(pcpu_qipriv.rsp_fq, i)))
dev_err(qidev, "Rsp FQ kill failed, cpu: %d\n", i);
-   kfree(per_cpu(pcpu_qipriv.rsp_fq, i));
}
 
/*
-- 
2.7.4