Re: [tpmdd-devel] in-kernel user of ecdsa

[Stephan Mueller]

Am Montag, 12. März 2018, 22:55:35 CET schrieb James Bottomley:

Hi James,

> > ECDSA is not implemented currently in the kernel crypto API.
> 
> an ECDSA signature is produced as a ECDH operation using the DSA
> algorithm instead of KDFe, so it's trivial with what we have; signature
> verification involves a separate point addition but we have all the
> primitives for this in crypto/ecc.c so adding it isn't really
> difficult, is it?

No, it is not. There even was a patch posted about a year ago to add ECDSA. 
But it was rejected due to missing in-kernel users. I guess that patch could 
be reactivated.

Ciao
Stephan

Re: [tpmdd-devel] in-kernel user of ecdsa

[James Bottomley]

On Mon, 2018-03-12 at 20:56 +0100, Stephan Mueller wrote:
> Am Montag, 12. März 2018, 19:09:18 CET schrieb James Bottomley:
> 
> Hi James,
> 
> > 
> > On Mon, 2018-03-12 at 19:07 +0200, Tudor Ambarus wrote:
> > > 
> > > Hi,
> > > 
> > > Would you consider using ECDSA in the kernel module signing
> > > facility? When compared with RSA, ECDSA has shorter keys, the key
> > > generation process is faster, the sign operation is faster, but
> > > the verify operation is slower than with RSA.
> > 
> > You missed the keyrings list, which is where the module signing
> > utility is discussed.
> > 
> > First question is, have you actually tried?  It looks like sign-
> > file doesn't do anything RSA specific so if you give it an EC X.509
> > certificate it will produce an ECDSA signature.
> > 
> > I think our kernel internal x509 parsers don't have the EC OIDs, so
> > signature verification will fail; but, especially since we have the
> > rest of the EC machinery in the crypto subsystem, that looks to be
> > simply fixable.
> 
> ECDSA is not implemented currently in the kernel crypto API.

an ECDSA signature is produced as a ECDH operation using the DSA
algorithm instead of KDFe, so it's trivial with what we have; signature
verification involves a separate point addition but we have all the
primitives for this in crypto/ecc.c so adding it isn't really
difficult, is it?

James

Re: [tpmdd-devel] in-kernel user of ecdsa

[Stephan Mueller]

Am Montag, 12. März 2018, 19:09:18 CET schrieb James Bottomley:

Hi James,

> On Mon, 2018-03-12 at 19:07 +0200, Tudor Ambarus wrote:
> > Hi,
> > 
> > Would you consider using ECDSA in the kernel module signing facility?
> > When compared with RSA, ECDSA has shorter keys, the key generation
> > process is faster, the sign operation is faster, but the verify
> > operation is slower than with RSA.
> 
> You missed the keyrings list, which is where the module signing utility
> is discussed.
> 
> First question is, have you actually tried?  It looks like sign-file
> doesn't do anything RSA specific so if you give it an EC X.509
> certificate it will produce an ECDSA signature.
> 
> I think our kernel internal x509 parsers don't have the EC OIDs, so
> signature verification will fail; but, especially since we have the
> rest of the EC machinery in the crypto subsystem, that looks to be
> simply fixable.

ECDSA is not implemented currently in the kernel crypto API.
> 
> James



Ciao
Stephan

Re: [tpmdd-devel] in-kernel user of ecdsa

[James Bottomley]

On Mon, 2018-03-12 at 19:07 +0200, Tudor Ambarus wrote:
> Hi,
> 
> Would you consider using ECDSA in the kernel module signing facility?
> When compared with RSA, ECDSA has shorter keys, the key generation
> process is faster, the sign operation is faster, but the verify
> operation is slower than with RSA.

You missed the keyrings list, which is where the module signing utility
is discussed.

First question is, have you actually tried?  It looks like sign-file
doesn't do anything RSA specific so if you give it an EC X.509
certificate it will produce an ECDSA signature.

I think our kernel internal x509 parsers don't have the EC OIDs, so
signature verification will fail; but, especially since we have the
rest of the EC machinery in the crypto subsystem, that looks to be
simply fixable.

James

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[James Bottomley]

