Re: [PATCH v15 0/9] Add io{read|write}64 to io-64-atomic headers

[Michael Ellerman]

Logan Gunthorpe  writes:
> On 4/4/2018 4:38 AM, Michael Ellerman wrote:
...
>> eg. It looks like I could take the two powerpc patches on their own for
>> 4.17, and then the rest could go via other trees?
>
> Yup! If you can take the powerpc patches I can keep trying to get the 
> rest in. They are largely independent and shouldn't really change 
> anything without the following patches.

OK, I'll take those then.

>> Is patch 1 stand alone?
>
> Essentially, yes, but patch 5 depends on it seeing it's changing the 
> same area and is trying to avoid creating the same kbuild warnings that 
> patch 1 suppresses.
>
>> The other option is to ask Andrew Morton to take it, as he often carries
>> these cross-tree type series.
>
> Thanks for the tip! I'll copy him when I repost it after the merge window.

Cool. If you want him to take it you should probably explicitly say so
when you repost.

cheers

Re: [PATCH v15 0/9] Add io{read|write}64 to io-64-atomic headers

[Logan Gunthorpe]

On 4/4/2018 4:38 AM, Michael Ellerman wrote:

What's the plan for getting it merged? Seems we don't have one?


Yeah, so far there is no plan. I'm not really sure who's attention I 
need to get or how to get it.



Given it touches two arches and generic stuff and drivers and crypto,
it's a bit of a mess to merge. It's not really something I want to merge
via the powerpc tree.


Understood.


Is there any way to split it? I couldn't immediately see what the hard
dependencies were between the patches.


Yes, a couple of patches have already been taken by their maintainers 
over the last few cycles.



eg. It looks like I could take the two powerpc patches on their own for
4.17, and then the rest could go via other trees?


Yup! If you can take the powerpc patches I can keep trying to get the 
rest in. They are largely independent and shouldn't really change 
anything without the following patches.



Is patch 1 stand alone?


Essentially, yes, but patch 5 depends on it seeing it's changing the 
same area and is trying to avoid creating the same kbuild warnings that 
patch 1 suppresses.



The other option is to ask Andrew Morton to take it, as he often carries
these cross-tree type series.


Thanks for the tip! I'll copy him when I repost it after the merge window.

Logan

Crypto Update for 4.17

[Herbert Xu]

Hi Linus: 

Here is the crypto update for 4.17:

API:

- Add AEAD support to crypto engine.
- Allow batch registration in simd.

Algorithms:

- Add CFB mode.
- Add speck block cipher.
- Add sm4 block cipher.
- Add new test case for crct10dif.
- Improve scheduling latency on ARM.
- Add scatter/gather support to gcm in aesni.
- Convert x86 crypto algorithms to skcihper.

Drivers:

- Add hmac(sha224/sha256) support in inside-secure.
- Add aes gcm/ccm support in stm32.
- Add stm32mp1 support in stm32.
- Add ccree driver from staging tree.
- Add gcm support over QI in caam.
- Add ks-sa hwrng driver.

Please note that there will be a conflict with the net tree due
updates to the same header file in the chelsio driver.


Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Andy Shevchenko (1):
  crypto: Deduplicate le32_to_cpu_array() and cpu_to_le32_array()

Antoine Tenart (20):
  MAINTAINERS: update the Inside Secure maintainer email
  crypto: inside-secure - do not overwrite the threshold value
  crypto: inside-secure - fix the extra cache computation
  crypto: inside-secure - fix the cache_len computation
  crypto: inside-secure - do not process request if no command was issued
  crypto: inside-secure - fix the invalidation step during cra_exit
  crypto: inside-secure - keep the requests push/pop synced
  crypto: inside-secure - unmap the result in the hash send error path
  crypto: atmel-aes - fix the keys zeroing on errors
  crypto: inside-secure - move cache result dma mapping to request
  crypto: inside-secure - wait for the request to complete if in the backlog
  crypto: inside-secure - move the digest to the request context
  crypto: inside-secure - fix typo s/allways/always/ in a define
  crypto: inside-secure - fix a typo in a register name
  crypto: inside-secure - improve the send error path
  crypto: inside-secure - do not access buffers mapped to the device
  crypto: inside-secure - improve the skcipher token
  crypto: inside-secure - the context ipad/opad should use the state sz
  crypto: inside-secure - hmac(sha256) support
  crypto: inside-secure - hmac(sha224) support

