[PATCH] crypto: qat - remove redundant arbiter configuration

2016-03-08 Thread Tadeusz Struk
The default arbiter configuration for ring weights and response ordering
is exactly what we want so we don't need to configure anything more.
This will also fix the problem where number of bundles is different
between different devices.

Reported-by: Ahsan Atta 
Signed-off-by: Tadeusz Struk 
---
 drivers/crypto/qat/qat_common/adf_hw_arbiter.c | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_hw_arbiter.c 
b/drivers/crypto/qat/qat_common/adf_hw_arbiter.c
index f267d9e..d7dd18d 100644
--- a/drivers/crypto/qat/qat_common/adf_hw_arbiter.c
+++ b/drivers/crypto/qat/qat_common/adf_hw_arbiter.c
@@ -49,7 +49,6 @@
 #include "adf_transport_internal.h"
 
 #define ADF_ARB_NUM 4
-#define ADF_ARB_REQ_RING_NUM 8
 #define ADF_ARB_REG_SIZE 0x4
 #define ADF_ARB_WTR_SIZE 0x20
 #define ADF_ARB_OFFSET 0x3
@@ -64,15 +63,6 @@
ADF_CSR_WR(csr_addr, ADF_ARB_RINGSRVARBEN_OFFSET + \
(ADF_ARB_REG_SLOT * index), value)
 
-#define WRITE_CSR_ARB_RESPORDERING(csr_addr, index, value) \
-   ADF_CSR_WR(csr_addr, (ADF_ARB_OFFSET + \
-   ADF_ARB_RO_EN_OFFSET) + (ADF_ARB_REG_SIZE * index), value)
-
-#define WRITE_CSR_ARB_WEIGHT(csr_addr, arb, index, value) \
-   ADF_CSR_WR(csr_addr, (ADF_ARB_OFFSET + \
-   ADF_ARB_WTR_OFFSET) + (ADF_ARB_WTR_SIZE * arb) + \
-   (ADF_ARB_REG_SIZE * index), value)
-
 #define WRITE_CSR_ARB_SARCONFIG(csr_addr, index, value) \
ADF_CSR_WR(csr_addr, ADF_ARB_OFFSET + \
(ADF_ARB_REG_SIZE * index), value)
@@ -99,15 +89,6 @@ int adf_init_arb(struct adf_accel_dev *accel_dev)
for (arb = 0; arb < ADF_ARB_NUM; arb++)
WRITE_CSR_ARB_SARCONFIG(csr, arb, arb_cfg);
 
-   /* Setup service weighting */
-   for (arb = 0; arb < ADF_ARB_NUM; arb++)
-   for (i = 0; i < ADF_ARB_REQ_RING_NUM; i++)
-   WRITE_CSR_ARB_WEIGHT(csr, arb, i, 0x);
-
-   /* Setup ring response ordering */
-   for (i = 0; i < ADF_ARB_REQ_RING_NUM; i++)
-   WRITE_CSR_ARB_RESPORDERING(csr, i, 0x);
-
/* Setup worker queue registers */
for (i = 0; i < hw_data->num_engines; i++)
WRITE_CSR_ARB_WQCFG(csr, i, i);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] crypto: implement DH primitives under akcipher API

2016-03-08 Thread Marcel Holtmann
Hi Tadeusz,

>>> In this way we can define a generic user side of the key exchange interface,
 and on the the driver side of the akcipher, the implementations would 
 overload
 the existing akcipher encrypt(), decrypt(), set_pub_key(), set_priv_key() 
 methods
 and do what should be done for a given algorithm. We just need to agree on 
 the
 format of the input parameters to these operations.
>> I think trying to heavily map this to encrypt and decrypt is a bad idea. 
>> These callbacks are not really designed to return what you need them to 
>> return. Key exchange is different.
>> 
>> So why are we trying to push this into akcipher in the first place? I mean 
>> lets just create a keyexch (or similar name) and design it explicitly for 
>> key exchange handling. We are also not trying to map hashes into skcipher. I 
>> think the clear distinction of semantics calls for a different API.
>> 
> 
> I just think that before introducing new API we should try to reuse what's 
> already
> there. Looking at RSA and DH, they basically perform x = a^b mod p, so we 
> should
> have what's needed for both and we just could overload the methods already 
> defined.
> I'm fine either way. Herbert, what's your preference?