On Mon, 2018-03-12 at 09:00 -0700, J Freyensee wrote:
> > 
> > +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +{
> > +   int rc;
> > +
> > +   rc = __tpm_buf_init(buf);
> 
> 
> Assuming that functions like tpm_buf_init() are the top-level API
> being defined in this patch, shouldn't it check if buf is valid
> before passing into the internal functions like __tpm_buf_init(buf)
> (maybe WARN()/BUG_ON()?).  Or does __tpm_buf_init(buf) do this check?

These are kernel internal APIs designed for on stack struct tpm_buf
usage, so I can't think of a viable threat model that would require
this type of checking ... do you have one?

James

in-kernel user of ecdsa

[Tudor Ambarus]

Hi,

Would you consider using ECDSA in the kernel module signing facility?
When compared with RSA, ECDSA has shorter keys, the key generation
process is faster, the sign operation is faster, but the verify
operation is slower than with RSA.

Smaller key sizes imply reduced memory footprint and bandwidth that are
especially attractive for memory constrained devices. I'm working with
such a device, capable of generating ecc keys, secure key storage and
ecdsa/ecdh crypto acceleration. I'm trying to find an in-kernel user of
ecdsa.


ECDSA and RSA comparison

-> ECDSA requires a much smaller key length in order to provide the same
security strength as RSA [1]:

Security StrengthRSA (bits)ECDSA (bits)
112   2048 224 - 255
128   3072 256 - 383
192   7680 384 - 511
256  15360 512+

7680 and 15360  keys are not included in the NIST standards for
interoperability and efficiency reasons, the keys are just too big.

-> key generation: ECC key generation is faster than IFC (Integer -
Factorization Cryptography). RSA private key is based on large prime
numbers, while for ECDSA any positive integer less than n is a valid
private key.

-> ECDSA sign operations are faster than RSA, but verify operations are
slower. Here's an openssl speed test that I've run on my computer:

  signverifysign/s verify/s
rsa 2048 bits 0.000604s 0.18s   1656.3  56813.7
rsa 4096 bits 0.004027s 0.62s248.3  16052.5

 signverifysign/s verify/s
256 bit ecdsa (nistp256)   0.s   0.0001s  28986.4  13516.3
384 bit ecdsa (nistp384)   0.0002s   0.0008s   5541.0   1322.2
521 bit ecdsa (nistp521)   0.0003s   0.0006s   3104.2   1756.2

Best,
ta

[1] NIST SP 800-57 Pt. 1 Rev. 4, Recommendation for key management

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[J Freyensee]

+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+   int rc;
+
+   rc = __tpm_buf_init(buf);



Assuming that functions like tpm_buf_init() are the top-level API being 
defined in this patch, shouldn't it check if buf is valid before passing 
into the internal functions like __tpm_buf_init(buf) (maybe 
WARN()/BUG_ON()?).  Or does __tpm_buf_init(buf) do this check?


Thanks,
Jay

Re: [PATCH v3 0/6] add integrity and security to TPM2 transactions

[James Bottomley]

On Mon, 2018-03-12 at 12:58 +0200, Jarkko Sakkinen wrote:
> On Sat, 2018-03-10 at 14:13 -0800, James Bottomley wrote:
> > 
> > By now, everybody knows we have a problem with the TPM2_RS_PW easy
> > button on TPM2 in that transactions on the TPM bus can be
> > intercepted
> > and altered.  The way to fix this is to use real sessions for HMAC
> > capabilities to ensure integrity and to use parameter and response
> > encryption to ensure confidentiality of the data flowing over the
> > TPM
> > bus.
> > 
> > This patch series is about adding a simple API which can ensure the
> > above properties as a layered addition to the existing TPM handling
> > code.  This series now includes protections for PCR extend, getting
> > random numbers from the TPM and data sealing and unsealing.  It
> > therefore eliminates all uses of TPM2_RS_PW in the kernel and adds
> > encryption protection to sensitive data flowing into and out of the
> > TPM.
> > 
> > This series is also dependent on additions to the crypto subsystem
> > to
> > fix problems in the elliptic curve key handling and add the Cipher
> > FeedBack encryption scheme:
> > 
> > https://marc.info/?l=linux-crypto-vger=151994371015475
> > 
> > In the third version I've added data sealing and unsealing
> > protection,
> > apart from one API based problem which means that the way trusted
> > keys
> > were protected it's not currently possible to HMAC protect an
> > authority
> > that comes with a policy, so the API will have to be extended to
> > fix
> > that case
> > 
> > I've verified this using the test suite in the last patch on a VM
> > connected to a tpm2 emulator.  I also instrumented the emulator to
> > make
> > sure the sensitive data was properly encrypted.
> > 
> > James
> 
> 1. Can I ignore v2 and just review/test this version? I haven't even
>    peeked into v2 yet.

Yes, v3 is a more complete version of v2 with a couple of sessions API
additions.

I think the way I'm going to fix the trusted key policy problem is to
move it back into the kernel for the simple PCR lock policy (which will
make changing from 1.2 to 2.0 seamless because the external Key API
will then become the same) so the kernel gets the missing TPM nonce and
can then do TPM2_PolicyAuthValue.

User generated policy sessions for trusted keys are very flexible but
also a hugely bad idea for consumers because it's so different from the
way 1.2 works and it means now the user has to exercise a TPM API to
produce the policy sessions.

Longer term, I think having a particular trusted key represent a policy
session which can then be attached to a different trusted key
representing the blob is the best idea because we can expose the policy
build up via the trusted key API and keep all the TPM nastiness inside
the kernel.

> 2. Do you know in which kernel version will the crypto additions
> land?

They're here:

https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/log/

So I'd guess next merge window.  You can do what we do in SCSI and
create a "postmerge" branch based on the cryptodev one (we often have
SCSI stuff with block tree precursors).  The way I run it is that I
don't send the merge window pull request until I see the merge-base
against Linus master move to the base of the patches (meaning all the
precursors are upstream).

> /Jarkko
> 

Re: what is a replacement for private_AES_set_encrypt_key and AES_encrypt functions

[Ard Biesheuvel]

On 12 March 2018 at 14:38, Vitaly Andrianov  wrote:
> Hello,
>
> The Texas Instruments keystone2 out-of-tree kernel uses the
> private_AES_set_encrypt_key() and
> the AES_encrypt() at the crypto HW acceleration driver.
>
> The "crypto: arm/aes - replace bit-sliced OpenSSL NEON code" commit removed
> those functions.
> Here is a code, which calls the removed functions.
>
> static inline int sa_aes_xcbc_subkey(u8 *sub_key1, u8 *sub_key2,
>  u8 *sub_key3, const u8 *key,
>  u16 key_sz)
> {
> struct AES_KEY enc_key;
>
> if (private_AES_set_encrypt_key(key, (key_sz * 8), _key)) {
> pr_err("%s: failed to set enc key\n", __func__);
> return -EINVAL;
> }
>
> if (sub_key1) {
> memset(sub_key1, 0x01, AES_BLOCK_SIZE);
> AES_encrypt(sub_key1, sub_key1, _key);
> }
>
> if (sub_key2) {
> memset(sub_key2, 0x02, AES_BLOCK_SIZE);
> AES_encrypt(sub_key2, sub_key2, _key);
> }
>
> if (sub_key3) {
> memset(sub_key3, 0x03, AES_BLOCK_SIZE);
> AES_encrypt(sub_key3, sub_key3, _key);
> }
>
> return 0;
> }
>
> Which functions can I use to replace the removed ones in the above code?
>

Look at xcbc_setkey() in arch/arm64/crypto/aes-glue.c for an example

what is a replacement for private_AES_set_encrypt_key and AES_encrypt functions

[Vitaly Andrianov]

Hello,

The Texas Instruments keystone2 out-of-tree kernel uses the 
private_AES_set_encrypt_key() and
the AES_encrypt() at the crypto HW acceleration driver.

The "crypto: arm/aes - replace bit-sliced OpenSSL NEON code" commit removed 
those functions.
Here is a code, which calls the removed functions.

static inline int sa_aes_xcbc_subkey(u8 *sub_key1, u8 *sub_key2,
 u8 *sub_key3, const u8 *key,
 u16 key_sz)
{
struct AES_KEY enc_key;

if (private_AES_set_encrypt_key(key, (key_sz * 8), _key)) {
pr_err("%s: failed to set enc key\n", __func__);
return -EINVAL;
}

if (sub_key1) {
memset(sub_key1, 0x01, AES_BLOCK_SIZE);
AES_encrypt(sub_key1, sub_key1, _key);
}

if (sub_key2) {
memset(sub_key2, 0x02, AES_BLOCK_SIZE);
AES_encrypt(sub_key2, sub_key2, _key);
}

if (sub_key3) {
memset(sub_key3, 0x03, AES_BLOCK_SIZE);
AES_encrypt(sub_key3, sub_key3, _key);
}

return 0;
}

Which functions can I use to replace the removed ones in the above code?

I'm very appreciate any advice.

Sincerely,
-Vitaly Andrianov

Re: [PATCH 2/2] crypto: talitos: Delete an error message for a failed memory allocation in talitos_edesc_alloc()

[Christophe LEROY]

Le 12/03/2018 à 14:32, SF Markus Elfring a écrit :

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:18:23 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 


Reviewed-by: Christophe Leroy 


---
  drivers/crypto/talitos.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a2271322db34..4c7318981d28 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1399,7 +1399,6 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
  
  	edesc = kmalloc(alloc_len, GFP_DMA | flags);

if (!edesc) {
-   dev_err(dev, "could not allocate edescriptor\n");
err = ERR_PTR(-ENOMEM);
goto error_sg;
}

Re: [PATCH 1/2] crypto: talitos: Use common error handling code in talitos_edesc_alloc()

[Christophe LEROY]

Le 12/03/2018 à 14:31, SF Markus Elfring a écrit :

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:08:55 +0100

Add jump targets so that an error message and the setting of a specific
error code is stored only once at the end of this function.

Signed-off-by: Markus Elfring 
---
  drivers/crypto/talitos.c | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6882fa2f8bad..a2271322db34 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1352,29 +1352,24 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
if (!dst || dst == src) {
src_len = assoclen + cryptlen + authsize;
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");


I don't think this is a good idea to move this dev_err(), just because
the text is identical twice.
The code is more clear when the text of the error is at the error.
Also, as this text is for the case where SRC SG is identical to DST SG,
the text could instead be "Invalid number of SG"


-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_nents = dst ? src_nents : 0;
dst_len = 0;
} else { /* dst && dst != src*/
src_len = assoclen + cryptlen + (encrypt ? 0 : authsize);
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_len = assoclen + cryptlen + (encrypt ? authsize : 0);
dst_nents = sg_nents_for_len(dst, dst_len);
if (dst_nents < 0) {
dev_err(dev, "Invalid number of dst SG.\n");


To be consistant, this one should also go at the end if we want to be 
consistant.



-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
+   goto set_error_code;
}
dst_nents = (dst_nents == 1) ? 0 : dst_nents;
}
@@ -1424,6 +1419,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 DMA_BIDIRECTIONAL);
}
return edesc;
+
+report_failure:
+   dev_err(dev, "Invalid number of src SG.\n");
+set_error_code:
+   err = ERR_PTR(-EINVAL);
  error_sg:
if (iv_dma)
dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);



Another solution could be to move the dma_map_single() of iv_dma after 
the kmalloc(), in that case we could directly return from the other 
error cases instead of having the unmap iv_dma first.


Christophe

[PATCH 2/2] crypto: talitos: Delete an error message for a failed memory allocation in talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:18:23 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/crypto/talitos.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a2271322db34..4c7318981d28 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1399,7 +1399,6 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 
edesc = kmalloc(alloc_len, GFP_DMA | flags);
if (!edesc) {
-   dev_err(dev, "could not allocate edescriptor\n");
err = ERR_PTR(-ENOMEM);
goto error_sg;
}
-- 
2.16.2

[PATCH 1/2] crypto: talitos: Use common error handling code in talitos_edesc_alloc()

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 12 Mar 2018 14:08:55 +0100

Add jump targets so that an error message and the setting of a specific
error code is stored only once at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/crypto/talitos.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6882fa2f8bad..a2271322db34 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1352,29 +1352,24 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
if (!dst || dst == src) {
src_len = assoclen + cryptlen + authsize;
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_nents = dst ? src_nents : 0;
dst_len = 0;
} else { /* dst && dst != src*/
src_len = assoclen + cryptlen + (encrypt ? 0 : authsize);
src_nents = sg_nents_for_len(src, src_len);
-   if (src_nents < 0) {
-   dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
-   }
+   if (src_nents < 0)
+   goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_len = assoclen + cryptlen + (encrypt ? authsize : 0);
dst_nents = sg_nents_for_len(dst, dst_len);
if (dst_nents < 0) {
dev_err(dev, "Invalid number of dst SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
+   goto set_error_code;
}
dst_nents = (dst_nents == 1) ? 0 : dst_nents;
}
@@ -1424,6 +1419,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 DMA_BIDIRECTIONAL);
}
return edesc;
+
+report_failure:
+   dev_err(dev, "Invalid number of src SG.\n");
+set_error_code:
+   err = ERR_PTR(-EINVAL);
 error_sg:
if (iv_dma)
dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);
-- 
2.16.2

Re: [PATCH v10 crypto 07/11] chtls: Program the TLS Key

[Stefano Brivio]

On Sat, 10 Mar 2018 00:40:12 +0530
Atul Gupta  wrote:

> Initialize the space reserved for storing the TLS keys
> get and free the location where key is stored for the TLS
> connection
> Program the tx and rx key as received from user in
> struct tls12_crypto_info_aes_gcm_128 and understood by hardware.
> 
> Signed-off-by: Atul Gupta 
> ---
>  drivers/crypto/chelsio/chtls/chtls_hw.c | 371 
> 
>  1 file changed, 371 insertions(+)
>  create mode 100644 drivers/crypto/chelsio/chtls/chtls_hw.c
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_hw.c 
> b/drivers/crypto/chelsio/chtls/chtls_hw.c
> new file mode 100644
> index 000..40cf84f
> --- /dev/null
> +++ b/drivers/crypto/chelsio/chtls/chtls_hw.c
> @@ -0,0 +1,371 @@
> +/*
> + * Copyright (c) 2017 Chelsio Communications, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Written by: Atul Gupta (atul.gu...@chelsio.com)
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "chtls.h"
> +#include "chtls_cm.h"
> +
> +static void __set_tcb_field_direct(struct chtls_sock *csk,
> +struct cpl_set_tcb_field *req, u16 word,
> +u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> + struct ulptx_idata *sc;
> +
> + INIT_TP_WR_CPL(req, CPL_SET_TCB_FIELD, csk->tid);
> + req->wr.wr_mid |= htonl(FW_WR_FLOWID_V(csk->tid));
> + req->reply_ctrl = htons(NO_REPLY_V(no_reply) |
> + QUEUENO_V(csk->rss_qid));
> + req->word_cookie = htons(TCB_WORD_V(word) | TCB_COOKIE_V(cookie));
> + req->mask = cpu_to_be64(mask);
> + req->val = cpu_to_be64(val);
> + sc = (struct ulptx_idata *)(req + 1);
> + sc->cmd_more = htonl(ULPTX_CMD_V(ULP_TX_SC_NOOP));
> + sc->len = htonl(0);
> +}
> +
> +static void __set_tcb_field(struct sock *sk, struct sk_buff *skb, u16 word,
> + u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> +
> + req = (struct cpl_set_tcb_field *)__skb_put(skb, wrlen);
> + __set_tcb_field_direct(csk, req, word, mask, val, cookie, no_reply);
> + set_wr_txq(skb, CPL_PRIORITY_CONTROL, csk->port_id);
> +}
> +
> +/*
> + * Send control message to HW, message go as immediate data and packet
> + * is freed immediately.
> + */
> +static void chtls_set_tcb_field(struct sock *sk, u16 word, u64 mask, u64 val)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + struct sk_buff *skb;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> + unsigned int credits_needed = DIV_ROUND_UP(wrlen, 16);
> +
> + skb = alloc_skb(wrlen, GFP_ATOMIC);
> + if (!skb)
> + return;
> +
> + __set_tcb_field(sk, skb, word, mask, val, 0, 1);
> + set_queue(skb, (csk->txq_idx << 1) | CPL_PRIORITY_DATA, sk);
> + csk->wr_credits -= credits_needed;
> + csk->wr_unacked += credits_needed;
> + enqueue_wr(csk, skb);
> + cxgb4_ofld_send(csk->egress_dev, skb);
> +}