Ard Biesheuvel (9):
  crypto: testmgr - add a new test case for CRC-T10DIF
  crypto: arm64/aes-ce-ccm - move kernel mode neon en/disable into loop
  crypto: arm64/aes-blk - move kernel mode neon en/disable into loop
  crypto: arm64/aes-bs - move kernel mode neon en/disable into loop
  crypto: arm64/chacha20 - move kernel mode neon en/disable into loop
  crypto: arm64/aes-blk - remove configurable interleave
  crypto: arm64/aes-blk - add 4 way interleave to CBC encrypt path
  crypto: arm64/aes-blk - add 4 way interleave to CBC-MAC encrypt path
  crypto: arm64/sha256-neon - play nice with CONFIG_PREEMPT kernels

Arnd Bergmann (1):
  crypto: bfin_crc - remove blackfin CRC driver

Atul Gupta (1):
  crypto: chelsio - no csum offload for ipsec path

Brijesh Singh (3):
  crypto: ccp - add check to get PSP master only when PSP is detected
  crypto: ccp - Fix sparse, use plain integer as NULL pointer
  include: psp-sev: Capitalize invalid length enum

Colin Ian King (4):
  crypto: chelsio - Make function aead_ccm_validate_input static
  crypto: ccp - Make function ccp_get_dma_chan_attr static
  crypto: qat - Make several functions static
  hwrng: cavium - make two functions static

Conor McLoughlin (1):
  crypto: testmgr - Fix incorrect values in PKCS#1 test vector

Corentin LABBE (6):
  crypto: doc - document crypto engine API
  crypto: engine - Permit to enqueue all async requests
  crypto: omap - convert to new crypto engine API
  crypto: virtio - convert to new crypto engine API
  crypto: stm32-hash - convert to the new crypto engine API
  crypto: stm32-cryp - convert to the new crypto engine API

Dave Watson (14):
  crypto: aesni - Merge INITIAL_BLOCKS_ENC/DEC
  crypto: aesni - Macro-ify func save/restore
  crypto: aesni - Add GCM_INIT macro
  crypto: aesni - Add GCM_COMPLETE macro
  crypto: aesni - Merge encode and decode to GCM_ENC_DEC macro
  crypto: aesni - Introduce gcm_context_data
  crypto: aesni - Split AAD hash calculation to separate macro
  crypto: aesni - Fill in new context data structures
  crypto: aesni - Move ghash_mul to GCM_COMPLETE
  crypto: aesni - Move HashKey computation from stack to gcm_context
  crypto: aesni - Introduce partial block macro
  crypto: aesni - Add fast path for > 16 byte update
  crypto: aesni - Introduce scatter/gather asm function stubs
  crypto: aesni - Update aesni-intel_glue to use scatter/gather

Eric Biggers (40):
  crypto: mcryptd - remove pointless wrapper functions
  crypto: sha1-mb - remove HASH_FIRST flag
  crypto: sha256-mb - remove HASH_FIRST flag
  crypto: sha512-mb - remove HASH_FIRST 

[bug report] crypto: chtls - Program the TLS session Key

[Dan Carpenter]

Hello Atul Gupta,

The patch d25f2f71f653: "crypto: chtls - Program the TLS session Key"
from Mar 31, 2018, leads to the following static checker warning:

drivers/crypto/chelsio/chtls/chtls_hw.c:239 chtls_key_info()
error: '__memcpy()' 'key' too small (2 vs 32)