I do not think that it is a valid argument because the operations are the same 
to use akcipher. While operations are similar, the inputs are totally 
different. And it is really not performing encrypt or decrypt operations here. 
You use different inputs and get different outputs. You are not encrypting or 
decrypting anything. Especially key generation via decrypt is something I can 
not wrap my mind around.

>>> Agree, we need to support all the cases, but dealing with different type of 
>>> keys
 needs to happen above this API. This API should solely be an interface to 
 math
 primitives of certain operations, which can be implemented in SW or can be 
 HW
 accelerated. Modules like public_key or afalg_akcipher need to be smart 
 enough
 and know what key types they deal with and do the right thing depending on 
 that
 knowledge.
>> I am not sure this can be that easily generalized. Just pushing the problem 
>> of key types and formats off to userspace just forces complexity and extra 
>> knowledge there. We need to be really carefully here. And I really dislike 
>> introducing custom ASN.1 structures that are not backed by an actual RFC 
>> document.
> 
> I'm not saying that we should push this to user space. I'm just saying this 
> should
> be handled above the crypto API.
> 
>>> Agree, and to make this work with afalg_akcipher, I have proposed new
 ALG_SET_KEY_ID and ALG_SET_KEY_ID operations. See here: 
 https://lkml.org/lkml/2015/12/26/65
 The plan is that in this case the afalg_akcipher will not use akcipher api 
 at all.
 In this case the algif_akcipher will use the request_key() interface to 
 retrieve
 the key type info and will use the operations that the new TPM interface 
 will define.
>> The ALG_SET_KEY_ID is great for skcipher where you key is a fixed size blob. 
>> There is works really well. And you will need if we let keyctl derive the 
>> session key from a set of asymmetric keys.
>> 
>> I get that we want to enable OpenSSL and others to gain access to kernel 
>> crypto operations. We want the same actually for ELL as well. However what 
>> is dangerous here is that AF_ALG can never support hardware based keys. So 
>> that means support in OpenSSL is broken by design. See earlier emails from 
>> David Woodhouse. If you have your certificates / keys in hardware then you 
>> need to be able to make them work with OpenSSL. We can not ignore this. It 
>> is a design we need to consider from the beginning.
> 
> If we will have the TPM interface that allows to invoke functions for HW keys 
> and
> the *_KEY_ID, then I don't see why AF_ALG would not work. As I said, in case 
> when
> the keys will be in HW the algif_akcipher will not use akcipher api, but use 
> the
> TPM defined functions in the same way as the keyctl would use it.
> So I think both AF_ALG and keyctl should work just fine.

When David and I talked to Herbert at the kernel summit in Seoul, he said that 
the crypto subsystem (and with that AF_ALG) always needs to support software 
fallback crypto. In case of hardware based keys, there is no software fallback 
possible. If you have a hardware key, then it always needs to use the hardware 
crypto that comes with the key.

And that is precisely my point in going forward with keyctl for all of these. 
We need _one_ interface that can handle software and hardware keys / key pairs.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] crypto: authenc - add TLS type encryption

2016-03-08 Thread Tadeusz Struk
Hi Cristian,
On 03/08/2016 12:20 AM, Cristian Stoica wrote:
> There is also a follow-up in the next paragraph:
> 
> "That pretty much sums up the new attack: the side-channel defenses that were 
> hoped to be sufficient were found not to be (again). So the answer, this time 
> I believe, is to make the processing rigorously constant-time."
> 
> The author makes new changes and continues instrumenting the code and still 
> finds 20 CPU cycles (out of 18000) difference between medians for different 
> paddings. This small difference was detected also on a timing side-channel - 
> which is the point I'm making.
> 
> SSL/TLS is prone to this implementation issue and many user-space libraries 
> got this wrong. It would be good to see some numbers to back-up the claim of 
> timing differences as not being an issue for this one.

It is hard to get the implementation right when the protocol design is error 
prone.
Later we should run some tests on it and see how relevant will this be for a 
remote timing attack.
Thanks,
-- 
TS
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[patch] crypto: rsa-pkcs1pad: indent a couple statements

2016-03-08 Thread Dan Carpenter
These if statements aren't indented far enough and it makes static
checkers complain.

Signed-off-by: Dan Carpenter 
---
Hopefully GCC6 will start complaining about these as well?

diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 1cea67d..1eb5cbe 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -705,7 +705,7 @@ static int pkcs1pad_create(struct crypto_template *tmpl, 
struct rtattr **tb)
 CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s)",
 rsa_alg->base.cra_driver_name) >=
CRYPTO_MAX_ALG_NAME)
-   goto out_drop_alg;
+   goto out_drop_alg;
} else {
if (snprintf(inst->alg.base.cra_name,
 CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s,%s)",
@@ -715,7 +715,7 @@ static int pkcs1pad_create(struct crypto_template *tmpl, 
struct rtattr **tb)
 CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s,%s)",
 rsa_alg->base.cra_driver_name, hash_name) >=
CRYPTO_MAX_ALG_NAME)
-   goto out_free_hash;
+   goto out_free_hash;
}
 
inst->alg.base.cra_flags = rsa_alg->base.cra_flags & CRYPTO_ALG_ASYNC;
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] crypto: authenc - add TLS type encryption

2016-03-08 Thread Cristian Stoica
 Hi Tadeusz,

There is also a follow-up in the next paragraph:

"That pretty much sums up the new attack: the side-channel defenses that were 
hoped to be sufficient were found not to be (again). So the answer, this time I 
believe, is to make the processing rigorously constant-time."

The author makes new changes and continues instrumenting the code and still 
finds 20 CPU cycles (out of 18000) difference between medians for different 
paddings. This small difference was detected also on a timing side-channel - 
which is the point I'm making.

SSL/TLS is prone to this implementation issue and many user-space libraries got 
this wrong. It would be good to see some numbers to back-up the claim of timing 
differences as not being an issue for this one.

Cristian S.



From: Tadeusz Struk 
Sent: Monday, March 7, 2016 4:31 PM
To: Cristian Stoica; herb...@gondor.apana.org.au
Cc: linux-crypto@vger.kernel.org; linux-ker...@vger.kernel.org; 
da...@davemloft.net
Subject: Re: [PATCH 1/3] crypto: authenc - add TLS type encryption

Hi Cristian,
On 03/07/2016 01:05 AM, Cristian Stoica wrote:
> Hi Tadeusz,
>
>
> +static int crypto_encauth_dgst_verify(struct aead_request *req,
> + unsigned int flags)
> +{
> +   struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> +   unsigned int authsize = crypto_aead_authsize(tfm);
> +   struct aead_instance *inst = aead_alg_instance(tfm);
> +   struct crypto_encauth_ctx *ctx = crypto_aead_ctx(tfm);
> +   struct encauth_instance_ctx *ictx = aead_instance_ctx(inst);
> +   struct crypto_ahash *auth = ctx->auth;
> +   struct encauth_request_ctx *areq_ctx = aead_request_ctx(req);
> +   struct ahash_request *ahreq = (void *)(areq_ctx->tail + ictx->reqoff);
> +   u8 *hash = areq_ctx->tail;
> +   int i, err = 0, padd_err = 0;
> +   u8 paddlen, *ihash;
> +   u8 padd[255];
> +
> +   scatterwalk_map_and_copy(&paddlen, req->dst, req->assoclen +
> +req->cryptlen - 1, 1, 0);
> +
> +   if (paddlen > 255 || paddlen > req->cryptlen) {
> +   paddlen = 1;
> +   padd_err = -EBADMSG;
> +   }
> +
> +   scatterwalk_map_and_copy(padd, req->dst, req->assoclen +
> +req->cryptlen - paddlen, paddlen, 0);
> +
> +   for (i = 0; i < paddlen; i++) {
> +   if (padd[i] != paddlen)
> +   padd_err = -EBADMSG;
> +   }
>
>
> This part seems to have the same issue my TLS patch has.
> See for reference what Andy Lutomirski had to say about it:
>
> http://www.mail-archive.com/linux-crypto%40vger.kernel.org/msg11719.html

Thanks for reviewing and for pointing this out. I was aware of the timing-side
issues and done everything I could to avoid it. The main issue that allowed the
Lucky Thirteen attack was that the digest wasn't performed at all if the padding
verification failed. This is not an issue here.
The other issue, which is caused by the length of data to digest being dependent
on the padding length is inevitable and there is nothing we can do about it.
As the note in the paper says:
"However, our behavior matches OpenSSL, so we leak only as much as they do."

Thanks,
--
TS--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html