What happens if e.g. alloc_skb() fails here? Is it fine to ignore the
error altogether?

> +
> +/*
> + * Set one of the t_flags bits in the TCB.
> + */
> +void chtls_set_tcb_tflag(struct sock *sk, unsigned int bit_pos, int val)
> +{
> + return chtls_set_tcb_field(sk, 1, 1ULL << bit_pos,
> + val << bit_pos);
> +}
> +
> +static void chtls_set_tcb_keyid(struct sock *sk, int keyid)
> +{
> + return chtls_set_tcb_field(sk, 31, 0xULL, keyid);
> +}
> +
> +static void chtls_set_tcb_seqno(struct sock *sk)
> +{
> + return chtls_set_tcb_field(sk, 28, ~0ULL, 0);
> +}
> +
> +static void chtls_set_tcb_quiesce(struct sock *sk, int val)
> +{
> + return chtls_set_tcb_field(sk, 1, (1ULL << TF_RX_QUIESCE_S),
> +TF_RX_QUIESCE_V(val));
> +}

Why would you return from a void function?

IMHO, it would be more productive if you could address the comments you
are receiving a bit more carefully instead of patching them up quickly
and race to re-post, because this doesn't seem to gain any time.

-- 
Stefano

Re: [PATCH v10 crypto 09/11] chtls: Inline TLS request Tx/Rx