drivers/crypto/chelsio/chtls/chtls_hw.c
   212  static int chtls_key_info(struct chtls_sock *csk,
   213struct _key_ctx *kctx,
   214u32 keylen, u32 optname)
   215  {
   216  unsigned char key[CHCR_KEYCTX_CIPHER_KEY_SIZE_256];
  ^^^
This is 2 bytes long.  It was probably supposed to be
AES_KEYSIZE_256 (32 bytes).

   217  struct tls12_crypto_info_aes_gcm_128 *gcm_ctx;
   218  unsigned char ghash_h[AEAD_H_SIZE];
   219  struct crypto_cipher *cipher;
   220  int ck_size, key_ctx_size;
   221  int ret;
   222  
   223  gcm_ctx = (struct tls12_crypto_info_aes_gcm_128 *)
   224>tlshws.crypto_info;
   225  
   226  key_ctx_size = sizeof(struct _key_ctx) +
   227 roundup(keylen, 16) + AEAD_H_SIZE;
   228  
   229  if (keylen == AES_KEYSIZE_128) {
   230  ck_size = CHCR_KEYCTX_CIPHER_KEY_SIZE_128;
   231  } else if (keylen == AES_KEYSIZE_192) {
   232  ck_size = CHCR_KEYCTX_CIPHER_KEY_SIZE_192;
   233  } else if (keylen == AES_KEYSIZE_256) {
   ^
keylen is 32.

   234  ck_size = CHCR_KEYCTX_CIPHER_KEY_SIZE_256;
   235  } else {
   236  pr_err("GCM: Invalid key length %d\n", keylen);
   237  return -EINVAL;
   238  }
   239  memcpy(key, gcm_ctx->key, keylen);
   ^
Memory corruption.  Smatch also complains that gcm_ctx->key is 16 bytes
instead of 32.

drivers/crypto/chelsio/chtls/chtls_hw.c:239 chtls_key_info()
error: '__memcpy()' 'gcm_ctx->key' too small (16 vs 32)

   240  

See also:

drivers/crypto/chelsio/chtls/chtls_hw.c:250 chtls_key_info() error: 
'crypto_cipher_setkey()' 'key' too small (2 vs 32)
drivers/crypto/chelsio/chtls/chtls_hw.c:274 chtls_key_info() error: 
'__memcpy()' 'gcm_ctx->key' too small (16 vs 32)
drivers/crypto/chelsio/chtls/chtls_hw.c:277 chtls_key_info() error: 
'__memset()' 'gcm_ctx->key' too small (16 vs 32)

regards,
dan carpenter

[bug report] crypto: chtls - Inline TLS record Tx

[Dan Carpenter]

Hello Atul Gupta,

This is a semi-automatic email about new static checker warnings.

The patch 36bedb3f2e5b: "crypto: chtls - Inline TLS record Tx" from 
Mar 31, 2018, leads to the following Smatch complaint:

drivers/crypto/chelsio/chtls/chtls_io.c:1156 chtls_sendpage()
warn: variable dereferenced before check 'skb' (see line 1155)

drivers/crypto/chelsio/chtls/chtls_io.c
  1154  
  1155  copy = mss - skb->len;
 
Dereference

  1156  if (!skb || (ULP_SKB_CB(skb)->flags & 
ULPCB_FLAG_NO_APPEND) ||

Check

  1157  copy <= 0) {
  1158  new_buf:

regards,
dan carpenter

[bug report] crypto: chtls - Register chtls with net tls

[Dan Carpenter]

Hello Atul Gupta,

The patch a08943947873: "crypto: chtls - Register chtls with net tls"
from Mar 31, 2018, leads to the following static checker warning:

drivers/crypto/chelsio/chtls/chtls_main.c:447 do_chtls_getsockopt()
warn: check that 'crypto_info.cipher_type' doesn't leak information

drivers/crypto/chelsio/chtls/chtls_main.c
   441  static int do_chtls_getsockopt(struct sock *sk, char __user *optval,
   442 int __user *optlen)
   443  {
   444  struct tls_crypto_info crypto_info;
   445  
   446  crypto_info.version = TLS_1_2_VERSION;
   447  if (copy_to_user(optval, _info, sizeof(struct 
tls_crypto_info)))
   448  return -EFAULT;

It is an info leak, but perhaps instead of just zeroing it out we could
set crypto_info.cipher_type to something meaningful?

   449  return 0;
   450  }

regards,
dan carpenter

[bug report] crypto: chtls - Inline TLS record Rx

[Dan Carpenter]

Hello Atul Gupta,

The patch b647993fca14: "crypto: chtls - Inline TLS record Rx" from
Mar 31, 2018, leads to the following static checker warning:

drivers/crypto/chelsio/chtls/chtls_io.c:1412 chtls_pt_recvmsg()
warn: inconsistent indenting

drivers/crypto/chelsio/chtls/chtls_io.c
  1401  if (sk->sk_backlog.tail) {
  1402  release_sock(sk);
  1403  lock_sock(sk);
  1404  chtls_cleanup_rbuf(sk, copied);
  1405  continue;
  1406  }
  1407  
  1408  if (copied >= target)
  1409  break;
  1410  chtls_cleanup_rbuf(sk, copied);
  1411  sk_wait_data(sk, , NULL);
  1412  continue;

The continue is indented too far.  Were you intending to check the
return from sk_wait_data() or something?

  1413  found_ok_skb:
  1414  if (!skb->len) {
  1415  skb_dst_set(skb, NULL);
  1416  __skb_unlink(skb, >sk_receive_queue);
  1417  kfree_skb(skb);
  1418  
  1419  if (!copied && !timeo) {
  1420  copied = -EAGAIN;
  1421  break;
  1422  }
  1423  
  1424  if (copied < target) {
  1425  release_sock(sk);
  1426  lock_sock(sk);
  1427  continue;
  1428  }
  1429  break;
  1430  }


regards,
dan carpenter

[bug report] crypto: chtls - Inline TLS record Tx

[Dan Carpenter]

Hello Atul Gupta,

The patch 36bedb3f2e5b: "crypto: chtls - Inline TLS record Tx" from
Mar 31, 2018, leads to the following static checker warning:

drivers/crypto/chelsio/chtls/chtls_io.c:913 tls_header_read()
warn: signedness bug returning '(-14)'

drivers/crypto/chelsio/chtls/chtls_io.c
   909  /* Read TLS header to find content type and data length */
   910  static u16 tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)
   ^^^
   911  {
   912  if (copy_from_iter(thdr, sizeof(*thdr), from) != sizeof(*thdr))
   913  return -EFAULT;

This has a signedness bug and the caller doesn't check for errors.

   914  return (__force u16)cpu_to_be16(thdr->length);
   915  }

regards,
dan carpenter

[bug report] crypto: chtls - Inline TLS record Tx

[Dan Carpenter]

Hello Atul Gupta,

The patch 36bedb3f2e5b: "crypto: chtls - Inline TLS record Tx" from
Mar 31, 2018, leads to the following static checker warning:

drivers/crypto/chelsio/chtls/chtls_io.c:1210 chtls_sendpage()
info: ignoring unreachable code.

drivers/crypto/chelsio/chtls/chtls_io.c
  1188  skb->len += copy;
  1189  if (skb->len == mss)
  1190  tx_skb_finalize(skb);
  1191  skb->data_len += copy;
  1192  skb->truesize += copy;
  1193  sk->sk_wmem_queued += copy;
  1194  tp->write_seq += copy;
  1195  copied += copy;
  1196  offset += copy;
  1197  size -= copy;
  1198  
  1199  if (corked(tp, flags) &&
  1200  (sk_stream_wspace(sk) < sk_stream_min_wspace(sk)))
  1201  ULP_SKB_CB(skb)->flags |= ULPCB_FLAG_NO_APPEND;
  1202  
  1203  if (!size)
  1204  break;
  1205  
  1206  if (unlikely(ULP_SKB_CB(skb)->flags & 
ULPCB_FLAG_NO_APPEND))
  1207  push_frames_if_head(sk);
  1208  continue;

  1209  
  1210  set_bit(SOCK_NOSPACE, >sk_socket->flags);
^^^
Not reachable.

  1211  }
  1212  out:
  1213  csk_reset_flag(csk, CSK_TX_MORE_DATA);
  1214  if (copied)
  1215  chtls_tcp_push(sk, flags);
  1216  done:
  1217  release_sock(sk);
  1218  return copied;
  1219  
  1220  do_error:
  1221  if (copied)
  1222  goto out;
  1223  
  1224  out_err:
  1225  if (csk_conn_inline(csk))