[Stefano Brivio]

On Sat, 10 Mar 2018 00:40:14 +0530
Atul Gupta  wrote:

> TLS handler for record transmit and receive.
> Create Inline TLS work request and post to FW.
> Create Inline TLS record CPLs for hardware
> 
> Signed-off-by: Atul Gupta 
> ---
>  drivers/crypto/chelsio/chtls/chtls_io.c | 1863 
> +++
>  1 file changed, 1863 insertions(+)
>  create mode 100644 drivers/crypto/chelsio/chtls/chtls_io.c
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c 
> b/drivers/crypto/chelsio/chtls/chtls_io.c
> new file mode 100644
> index 000..f7f5826
> --- /dev/null
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -0,0 +1,1863 @@
> +/*
> + * Copyright (c) 2017 Chelsio Communications, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Written by: Atul Gupta (atul.gu...@chelsio.com)
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "chtls.h"
> +#include "chtls_cm.h"
> +
> +static bool is_tls_hw(struct chtls_sock *csk)
> +{
> + return csk->tlshws.ofld;
> +}

Do you actually need this function?

> +static bool is_tls_rx(struct chtls_sock *csk)
> +{
> + return (csk->tlshws.rxkey >= 0);
> +}

You can drop the ().

> +
> +static bool is_tls_tx(struct chtls_sock *csk)
> +{
> + return (csk->tlshws.txkey >= 0);
> +}

You can drop the ().

> +static bool is_tls_skb(struct chtls_sock *csk, const struct sk_buff *skb)
> +{
> + return (is_tls_hw(csk) && skb_ulp_tls_skb_flags(skb));
> +}

You can drop the ().

> +static int key_size(void *sk)
> +{
> + return 16; /* Key on DDR */
> +}

Do you actually need this function? Can't you turn that into a #define?

> +
> +#define ceil(x, y) \
> + ({ unsigned long __x = (x), __y = (y); (__x + __y - 1) / __y; })

This doesn't look like a ceiling function, could you perhaps name it
more descriptively?

> +static int data_sgl_len(const struct sk_buff *skb)
> +{
> + unsigned int cnt;
> +
> + cnt = skb_shinfo(skb)->nr_frags;
> + return (sgl_len(cnt) * 8);

You can drop the ().

> +}
> +
> +static int nos_ivs(struct sock *sk, unsigned int size)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +
> + return ceil(size, csk->tlshws.mfs);
> +}
> +
> +#define TLS_WR_CPL_LEN \
> + (sizeof(struct fw_tlstx_data_wr) + \
> + sizeof(struct cpl_tx_tls_sfo))
> +
> +static int is_ivs_imm(struct sock *sk, const struct sk_buff *skb)

Shouldn't this be a bool?