regards,
dan carpenter

Re: [PATCH v15 0/9] Add io{read|write}64 to io-64-atomic headers

[Michael Ellerman]

Logan Gunthorpe  writes:

> This is v14 of my cleanup series to push a number of instances of people
> defining their own io{read|write}64 functions into common headers seing
> they don't exist in non-64bit systems. This series adds inline functions to 
> the
> io-64-nonatomic headers and then cleans up the drivers that defined their
> own copies.
>
> This cleanup was originally requested by Greg after he reviewed my
> Switchtec NTB code. And I hope someone can pick it up or at least give
> feedback on it soon as it's been around relatively unchanged for a few
> cycles now and I'm getting a bit tired of resubmitting it with little to
> no interest.

Yeah fair enough.

What's the plan for getting it merged? Seems we don't have one?

Given it touches two arches and generic stuff and drivers and crypto,
it's a bit of a mess to merge. It's not really something I want to merge
via the powerpc tree.

Is there any way to split it? I couldn't immediately see what the hard
dependencies were between the patches.

eg. It looks like I could take the two powerpc patches on their own for
4.17, and then the rest could go via other trees?

Is patch 1 stand alone?

The other option is to ask Andrew Morton to take it, as he often carries
these cross-tree type series.

cheers

> Changes since v14:
> - Rebased onto v4.16-rc7
> - Replace the first two patches so that instead of correcting the
>   endianness annotations we change to using writeX() and readX() with
>   swabX() calls. This makes the big-endian functions more symmetric
>   with the little-endian versions (with respect to barriers that are
>   not included in the raw functions). As a side effect, it also fixes
>   the kbuild warnings that the first two patches tried to address.
>
> Changes since v13:
> - Changed the subject of patch 0001 to correct a nit pointed out by Luc
>
> Changes since v12:
> - Rebased onto v4.16-rc6
> - Split patch 0001 into two and reworked the commit log as requested
>   by Luc Van Oostenryck
>
> Changes since v11:
> - Rebased onto v4.16-rc5
> - Added a patch (0001) to fix some old and new sparse warnings
>   that the kbuild robot warned about this cycle. The latest version
>   of sparse was required to reproduce these.
> - Added a patch (0002) to add io{read|write}64 to parisc which the kbuild
>   robot also found errors for this cycle
>
> Changes since v10:
> - Rebased onto v4.16-rc4, this droped the drm/tilcdc patch which was
>   picked up by that tree and is already in 4.16.
>
> Changes since v9:
> - Rebased onto v4.15-rc6
> - Fixed a couple of issues in the new version of the CAAM patch as
>   pointed out by Horia
>
> Changes since v8:
> - Rebased onto v4.15-rc2, as a result rewrote patch 7 seeing someone did
>   some similar cleanup in that area.
> - Added a patch to clean up the Switchtec NTB driver which landed in
>   v4.15-rc1
>
> Changes since v7:
> - Fix minor nits from Andy Shevchenko
> - Rebased onto v4.14-rc1
>
> Changes since v6:
>  ** none **
>
> Changes since v5:
> - Added a fix to the tilcdc driver to ensure it doesn't use the
>   non-atomic operation. (This includes adding 
> io{read|write}64[be]_is_nonatomic
>   defines).
>
> Changes since v4:
> - Add functions so the powerpc implementation of iomap.c compiles. (As
>   noticed by Horia)
>
> Changes since v3:
>
> - I noticed powerpc didn't use the appropriate functions seeing
>   readq/writeq were not defined when iomap.h was included. Thus I've
>   included a patch to adjust this
> - Fixed some mistakes with a couple of the defines in io-64-nonatomic*
>   headers
> - Fixed a typo noticed by Horia.
>
> (earlier versions were drastically different)
>
> --
>
> Logan Gunthorpe (9):
>   iomap: Use non-raw io functions for io{read|write}XXbe
>   parisc: iomap: introduce io{read|write}64
>   powerpc: io.h: move iomap.h include so that it can use readq/writeq
> defs
>   powerpc: iomap.c: introduce io{read|write}64_{lo_hi|hi_lo}
>   iomap: introduce io{read|write}64_{lo_hi|hi_lo}
>   io-64-nonatomic: add io{read|write}64[be]{_lo_hi|_hi_lo} macros
>   ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks
>   crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64
>   ntb: ntb_hw_switchtec: Cleanup 64bit IO defines to use the common
> header
>
>  arch/parisc/include/asm/io.h   |   9 +++
>  arch/parisc/lib/iomap.c|  64 +++
>  arch/powerpc/include/asm/io.h  |   6 +-
>  arch/powerpc/kernel/iomap.c|  40 ++
>  drivers/crypto/caam/regs.h |  30 +--
>  drivers/ntb/hw/intel/ntb_hw_intel.c|  30 +--
>  drivers/ntb/hw/mscc/ntb_hw_switchtec.c |  36 +
>  include/asm-generic/iomap.h|  26 --
>  include/linux/io-64-nonatomic-hi-lo.h  |  64 +++
>  include/linux/io-64-nonatomic-lo-hi.h  |  64 +++
>  lib/iomap.c| 140 
> -
>  11 files changed, 409 