> +{
> + int ivs_size = nos_ivs(sk, skb->len) * CIPHER_BLOCK_SIZE;
> + int hlen = TLS_WR_CPL_LEN + data_sgl_len(skb);
> +
> + if ((hlen + key_size(sk) + ivs_size) <
> + MAX_IMM_OFLD_TX_DATA_WR_LEN) {
> + ULP_SKB_CB(skb)->ulp.tls.iv = 1;
> + return 1;
> + }
> + ULP_SKB_CB(skb)->ulp.tls.iv = 0;

Are these assignments intended? They don't seem to fit with the name of
the function.

> + return 0;
> +}
> +
> +static int max_ivs_size(struct sock *sk, int size)
> +{
> + return (nos_ivs(sk, size) * CIPHER_BLOCK_SIZE);
> +}

You can drop the ().

> +
> +static int ivs_size(struct sock *sk, const struct sk_buff *skb)
> +{
> + return (is_ivs_imm(sk, skb) ? (nos_ivs(sk, skb->len) *
> +  CIPHER_BLOCK_SIZE) : 0);
> +}

You can drop the ().

> [...]
>
> +static u64 tls_sequence_number(struct chtls_hws *hws)
> +{
> + return (hws->tx_seq_no++);
> +}

You can drop the (), and I find the name of this function a bit misleading
as you are actually increasing the sequence number, not just returning
it.

> +
> +static int is_sg_request(const struct sk_buff *skb)
> +{
> + return (skb->peeked ||
> + (skb->len > MAX_IMM_ULPTX_WR_LEN));
> +}

You can drop the (). This should be a bool.

> +
> +/*
> + * Returns true if an sk_buff carries urgent data.
> + */
> +static int skb_urgent(struct sk_buff *skb)
> +{
> + return (ULP_SKB_CB(skb)->flags & ULPCB_FLAG_URG) != 0;
> +}

This should be a bool, you can drop the (), and avoid that != 0
comparison.

-- 
Stefano

Re: [RFC 0/5] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

On Sat, 2018-03-10 at 10:29 -0800, James Bottomley wrote:
> OK, you might want to wait for v3 then.  I've got it working with
> sealed (trusted) keys, well except for a problem with the trusted keys
> API that means we can't protect the password for policy based keys.  I
> think the API is finally complete, so I'll send v3 as a PATCH not an
> RFC.
> 
> The point of the last patch is to show the test rig for this I'm
> running in a VM using an instrumented tpm2 emulator to prove we're
> getting all the correct data in and out (and that the encryption and
> hmac are working); more physical TPM testing would be useful ..

Sorry, I did not notice this email in my inbox before I responded
to you v3 cover letter :-) Thank you.

/Jarkko

Re: [PATCH v3 0/6] add integrity and security to TPM2 transactions

[Jarkko Sakkinen]

On Sat, 2018-03-10 at 14:13 -0800, James Bottomley wrote:
> By now, everybody knows we have a problem with the TPM2_RS_PW easy
> button on TPM2 in that transactions on the TPM bus can be intercepted
> and altered.  The way to fix this is to use real sessions for HMAC
> capabilities to ensure integrity and to use parameter and response
> encryption to ensure confidentiality of the data flowing over the TPM
> bus.
> 
> This patch series is about adding a simple API which can ensure the
> above properties as a layered addition to the existing TPM handling
> code.  This series now includes protections for PCR extend, getting
> random numbers from the TPM and data sealing and unsealing.  It
> therefore eliminates all uses of TPM2_RS_PW in the kernel and adds
> encryption protection to sensitive data flowing into and out of the
> TPM.
> 
> This series is also dependent on additions to the crypto subsystem to
> fix problems in the elliptic curve key handling and add the Cipher
> FeedBack encryption scheme:
> 
> https://marc.info/?l=linux-crypto-vger=151994371015475
> 
> In the third version I've added data sealing and unsealing protection,
> apart from one API based problem which means that the way trusted keys
> were protected it's not currently possible to HMAC protect an authority
> that comes with a policy, so the API will have to be extended to fix
> that case
> 
> I've verified this using the test suite in the last patch on a VM
> connected to a tpm2 emulator.  I also instrumented the emulator to make
> sure the sensitive data was properly encrypted.
> 
> James

1. Can I ignore v2 and just review/test this version? I haven't even
   peeked into v2 yet.
2. Do you know in which kernel version will the crypto additions land?

/Jarkko