Re: [crypto-chtls] Supicious code in chtls_io

[Atul Gupta]

On 4/4/2018 3:16 AM, Gustavo A. R. Silva wrote:
> Hi all,
>
> While doing some static analysis I came across the following piece of code at 
> drivers/crypto/chelsio/chtls/chtls_io.c:1203:
>
> 1203 if (!size)
> 1204 break;
> 1205 
> 1206 if (unlikely(ULP_SKB_CB(skb)->flags & 
> ULPCB_FLAG_NO_APPEND))
> 1207 push_frames_if_head(sk);
> 1208 continue;
> 1209 
> 1210 set_bit(SOCK_NOSPACE, >sk_socket->flags);
> 1211 }
>
>
> The issue is that in the code above, set_bit is never reached due to the 
> 'continue' statement at line 1208.
>
> I wonder if the actual intention of the code was something like this:
>
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c 
> b/drivers/crypto/chelsio/chtls/chtls_io.c
> index 5a75be4..a949a6c 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -1203,9 +1203,10 @@ int chtls_sendpage(struct sock *sk, struct page *page,
> if (!size)
> break;
>  
> -   if (unlikely(ULP_SKB_CB(skb)->flags & ULPCB_FLAG_NO_APPEND))
> +   if (unlikely(ULP_SKB_CB(skb)->flags & ULPCB_FLAG_NO_APPEND)) {
> push_frames_if_head(sk);
> -   continue;
> +   continue;
> +   }
>  
> set_bit(SOCK_NOSPACE, >sk_socket->flags);
> }
>
>
> What do you think?
Thanks for pointing, there is additional change required. I will send the patch 
once the window opens.
>
> I can send a proper patch for this.
>
> Thanks
> --
> Gustavo

Re: [v3] crypto: ctr - avoid VLA use

[Salvatore Mesoraca]

2018-04-03 23:37 GMT+02:00 Laura Abbott :
> On 03/30/2018 01:53 AM, Salvatore Mesoraca wrote:
>> ---
>>   crypto/ctr.c | 15 +--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/crypto/ctr.c b/crypto/ctr.c
>> index 854d924..49c469d 100644
>> --- a/crypto/ctr.c
>> +++ b/crypto/ctr.c
>> @@ -21,6 +21,9 @@
>>   #include 
>>   #include 
>>   +#define MAX_BLOCKSIZE 16
>> +#define MAX_ALIGNMASK 15
>> +
>
>
> Can we pull this out into a header file, I think this would cover
>
> crypto/cipher.c: In function ‘cipher_crypt_unaligned’:
> crypto/cipher.c:70:2: warning: ISO C90 forbids variable length array
> ‘buffer’ [-Wvla]
>   u8 buffer[size + alignmask];
>   ^~

Yeah, I'll send a patchset that includes the fix for crypto/cipher.c too.

Thank you for the suggestion :)

Salvatore