[PATCH] crypto: atmel - switch to SPDX license identifiers

[Tudor Ambarus]

Adopt the SPDX license identifiers to ease license compliance
management.

Signed-off-by: Tudor Ambarus 
---
 drivers/crypto/atmel-aes.c |  5 +
 drivers/crypto/atmel-authenc.h | 13 +
 drivers/crypto/atmel-ecc.c | 11 +--
 drivers/crypto/atmel-ecc.h | 14 +-
 drivers/crypto/atmel-sha.c |  5 +
 drivers/crypto/atmel-tdes.c|  5 +
 6 files changed, 6 insertions(+), 47 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 801aeab..2b7af44 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Cryptographic API.
  *
@@ -6,10 +7,6 @@
  * Copyright (c) 2012 Eukréa Electromatique - ATMEL
  * Author: Nicolas Royer 
  *
- * 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.
- *
  * Some ideas are from omap-aes.c driver.
  */
 
diff --git a/drivers/crypto/atmel-authenc.h b/drivers/crypto/atmel-authenc.h
index 2a60d12..cbd37a2 100644
--- a/drivers/crypto/atmel-authenc.h
+++ b/drivers/crypto/atmel-authenc.h
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * API for Atmel Secure Protocol Layers Improved Performances (SPLIP)
  *
@@ -5,18 +6,6 @@
  *
  * Author: Cyrille Pitchen 
  *
- * 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.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- *
  * This driver is based on drivers/mtd/spi-nor/fsl-quadspi.c from Freescale.
  */
 
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 74f083f..ba00e45 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -1,18 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Microchip / Atmel ECC (I2C) driver.
  *
  * Copyright (c) 2017, Microchip Technology Inc.
  * Author: Tudor Ambarus 
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
  */
 
 #include 
diff --git a/drivers/crypto/atmel-ecc.h b/drivers/crypto/atmel-ecc.h
index 25232c8..643a3b9 100644
--- a/drivers/crypto/atmel-ecc.h
+++ b/drivers/crypto/atmel-ecc.h
@@ -1,19 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Copyright (c) 2017, Microchip Technology Inc.
  * Author: Tudor Ambarus 
- *
- * 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.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- *
  */
 
 #ifndef __ATMEL_ECC_H__
diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index 8a19df2..ab0cfe7 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Cryptographic API.
  *
@@ -6,10 +7,6 @@
  * Copyright (c) 2012 Eukréa Electromatique - ATMEL
  * Author: Nicolas Royer 
  *
- * 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.
- *
  * Some ideas are from omap-sham.c drivers.
  */
 
diff --git a/drivers/crypto/atmel-tdes.c b/drivers/crypto/atmel-tdes.c
index 97b0423..438e1ff 100644
--- a/drivers/crypto/atmel-tdes.c
+++ b/drivers/crypto/atmel-tdes.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Cryptographic API.
  *
@@ -6,10 +7,6 @@
  * Copyright (c) 2012 Eukréa Electromatique - ATMEL
  * Author: Nicolas Royer 
  *
- * 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 Softwar

Re: [PATCH 9/9 v2] crypto: atmel-ecc: Break out lock check helper

[Tudor Ambarus]

On 06/28/2018 04:07 PM, Linus Walleij wrote:
> This breaks out a lock status checker to be used with further
> refactorings.
> 
> Signed-off-by: Linus Walleij 
> ---
> ChangeLog v1->v2:
> - Rebased
> ---
>  drivers/crypto/atmel-ecc.c | 38 ++
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index 0dfea6eadc56..549ec7190287 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -666,7 +666,9 @@ static int atmel_ecc_get_serial(struct i2c_client *client)
>   return ret;
>  }
>  
> -static int device_sanity_check(struct i2c_client *client)
> +static int atmel_ecc_get_lock_status(struct i2c_client *client,
> +  bool *config_is_locked,
> +  bool *otp_data_is_locked)
>  {
>   struct atmel_ecc_cmd *cmd;
>   int ret;
> @@ -680,21 +682,49 @@ static int device_sanity_check(struct i2c_client 
> *client)
>   ret = atmel_ecc_send_receive(client, cmd);
>   if (ret) {
>   dev_err(>dev,
> - "failed to send ECC init command\n");
> + "failed to send config read command\n");
>   goto free_cmd;
>   }
>  
> + /* According to datasheet anything else than 0x55 means "locked" */
> + if (cmd->data[RSP_DATA_IDX+3] == 0x55)
> + *config_is_locked = false;
> + else
> + *config_is_locked = true;
> +
> + if (cmd->data[RSP_DATA_IDX+2] == 0x55)
> + *otp_data_is_locked = false;
> + else
> + *otp_data_is_locked = true;
> +
> +free_cmd:
> + kfree(cmd);
> + return ret;
> +}
> +
> +static int device_sanity_check(struct i2c_client *client)
> +{
> + struct atmel_ecc_cmd *cmd;

when applying this patch you'll see that you will free this pointer without
allocating memory for it.

Best,
ta

> + bool config_locked;
> + bool otp_data_locked;
> + int ret;
> +
>   /*
>* It is vital that the Configuration, Data and OTP zones be locked
>* prior to release into the field of the system containing the device.
>* Failure to lock these zones may permit modification of any secret
>* keys and may lead to other security problems.
>*/
> - if (cmd->data[RSP_DATA_IDX+3] == 0x55) {
> + ret = atmel_ecc_get_lock_status(client, _locked,
> + _data_locked);
> + if (ret)
> + return ret;
> +
> + if (!config_locked) {
>   dev_err(>dev, "configuration zone is unlocked\n");
>   ret = -ENOTSUPP;
>   }
> - if (cmd->data[RSP_DATA_IDX+2] == 0x55) {
> + if (!otp_data_locked) {
>   dev_err(>dev, "data and OTP zones are unlocked\n");
>   ret = -ENOTSUPP;
>   }
> 

Re: [PATCH 7/9 v2] crypto: atmel-ecc: Print out serial number

[Tudor Ambarus]

Hi, Linus,

On 06/28/2018 04:07 PM, Linus Walleij wrote:
> This reads out the serial number of the crypto chip and prints it,
> also toss this into the entropy pool as it is device-unique data.
> 
> Signed-off-by: Linus Walleij 
> ---
> ChangeLog v1->v2:
> - kfree(cmd) was missed. Fix it with a goto construction.
> - Coding style fixes.
> ---
>  drivers/crypto/atmel-ecc.c | 60 ++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index f3322fae454e..659cb3cf37a9 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -610,6 +611,61 @@ static inline size_t atmel_ecc_wake_token_sz(u32 
> bus_clk_rate)
>   return DIV_ROUND_UP(no_of_bits, 8);
>  }
>  
> +static int atmel_ecc_get_serial(struct i2c_client *client)
> +{
> + struct atmel_ecc_cmd *cmd;
> + int ret;
> + int i;
> + u8 serial[9];
> +
> + cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
> + if (!cmd)
> + return -ENOMEM;
> +
> + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_0_3);
> + ret = atmel_ecc_send_receive(client, cmd);
> + if (ret) {
> + dev_err(>dev,
> + "failed to read serial byte 0-3\n");

the message can fit in the previous line.

> + goto out_free_cmd;
> + }
> + for (i = 0; i < 4; i++)
> + serial[i] = cmd->data[RSP_DATA_IDX + i];
> +
> + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_4_7);
> + ret = atmel_ecc_send_receive(client, cmd);
> + if (ret) {
> + dev_err(>dev,
> + "failed to read serial byte 4-7\n");
> + goto out_free_cmd;
> + }
> + for (i = 0; i < 4; i++)
> + serial[4 + i] = cmd->data[RSP_DATA_IDX + i];
> +

unnecessary blank line. Some of these coding style issues can be catch with
checkpatch.pl --strict. There are few coding style checks in the next 2 patches 
too.

> +
> + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_8_I2CEN);
> + ret = atmel_ecc_send_receive(client, cmd);
> + if (ret) {
> + kfree(cmd);

you will double free cmd, here and in out_free_cmd.

Otherwise looks good. The device supports 4-byte or 32-byte reads. You can also
do just one read of 32 bytes instead of three reads of four bytes each, but I
can't tell which one is better.

Thanks, Linus,
ta

> + dev_err(>dev,
> + "failed to read serial byte 8\n");
> + goto out_free_cmd;
> + }
> + serial[8] = cmd->data[RSP_DATA_IDX];
> +
> + /* This is device-unique data so it goes into the entropy pool */
> + add_device_randomness(serial, sizeof(serial));
> +
> + dev_info(>dev,
> +  "serial number: %02x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> +  serial[0], serial[1], serial[2], serial[3], serial[4],
> +  serial[5], serial[6], serial[7], serial[8]);
> +
> +out_free_cmd:
> + kfree(cmd);
> + return ret;
> +}
> +
>  static int device_sanity_check(struct i2c_client *client)
>  {
>   struct atmel_ecc_cmd *cmd;
> @@ -692,6 +748,10 @@ static int atmel_ecc_probe(struct i2c_client *client,
>   if (ret)
>   return ret;
>  
> + ret = atmel_ecc_get_serial(client);
> + if (ret)
> + return ret;
> +
>   spin_lock(_data.i2c_list_lock);
>   list_add_tail(_priv->i2c_client_list_node,
> _data.i2c_client_list);
> 

Re: [PATCH 6/9 v2] crypto: atmel-ecc: Marshal the command while sending

[Tudor Ambarus]

Hi, Linus,

On 06/28/2018 04:07 PM, Linus Walleij wrote:
> Instead of casting the struct for the command into (u8 *)
> which is problematic in many ways, and instead of
> calculating the CRC sum in a separate function, marshal,
> checksum and send the command in one single function.
> 
> Instead of providing the length of the whole command
> in defines, it makes more sense to provide the length
> of the data buffer used with the command.
> 
> This avoids the hazzle to try to keep the command
> structure in the device endianness, we fix up the
> endianness when marshalling the command instead.
> 
> Signed-off-by: Linus Walleij 
> ---
> ChangeLog v1->v2:
> - Rebase on other changes.
> ---
>  drivers/crypto/atmel-ecc.c | 72 ++
>  drivers/crypto/atmel-ecc.h | 13 +++
>  2 files changed, 41 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index 2ec570d06a27..f3322fae454e 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -113,29 +113,6 @@ struct atmel_ecc_work_data {
>   struct atmel_ecc_cmd cmd;
>  };
>  
> -static u16 atmel_ecc_crc16(u16 crc, const u8 *buffer, size_t len)
> -{
> - return cpu_to_le16(bitrev16(crc16(crc, buffer, len)));
> -}
> -
> -/**
> - * atmel_ecc_checksum() - Generate 16-bit CRC as required by ATMEL ECC.
> - * CRC16 verification of the count, opcode, param1, param2 and data bytes.
> - * The checksum is saved in little-endian format in the least significant
> - * two bytes of the command. CRC polynomial is 0x8005 and the initial 
> register
> - * value should be zero.
> - *
> - * @cmd : structure used for communicating with the device.
> - */
> -static void atmel_ecc_checksum(struct atmel_ecc_cmd *cmd)
> -{
> - u8 *data = >count;
> - size_t len = cmd->count - CRC_SIZE;
> - u16 *crc16 = (u16 *)(data + len);
> -
> - *crc16 = atmel_ecc_crc16(0, data, len);
> -}
> -
>  static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,
>   u16 config_word)
>  {
> @@ -143,10 +120,7 @@ static void atmel_ecc_init_read_config_word(struct 
> atmel_ecc_cmd *cmd,
>   cmd->opcode = OPCODE_READ;
>   cmd->param1 = CONFIG_ZONE;
>   cmd->param2 = config_word;
> - cmd->count = READ_COUNT;
> -
> - atmel_ecc_checksum(cmd);
> -
> + cmd->datasz = READ_DATASZ;
>   cmd->msecs = MAX_EXEC_TIME_READ;
>   cmd->rxsize = READ_RSP_SIZE;
>  }
> @@ -154,14 +128,11 @@ static void atmel_ecc_init_read_config_word(struct 
> atmel_ecc_cmd *cmd,
>  static void atmel_ecc_init_genkey_cmd(struct atmel_ecc_cmd *cmd, u16 keyid)
>  {
>   cmd->word_addr = COMMAND;
> - cmd->count = GENKEY_COUNT;
> + cmd->datasz = GENKEY_DATASZ;
>   cmd->opcode = OPCODE_GENKEY;
>   cmd->param1 = GENKEY_MODE_PRIVATE;
>   /* a random private key will be generated and stored in slot keyID */
> - cmd->param2 = cpu_to_le16(keyid);
> -
> - atmel_ecc_checksum(cmd);
> -
> + cmd->param2 = keyid;
>   cmd->msecs = MAX_EXEC_TIME_GENKEY;
>   cmd->rxsize = GENKEY_RSP_SIZE;
>  }
> @@ -172,11 +143,11 @@ static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd 
> *cmd,
>   size_t copied;
>  
>   cmd->word_addr = COMMAND;
> - cmd->count = ECDH_COUNT;
> + cmd->datasz = ECDH_DATASZ;
>   cmd->opcode = OPCODE_ECDH;
>   cmd->param1 = ECDH_PREFIX_MODE;
>   /* private key slot */
> - cmd->param2 = cpu_to_le16(DATA_SLOT_2);
> + cmd->param2 = DATA_SLOT_2;
>  
>   /*
>* The device only supports NIST P256 ECC keys. The public key size will
> @@ -186,9 +157,6 @@ static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd 
> *cmd,
>   copied = sg_copy_to_buffer(pubkey, 1, cmd->data, ATMEL_ECC_PUBKEY_SIZE);
>   if (copied != ATMEL_ECC_PUBKEY_SIZE)
>   return -EINVAL;
> -
> - atmel_ecc_checksum(cmd);
> -
>   cmd->msecs = MAX_EXEC_TIME_ECDH;
>   cmd->rxsize = ECDH_RSP_SIZE;
>  
> @@ -302,7 +270,11 @@ static int atmel_ecc_send_receive(struct i2c_client 
> *client,
> struct atmel_ecc_cmd *cmd)
>  {
>   struct atmel_ecc_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
> + u8 buf[MAX_CMD_SIZE];
> + u16 cmdcrc;
> + u8 cmdlen;
>   int ret;
> + int i;
>  
>   mutex_lock(_priv->lock);
>  
> @@ -312,7 +284,31 @@ static int atmel_ecc_send_receive(struct i2c_client 
> *client,
>   goto err;
>   }
>  
> - ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
> + /* Marshal the command */
> + cmdlen = 6 + cmd->datasz + CRC_SIZE;
> + buf[0] = cmd->word_addr;
> + /* This excludes the word address, includes CRC */
> + buf[1] = cmdlen - 1;
> + buf[2] = cmd->opcode;
> + buf[3] = cmd->param1;
> + /* Enforce little-endian byte order */
> + buf[4] = cmd->param2 & 0xff;
> + buf[5] = (cmd->param2 >> 8);
> + /* Copy over the 

Re: [PATCH 3/9 v2] crypto: atmel-ecc: More helpful error messages

[Tudor Ambarus]

Hi, Linus,

On 06/28/2018 04:07 PM, Linus Walleij wrote:
> Report errors once when they happen on the I2C bus so we
> get good information in cases such as when the wrong I2C
> address is used.
> 
> Signed-off-by: Linus Walleij 
> ---
> ChangeLog v1->v2:
> - Strip some comments that are now obvious from the context
>   with the error messages.
> - Do not print the excess ECC error message,  atmel_ecc_status()
>   already prints error messages.
> ---
>  drivers/crypto/atmel-ecc.c | 24 
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index 33773920e4bf..baef0d07164d 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -310,26 +310,31 @@ static int atmel_ecc_send_receive(struct i2c_client 
> *client,
>   mutex_lock(_priv->lock);
>  
>   ret = atmel_ecc_wakeup(client);
> - if (ret)
> + if (ret) {
> + dev_err(>dev, "wakeup failed\n");
>   goto err;
> + }
>  
> - /* send the command */
>   ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(>dev, "command send failed\n");
>   goto err;
> + }
>  
>   /* delay the appropriate amount of time for command to execute */
>   msleep(cmd->msecs);
>  
> - /* receive the response */
>   ret = i2c_master_recv(client, cmd->data, cmd->rxsize);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(>dev, "getting response failed\n");
>   goto err;
> + }
>  
> - /* put the device into low-power mode */
>   ret = atmel_ecc_sleep(client);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(>dev, "putting to sleep failed\n");
>   goto err;
> + }
>  
>   mutex_unlock(_priv->lock);
>   return atmel_ecc_status(>dev, cmd->data);
> @@ -624,8 +629,11 @@ static int device_sanity_check(struct i2c_client *client)
>   atmel_ecc_init_read_cmd(cmd);
>  
>   ret = atmel_ecc_send_receive(client, cmd);
> - if (ret)
> + if (ret) {
> + dev_err(>dev,

If you want to have two errors reported, one related to i2c and the other to
crypto, I propose to be consistent and add an error message for the other call
to atmel_ecc_send_receive() in atmel_ecdh_set_secret().


> + "failed to send ECC init command\n");

This line fits well in the previous. I would remove "ECC" from it, looks a bit
redundant:
atmel-ecc 2-0058: failed to send ECC init command

Thanks!
ta

>   goto free_cmd;
> + }
>  
>   /*
>* It is vital that the Configuration, Data and OTP zones be locked
> 

Re: [PATCH 2/9 v2] crypto: atmel-ecc: Just warn on missing clock frequency

[Tudor Ambarus]

Hi, Linus,

On 06/28/2018 04:07 PM, Linus Walleij wrote:
> The Atmel ECC driver contains a check for the I2C bus clock
> frequency, so as to check that the I2C adapter in use
> satisfies the device specs.
> 
> If the device is connected to a device tree node that does not
> contain a clock frequency setting, such as an I2C mux or gate,
> this blocks the probe. Make the probe continue with a warning
> if no clock frequency can be found, assuming it is safe.
> 
> Signed-off-by: Linus Walleij 
> ---
> ChangeLog v1->v2:
> - Instead of silently ignoring the missing clock frequency,
>   issue a warning and continue.
> ---
>  drivers/crypto/atmel-ecc.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
> index e66f18a0ddd0..33773920e4bf 100644
> --- a/drivers/crypto/atmel-ecc.c
> +++ b/drivers/crypto/atmel-ecc.c
> @@ -659,12 +659,9 @@ static int atmel_ecc_probe(struct i2c_client *client,
>  
>   ret = of_property_read_u32(client->adapter->dev.of_node,
>  "clock-frequency", _clk_rate);
> - if (ret) {
> - dev_err(dev, "of: failed to read clock-frequency property\n");
> - return ret;
> - }
> -> -  if (bus_clk_rate > 100L) {
> + if (ret)
> + dev_warn(dev, "i2c host missing clock frequency information\n");

Not ok, bus_clk_rate is used to compute the wake_token. If you can't find the
clock-frequency then you will use the stack value of bus_clk_rate. If you don't
want to break the probe, then I propose to do the same assumption that is done
in i2c buses when clock-frequency is not found: assume it is 100kHZ. Although,
personally, I don't like this assumption and that's why I proposed that patch to
Wolfram. Nevertheless, let's do what we can in this conditions: make the same
assumption.

Thanks,
ta

> + else if (bus_clk_rate > 100L) {
>   dev_err(dev, "%d exceeds maximum supported clock frequency 
> (1MHz)\n",
>   bus_clk_rate);
>   return -EINVAL;
> 

[PATCH] crypto: atmel-ecc - fix to allow multi segment scatterlists

[Tudor Ambarus]

Remove the limitation of single element scatterlists. ECDH with
multi-element scatterlists is needed by TPM.

Similar to 'commit 95ec01ba1ef0 ("crypto: ecdh - fix to allow multi
segment scatterlists")'.

Signed-off-by: Tudor Ambarus 
---
 drivers/crypto/atmel-ecc.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index e66f18a0..a25772e 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -186,7 +186,10 @@ static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd 
*cmd,
 * always be the same. Use a macro for the key size to avoid unnecessary
 * computations.
 */
-   copied = sg_copy_to_buffer(pubkey, 1, cmd->data, ATMEL_ECC_PUBKEY_SIZE);
+   copied = sg_copy_to_buffer(pubkey,
+  sg_nents_for_len(pubkey,
+   ATMEL_ECC_PUBKEY_SIZE),
+  cmd->data, ATMEL_ECC_PUBKEY_SIZE);
if (copied != ATMEL_ECC_PUBKEY_SIZE)
return -EINVAL;
 
@@ -268,15 +271,17 @@ static void atmel_ecdh_done(struct atmel_ecc_work_data 
*work_data, void *areq,
struct kpp_request *req = areq;
struct atmel_ecdh_ctx *ctx = work_data->ctx;
struct atmel_ecc_cmd *cmd = _data->cmd;
-   size_t copied;
-   size_t n_sz = ctx->n_sz;
+   size_t copied, n_sz;
 
if (status)
goto free_work_data;
 
+   /* might want less than we've got */
+   n_sz = min_t(size_t, ctx->n_sz, req->dst_len);
+
/* copy the shared secret */
-   copied = sg_copy_from_buffer(req->dst, 1, >data[RSP_DATA_IDX],
-n_sz);
+   copied = sg_copy_from_buffer(req->dst, sg_nents_for_len(req->dst, n_sz),
+>data[RSP_DATA_IDX], n_sz);
if (copied != n_sz)
status = -EINVAL;
 
@@ -440,7 +445,7 @@ static int atmel_ecdh_generate_public_key(struct 
kpp_request *req)
 {
struct crypto_kpp *tfm = crypto_kpp_reqtfm(req);
struct atmel_ecdh_ctx *ctx = kpp_tfm_ctx(tfm);
-   size_t copied;
+   size_t copied, nbytes;
int ret = 0;
 
if (ctx->do_fallback) {
@@ -448,10 +453,14 @@ static int atmel_ecdh_generate_public_key(struct 
kpp_request *req)
return crypto_kpp_generate_public_key(req);
}
 
+   /* might want less than we've got */
+   nbytes = min_t(size_t, ATMEL_ECC_PUBKEY_SIZE, req->dst_len);
+
/* public key was saved at private key generation */
-   copied = sg_copy_from_buffer(req->dst, 1, ctx->public_key,
-ATMEL_ECC_PUBKEY_SIZE);
-   if (copied != ATMEL_ECC_PUBKEY_SIZE)
+   copied = sg_copy_from_buffer(req->dst,
+sg_nents_for_len(req->dst, nbytes),
+ctx->public_key, nbytes);
+   if (copied != nbytes)
ret = -EINVAL;
 
return ret;
@@ -470,6 +479,10 @@ static int atmel_ecdh_compute_shared_secret(struct 
kpp_request *req)
return crypto_kpp_compute_shared_secret(req);
}
 
+   /* must have exactly two points to be on the curve */
+   if (req->src_len != ATMEL_ECC_PUBKEY_SIZE)
+   return -EINVAL;
+
gfp = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ? GFP_KERNEL :
 GFP_ATOMIC;
 
-- 
2.9.4

[PATCH] crypto: atmel-ecc - remove overly verbose dev_info

[Tudor Ambarus]

Remove it because when using a slow console, it can affect
the speed of crypto operations.

Similar to 'commit 730f23b66095 ("crypto: vmx - Remove overly
verbose printk from AES XTS init")'.

Signed-off-by: Tudor Ambarus 
---
 drivers/crypto/atmel-ecc.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index a25772e..74f083f 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -567,10 +567,6 @@ static int atmel_ecdh_init_tfm(struct crypto_kpp *tfm)
}
 
crypto_kpp_set_flags(fallback, crypto_kpp_get_flags(tfm));
-
-   dev_info(>client->dev, "Using '%s' as fallback implementation.\n",
-crypto_tfm_alg_driver_name(crypto_kpp_tfm(fallback)));
-
ctx->fallback = fallback;
 
return 0;
-- 
2.9.4

Re: [PATCH 8/9] crypto: atmel-ecc: Detail what is unlocked

[Tudor Ambarus]

Hi, Linus,

On 06/05/2018 04:49 PM, Linus Walleij wrote:

Instead of just providing a broad error message about the
chip being unlocked provide details on what is unlocked,
one line per thing that can be locked: data and OTP and
configuration are locked independently. Loose the

Failure to lock these zones may permit modification of secret keys, so
we don't permit using the device if any of these zones is unlocked.
Why do you think it's relevant to indicate which zone is unlocked?

Thanks,
ta

Re: [PATCH 7/9] crypto: atmel-ecc: Print out serial number

[Tudor Ambarus]

Hi, Linus,

On 06/05/2018 04:49 PM, Linus Walleij wrote:

This reads out the serial number of the crypto chip and prints it,
also toss this into the entropy pool as it is device-unique data.

Signed-off-by: Linus Walleij 
---
  drivers/crypto/atmel-ecc.c | 56 ++
  1 file changed, 56 insertions(+)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 603e29bdcb97..fd8149313104 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -29,6 +29,7 @@
  #include 
  #include 
  #include 
+#include 


includes should be ordered alphabetically.


  #include 
  #include 
  #include 
@@ -616,6 +617,57 @@ static inline size_t atmel_ecc_wake_token_sz(u32 
bus_clk_rate)
return DIV_ROUND_UP(no_of_bits, 8);
  }
  
+static int atmel_ecc_get_serial(struct i2c_client *client)

+{
+   struct atmel_ecc_cmd *cmd;
+   u8 serial[9];


int i, ret; before serial to avoid stack padding.


+   int ret;
+   int i;
+
+   cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
+   if (!cmd)
+   return -ENOMEM;
+
+   atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_0_3);
+   ret = atmel_ecc_send_receive(client, cmd);
+   if (ret) {


free(cmd);


+   dev_err(>dev,
+   "failed to read serial byte 0-3\n");
+   return ret;
+   }
+   for (i = 0; i < 4; i++)
+   serial[0+i] = cmd->data[RSP_DATA_IDX + i];


serial[i]?


+
+   atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_4_7);


can't we read the serial_no once?


+   ret = atmel_ecc_send_receive(client, cmd);
+   if (ret) {
+   dev_err(>dev,
+   "failed to read serial byte 4-7\n");
+   return ret;
+   }
+   for (i = 0; i < 4; i++)
+   serial[4+i] = cmd->data[RSP_DATA_IDX + i];


spaces preferred around that '+'


+
+
+   atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_8_I2CEN);
+   ret = atmel_ecc_send_receive(client, cmd);
+   if (ret) {
+   dev_err(>dev,
+   "failed to read serial byte 8\n");
+   return ret;
+   }
+   serial[8] = cmd->data[RSP_DATA_IDX];
+
+   /* This is device-unique data so it goes into the entropy pool */
+   add_device_randomness(serial, sizeof(serial));
+
+   dev_info(>dev,
+"serial number: %02x%02x%02x%02x%02x%02x%02x%02x%02x\n",
+serial[0], serial[1], serial[2], serial[3], serial[4],
+serial[5], serial[6], serial[7], serial[8]);


Why do you need the serial number printed out?


+   return 0;
+}
+
  static int device_sanity_check(struct i2c_client *client)
  {
struct atmel_ecc_cmd *cmd;
@@ -700,6 +752,10 @@ static int atmel_ecc_probe(struct i2c_client *client,
if (ret)
return ret;
  
+	ret = atmel_ecc_get_serial(client);

+   if (ret)
+   return ret;
+
spin_lock(_data.i2c_list_lock);
list_add_tail(_priv->i2c_client_list_node,
  _data.i2c_client_list);

Re: [PATCH 4/9] crypto: atmel-ecc: Provide config zone defines

[Tudor Ambarus]

Hi, Linus,

On 06/05/2018 04:49 PM, Linus Walleij wrote:

The config zone has 0x16 words of 4 bytes each, so provide
some basic defines so that we can address these individually.


Are you going to use all these defines? I would add just the defines
that are needed, when they are needed, but I guess it's a matter of
preference.

Thanks,
ta


Rename the last word to "footer", this is where we currently
look to see if the configuration is locked.

Signed-off-by: Linus Walleij 
---
  drivers/crypto/atmel-ecc.c |  2 +-
  drivers/crypto/atmel-ecc.h | 25 +++--
  2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 214b0572bf8b..f1f422385a91 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -145,7 +145,7 @@ static void atmel_ecc_init_read_cmd(struct atmel_ecc_cmd 
*cmd)
 * (UserExtra, Selector, LockValue, LockConfig).
 */
cmd->param1 = CONFIG_ZONE;
-   cmd->param2 = DEVICE_LOCK_ADDR;
+   cmd->param2 = CONFIG_ZONE_FOOTER;
cmd->count = READ_COUNT;
  
  	atmel_ecc_checksum(cmd);

diff --git a/drivers/crypto/atmel-ecc.h b/drivers/crypto/atmel-ecc.h
index 25232c8abcc2..6d586a3e443d 100644
--- a/drivers/crypto/atmel-ecc.h
+++ b/drivers/crypto/atmel-ecc.h
@@ -89,8 +89,29 @@ static const struct {
  #define RSP_DATA_IDX  1 /* buffer index of data in response */
  #define DATA_SLOT_2   2 /* used for ECDH private key */
  
-/* Definitions for the device lock state */

-#define DEVICE_LOCK_ADDR   0x15
+/* Definitions for the configuration zone words, these are 4 bytes each */
+#define CONFIG_ZONE_SERIAL_0_3 0x00
+#define CONFIG_ZONE_REVISION   0x01
+#define CONFIG_ZONE_SERIAL_4_7 0x02
+#define CONFIG_ZONE_SERIAL_8_I2CEN 0x03
+#define CONFIG_ZONE_I2C_OTP0x04
+#define CONFIG_ZONE_SLOT_0_1   0x05
+#define CONFIG_ZONE_SLOT_2_3   0x06
+#define CONFIG_ZONE_SLOT_4_5   0x07
+#define CONFIG_ZONE_SLOT_6_7   0x08
+#define CONFIG_ZONE_SLOT_8_9   0x09
+#define CONFIG_ZONE_SLOT_10_11 0x0a
+#define CONFIG_ZONE_SLOT_12_13 0x0b
+#define CONFIG_ZONE_SLOT_14_15 0x0c
+#define CONFIG_ZONE_FLAG_0_1   0x0d
+#define CONFIG_ZONE_FLAG_2_3   0x0e
+#define CONFIG_ZONE_FLAG_4_5   0x0f
+#define CONFIG_ZONE_FLAG_6_7   0x10
+#define CONFIG_ZONE_LKU_0_10x11
+#define CONFIG_ZONE_LKU_2_30x12
+#define CONFIG_ZONE_LKU_4_50x13
+#define CONFIG_ZONE_LKU_6_70x14
+#define CONFIG_ZONE_FOOTER 0x15
  #define LOCK_VALUE_IDX(RSP_DATA_IDX + 2)
  #define LOCK_CONFIG_IDX   (RSP_DATA_IDX + 3)
  

Re: [PATCH 3/9] crypto: atmel-ecc: More helpful error messages

[Tudor Ambarus]

Hi, Linus,

On 06/05/2018 04:49 PM, Linus Walleij wrote:

Report errors once when they happen on the I2C bus so we
get good information in cases such as when the wrong I2C
address is used.

Signed-off-by: Linus Walleij 
---
  drivers/crypto/atmel-ecc.c | 27 +--
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 145ab3a39a56..214b0572bf8b 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -310,29 +310,41 @@ static int atmel_ecc_send_receive(struct i2c_client 
*client,
mutex_lock(_priv->lock);
  
  	ret = atmel_ecc_wakeup(client);

-   if (ret)
+   if (ret) {
+   dev_err(>dev, "wakeup failed\n");
goto err;
+   }
  
  	/* send the command */


I guess that this comment will become superfluous if you're going to add
an error message.


ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
-   if (ret < 0)
+   if (ret < 0) {
+   dev_err(>dev, "command send failed\n");
goto err;
+   }
  
  	/* delay the appropriate amount of time for command to execute */

msleep(cmd->msecs);
  
  	/* receive the response */

ret = i2c_master_recv(client, cmd->data, cmd->rxsize);
-   if (ret < 0)
+   if (ret < 0) {
+   dev_err(>dev, "getting response failed\n");
goto err;
+   }
  
  	/* put the device into low-power mode */

ret = atmel_ecc_sleep(client);
-   if (ret < 0)
+   if (ret < 0) {
+   dev_err(>dev, "putting to sleep failed\n");
goto err;
+   }
  
  	mutex_unlock(_priv->lock);

-   return atmel_ecc_status(>dev, cmd->data);
+   ret = atmel_ecc_status(>dev, cmd->data);


atmel_ecc_status already prints errors when needed.


+   if (ret < 0)
+   dev_err(>dev, "ECC status parse failed\n");
+
+   return ret;
  err:
mutex_unlock(_priv->lock);
return ret;
@@ -624,8 +636,11 @@ static int device_sanity_check(struct i2c_client *client)
atmel_ecc_init_read_cmd(cmd);
  
  	ret = atmel_ecc_send_receive(client, cmd);

-   if (ret)
+   if (ret) {
+   dev_err(>dev,
+   "failed to send ECC init command\n");


Here we will have two errors reported, the first being from the
atmel_ecc_send_receive(). I would go with just an error reported. Do we
really care what happened with the i2c transfer, or it's enough to
report that the init failed?

Thanks,
ta


goto free_cmd;
+   }
  
  	/*

 * It is vital that the Configuration, Data and OTP zones be locked

Re: [PATCH 1/9] crypto: atmel-ecc: Make available for other platforms

[Tudor Ambarus]

On 06/05/2018 04:49 PM, Linus Walleij wrote:

This is a pure I2C driver, and this device appears on the
96boards Secure96 mezzanine card, so we want to enable the
driver on other devices. Cut the Kconfig limitations to
Atmel SoC only.

Signed-off-by: Linus Walleij 


Reviewed-by: Tudor Ambarus 

Thanks,
ta


---
  drivers/crypto/Kconfig | 1 -
  1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index d1ea1a07cecb..9e33f2de8dae 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -522,7 +522,6 @@ config CRYPTO_DEV_ATMEL_SHA
  
  config CRYPTO_DEV_ATMEL_ECC

tristate "Support for Microchip / Atmel ECC hw accelerator"
-   depends on ARCH_AT91 || COMPILE_TEST
depends on I2C
select CRYPTO_ECDH
select CRC16

Re: [PATCH 6/9] crypto: atmel-ecc: Marshal the command while sending

[Tudor Ambarus]

Hi, Linus,

On 06/05/2018 04:49 PM, Linus Walleij wrote:

Instead of casting the struct for the command into (u8 *)
which is problematic in many ways, and instead of
calculating the CRC sum in a separate function, marshal,
checksum and send the command in one single function.

Instead of providing the length of the whole command
in defines, it makes more sense to provide the length
of the data buffer used with the command.

This avoids the hazzle to try to keep the command
structure in the device endianness, we fix up the
endianness when marshalling the command instead.

Signed-off-by: Linus Walleij 
---
  drivers/crypto/atmel-ecc.c | 71 ++
  drivers/crypto/atmel-ecc.h | 13 +++
  2 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index d89b69d228ac..603e29bdcb97 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -113,29 +113,6 @@ struct atmel_ecc_work_data {
struct atmel_ecc_cmd cmd;
  };
  
-static u16 atmel_ecc_crc16(u16 crc, const u8 *buffer, size_t len)

-{
-   return cpu_to_le16(bitrev16(crc16(crc, buffer, len)));
-}
-
-/**
- * atmel_ecc_checksum() - Generate 16-bit CRC as required by ATMEL ECC.
- * CRC16 verification of the count, opcode, param1, param2 and data bytes.
- * The checksum is saved in little-endian format in the least significant
- * two bytes of the command. CRC polynomial is 0x8005 and the initial register
- * value should be zero.
- *
- * @cmd : structure used for communicating with the device.
- */
-static void atmel_ecc_checksum(struct atmel_ecc_cmd *cmd)
-{
-   u8 *data = >count;
-   size_t len = cmd->count - CRC_SIZE;
-   u16 *crc16 = (u16 *)(data + len);
-
-   *crc16 = atmel_ecc_crc16(0, data, len);
-}
-
  static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,
u16 config_word)
  {
@@ -143,10 +120,7 @@ static void atmel_ecc_init_read_config_word(struct 
atmel_ecc_cmd *cmd,
cmd->opcode = OPCODE_READ;
cmd->param1 = CONFIG_ZONE;
cmd->param2 = config_word;
-   cmd->count = READ_COUNT;
-
-   atmel_ecc_checksum(cmd);
-
+   cmd->datasz = READ_DATASZ;
cmd->msecs = MAX_EXEC_TIME_READ;
cmd->rxsize = READ_RSP_SIZE;
  }
@@ -154,14 +128,11 @@ static void atmel_ecc_init_read_config_word(struct 
atmel_ecc_cmd *cmd,
  static void atmel_ecc_init_genkey_cmd(struct atmel_ecc_cmd *cmd, u16 keyid)
  {
cmd->word_addr = COMMAND;
-   cmd->count = GENKEY_COUNT;
+   cmd->datasz = GENKEY_DATASZ;
cmd->opcode = OPCODE_GENKEY;
cmd->param1 = GENKEY_MODE_PRIVATE;
/* a random private key will be generated and stored in slot keyID */
-   cmd->param2 = cpu_to_le16(keyid);
-
-   atmel_ecc_checksum(cmd);
-
+   cmd->param2 = keyid;
cmd->msecs = MAX_EXEC_TIME_GENKEY;
cmd->rxsize = GENKEY_RSP_SIZE;
  }
@@ -172,11 +143,11 @@ static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd 
*cmd,
size_t copied;
  
  	cmd->word_addr = COMMAND;

-   cmd->count = ECDH_COUNT;
+   cmd->datasz = ECDH_DATASZ;
cmd->opcode = OPCODE_ECDH;
cmd->param1 = ECDH_PREFIX_MODE;
/* private key slot */
-   cmd->param2 = cpu_to_le16(DATA_SLOT_2);
+   cmd->param2 = DATA_SLOT_2;
  
  	/*

 * The device only supports NIST P256 ECC keys. The public key size will
@@ -186,9 +157,6 @@ static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd 
*cmd,
copied = sg_copy_to_buffer(pubkey, 1, cmd->data, ATMEL_ECC_PUBKEY_SIZE);
if (copied != ATMEL_ECC_PUBKEY_SIZE)
return -EINVAL;
-
-   atmel_ecc_checksum(cmd);
-
cmd->msecs = MAX_EXEC_TIME_ECDH;
cmd->rxsize = ECDH_RSP_SIZE;
  
@@ -302,7 +270,11 @@ static int atmel_ecc_send_receive(struct i2c_client *client,

  struct atmel_ecc_cmd *cmd)
  {
struct atmel_ecc_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
+   u8 buf[MAX_CMD_SIZE];
+   u16 cmdcrc;
+   u8 cmdlen;
int ret;
+   int i;
  
  	mutex_lock(_priv->lock);
  
@@ -312,8 +284,31 @@ static int atmel_ecc_send_receive(struct i2c_client *client,

goto err;
}
  
+	/* Marshal the command */

+   cmdlen = 6 + cmd->datasz + CRC_SIZE;
+   buf[0] = cmd->word_addr;
+   /* This excludes the word address, includes CRC */
+   buf[1] = cmdlen - 1;
+   buf[2] = cmd->opcode;
+   buf[3] = cmd->param1;
+   /* Enforce little-endian byte order */
+   buf[4] = cmd->param2 & 0xff;
+   buf[5] = (cmd->param2 >> 8);
+   /* Copy over the data array */
+   for (i = 0; i < cmd->datasz; i++)
+   buf[6+i] = cmd->data[i];
+   /*
+* CRC sum the command, do not include word addr or CRC. The
+* bit order in the CRC16 algorithm inside the chip is reversed,
+* so we need to swizzle 

Re: [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency

[Tudor Ambarus]

Hi, Linus, Wolfram,

On 06/05/2018 04:49 PM, Linus Walleij wrote:

The Atmel ECC driver contains a check for the I2C bus clock
frequency, so as to check that the I2C adapter in use
satisfies the device specs.

If the device is connected to a device tree node that does not
contain a clock frequency setting, such as an I2C mux or gate,
this blocks the probe. Make the probe continue silently if
no clock frequency can be found, assuming all is safe.


I don't think it's safe. We use bus_clk_rate to compute the ecc508's
wake token. If you can't wake the device, it will ignore all your
commands.

My proposal was to introduce a bus_freq_hz member in the i2c_adapter
structure (https://patchwork.kernel.org/patch/10307637/), but I haven't
received feedback yet, Wolfram?

I would say first to check the bus_freq_hz from adapter (if it will be
accepted) else to look into dt and, in the last case, if nowhere
provided, to block the probe.

Thanks,
ta



Signed-off-by: Linus Walleij 
---
  drivers/crypto/atmel-ecc.c | 11 +--
  1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index e66f18a0ddd0..145ab3a39a56 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -657,14 +657,13 @@ static int atmel_ecc_probe(struct i2c_client *client,
return -ENODEV;
}
  
+	/*

+* Silently assume all is fine if there is no
+* "clock-frequency" property.
+*/
ret = of_property_read_u32(client->adapter->dev.of_node,
   "clock-frequency", _clk_rate);
-   if (ret) {
-   dev_err(dev, "of: failed to read clock-frequency property\n");
-   return ret;
-   }
-
-   if (bus_clk_rate > 100L) {
+   if (!ret && (bus_clk_rate > 100L)) {
dev_err(dev, "%d exceeds maximum supported clock frequency 
(1MHz)\n",
bus_clk_rate);
return -EINVAL;

Re: [RFC PATCH 5/5] KEYS: add KPP ecdh parser

[Tudor Ambarus]

Hi, Denis,

On 05/14/2018 10:54 PM, Denis Kenzior wrote:

Hi Tudor,

On 02/28/2018 10:52 AM, Tudor Ambarus wrote:

The ECDH private keys are expected to be encoded with the ecdh
helpers from kernel.

Use the ecdh helpers to check if the key is valid. If valid,
allocate a tfm and set the private key. There is a one-to-one
binding between the private key and the tfm. The tfm is allocated
once and used as many times as needed.

ECDH keys can be loaded like this:
 # echo -n 
020028000200200024d121ebe5cf2d83f6621b6e43843aa38be086c32019da92505303e1c0eab882 
\


This part looks a bit scary.  Isn't this translating directly to 
kpp_secret data structure (in looks like little-endian order) followed 
curve_id, etc. >


yes, this is how it works.

If the intent is to extend KPP with regular DH, DH + KDF, etc, then we 
might want to invent a proper format here?  I don't think that a Diffie 
Hellman or ECDH Private Key format was ever invented, similar to how 
PKCS8 is used for RSA.




This can be resolved by falling through kpp decoding types until one
recognizes the format.

Inventing an ASN.1 syntax would be logical but somewhat painful as D-H 
is frequently used with plain old random numbers and certificates are 
not stored on disk...


There was this kind of discussion when kpp was introduced, see:
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19599.html

Best,
ta




 | xxd -r -p | keyctl padd asymmetric private @s

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
  crypto/asymmetric_keys/Kconfig  |   8 +++
  crypto/asymmetric_keys/Makefile |   5 ++
  crypto/asymmetric_keys/kpp_parser.c | 124 


  include/crypto/asym_kpp_subtype.h   |   2 +
  4 files changed, 139 insertions(+)
  create mode 100644 crypto/asymmetric_keys/kpp_parser.c


Regards,
-Denis

Re: [RFC PATCH 1/5] KEYS: Provide key type operations for kpp ops

[Tudor Ambarus]

Hi, Denis,

Thanks for the review! Please see inline.

On 05/14/2018 09:48 PM, Denis Kenzior wrote:

Hi Tudor,

On 02/28/2018 10:52 AM, Tudor Ambarus wrote:

Provide three new operations in the key_type struct that can be used to
provide access to kpp operations. These will be implemented for the
asymmetric key type in a later patch and may refer to a key retained in
RAM by the kernel or a key retained in crypto hardware.

 int (*asym_kpp_query)(const struct kernel_kpp_params *params,
   struct kernel_kpp_query *res);
 int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params,
   void *out);
 int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params,
const void *in, void *out);

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
  Documentation/security/keys/core.rst | 54 


  include/linux/key-type.h |  7 +
  include/linux/keyctl.h   | 11 
  include/uapi/linux/keyctl.h  |  3 ++
  4 files changed, 75 insertions(+)

diff --git a/Documentation/security/keys/core.rst 
b/Documentation/security/keys/core.rst

index d224fd0..9b69a1f 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1688,6 +1688,60 @@ The structure has a number of fields, some of 
which are mandatory:
   If successful, 0 will be returned.  If the key doesn't support 
this,

   EOPNOTSUPP will be returned.
+  *  ``int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params, 
void *out);``
+  *  ``int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params, 
const void *in, void *out);``

+
+ These methods are optional. If provided the first allows to 
generate the
+ public key pair corresponding to the private key. The second 
method allows
+ to generate a shared secret by  combining the private key and 
the other

+ party's public key.
+
+ In all cases, the following information is provided in the 
params block::

+
+   struct kernel_kpp_query {
+   struct key  *key;
+   __u32   in_len; /* Input data size */
+   __u32   out_len;/* Output buffer size */
+   }
+


Probably not a huge deal as most common key sizes are already supported, 
but... is there a way to query supported key sizes?  I think for 
DH_COMPUTE we didn't have this problem as everything was done in 
software.  However, if the intent is to use TPM / other hardware engines 
we might need a way to query supported key sizes.


Oh, there's a typo here, this structure should have been named
'kernel_kpp_params' and not 'kernel_kpp_query'. 'kernel_kpp_query' is
defined below, it provides a way for determining the maximum supported
key size.



+ This includes the key to be used and the sizes in bytes of the 
input and

+ output buffers.
+
+ For a given operation, the in and out buffers are used as follows::
+
+   Operation IDin,in_lenout,out_len
+   === ===  

+   KEYCTL_KPP_GEN_PUBKEY   -Corresponding 
public key

+   KEYCTL_KPP_COMPUTE_SS   Pair's public keyShared Secret
+
+ If successful, the public key generation and the shared secret 
computation

+ will return the amount of data written into the output buffer.
+
+  *  ``int (*asym_kpp_query)(const struct kernel_kpp_params *params, 
struct kernel_kpp_query *res);``

+
+ This method is optional. If provided it allows information about 
the

+ asymmetric KPP (Key Protocol Primitives) key held in the key to be
+ determined.
+
+ The ``params`` block will contain the key to be queried. 
``in_len`` and

+ ``out_len`` are unused.
+
+ If successful, the following information is filled in::
+
+   struct kernel_kpp_query {
+   __u32   supported_ops;  /* Which ops are 
supported */
+   __u32   max_size;   /* Maximum size of the 
output buffer */

+   };
+
+ The supported_ops field will contain a bitmask indicating what 
operations
+ are supported by the key, including public key generation and 
shared

+ secret computation. The following constants are defined for this::
+
+   KEYCTL_SUPPORTS_{GEN_PUBKEY, COMPUTE_SS};
+
+ If successful, 0 is returned.  If the key is not an asymmetric 
kpp key,

+ EOPNOTSUPP is returned.
+
  Request-Key Callback Service
  
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index bc9af55..d354b6b 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -19,6 +19,8 @@
  struct kernel_pkey_query;
  struct kernel_pkey_params;
+struct kernel_kpp_query;
+struct kernel_kpp_params;
  /*
   * key under-construction record
@@ -165,6 +167,11 @@ struct key_type {
 const void *in

Re: [RFC PATCH 0/5] KEYS: add kpp keyctl operations

[Tudor Ambarus]

ping again.

On 04/11/2018 02:08 PM, Tudor Ambarus wrote:

Hi,

There was a long discussion about which interface to chose to export
akcipher and kpp to user-space. This series came as an alternative to
what Stephan proposed for af_alg[1]. I would like some feedback before
diving into tpm.

Best,
ta

[1] https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg27255.html

On 02/28/2018 06:52 PM, Tudor Ambarus wrote:

This series provides keyctl access for kpp operations, including
a query function, a function to generate the public key that is
associated with the private key and a function to compute the
shared secret.

I've added a KPP ecdh parser so that you can load an ECDH private
key into the kernel. The ECDH private keys are expected to be encoded
with the ecdh helpers from kernel. If the private key is valid, the
parser will allocate a tfm and set the private key. There is a
one-to-one binding between the private key and the tfm. The tfm will be
associated with the key for the entire life of the key. The tfm is
allocated once and used as many times as needed.

The kernel patches can be found here also:

https://github.com/ambarus/linux/commits/keys-kpp

The keyutils changes can be found here:

https://github.com/ambarus/keyutils/commits/keys-kpp

These patches are similar to what David Howells proposed for akcipher:

https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19915.html

I've ported David's patches on top of keys-next and then I've added my
patches on top of them.

Both proposals, David's and mine, lack support for accessing TPM.
Before getting familiar with TPM, please let me know how you feel
about this series.

Tudor Ambarus (5):
   KEYS: Provide key type operations for kpp ops
   KEYS: Provide keyctls to drive the new key type ops for kpp
   KEYS: Provide missing asym kpp subops for new key type ops
   KEYS: add asymmetric kpp subtype
   KEYS: add KPP ecdh parser

  Documentation/security/keys/core.rst | 113 +
  crypto/asymmetric_keys/Kconfig   |  15 +++
  crypto/asymmetric_keys/Makefile  |   6 +
  crypto/asymmetric_keys/asym_kpp.c| 142 +
  crypto/asymmetric_keys/asymmetric_type.c |  77 
  crypto/asymmetric_keys/kpp_parser.c  | 124 +++
  include/crypto/asym_kpp_subtype.h|  14 +++
  include/keys/asymmetric-subtype.h|  12 ++
  include/linux/key-type.h |   7 ++
  include/linux/keyctl.h   |  11 ++
  include/uapi/linux/keyctl.h  |  19 +++
  security/keys/Makefile   |   1 +
  security/keys/compat.c   |  10 ++
  security/keys/internal.h |  28 +
  security/keys/keyctl.c   |  13 ++
  security/keys/keyctl_kpp.c   | 205 
+++

  16 files changed, 797 insertions(+)
  create mode 100644 crypto/asymmetric_keys/asym_kpp.c
  create mode 100644 crypto/asymmetric_keys/kpp_parser.c
  create mode 100644 include/crypto/asym_kpp_subtype.h
  create mode 100644 security/keys/keyctl_kpp.c


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

Re: [PATCH v2 1/2] crypto: ccree: enable support for hardware keys

[Tudor Ambarus]

Hi, Gilad,

On 04/23/2018 10:25 AM, Gilad Ben-Yossef wrote:

Enable CryptoCell support for hardware keys.

Hardware keys are regular AES keys loaded into CryptoCell internal memory
via firmware, often from secure boot ROM or hardware fuses at boot time.

As such, they can be used for enc/dec purposes like any other key but
cannot (read: extremely hard to) be extracted since since they are not
available anywhere in RAM during runtime.

The mechanism has some similarities to s390 secure keys although the keys
are not wrapped or sealed, but simply loaded offline. The interface was
therefore modeled based on the s390 secure keys support.


I'm interested in hardware keys, ecc508 supports them too. In your
proposal you expect that the user will provide a specific key token that
is meaningful only for the ccree driver. If another driver that supports
"cbc(paes)" shows up, you will force the user to select a specific
driver implementation and to know what kind of key token to provide.
Shouldn't we have a common API that can address other drivers too?

Best,
ta

Re: [PATCH RESEND] crypto: caam - strip input zeros from RSA input buffer

[Tudor Ambarus]

On 04/16/2018 04:07 PM, Horia Geantă wrote:

Sometimes the provided RSA input buffer provided is not stripped
of leading zeros. This could cause its size to be bigger than that
of the modulus, making the HW complain:

caam_jr 2142000.jr1: 4789: DECO: desc idx 7:
Protocol Size Error - A protocol has seen an error in size. When
running RSA, pdb size N < (size of F) when no formatting is used; or
pdb size N < (F + 11) when formatting is used.

Fix the problem by stripping off the leading zero from input data
before feeding it to the CAAM accelerator.

Fixes: 8c419778ab57e ("crypto: caam - add support for RSA algorithm")
Cc: <sta...@vger.kernel.org> # 4.8+
Reported-by: Martin Townsend <mtownsend1...@gmail.com>
Link: 
https://lkml.kernel.org/r/cabatt_ytyoryktapcb4izhnanekkgfi9xaqmjhi_n-8ywoc...@mail.gmail.com
Signed-off-by: Horia Geantă <horia.gea...@nxp.com>
---
(Hopefully this one will reach the mailing list.
Sorry for the noise, problems with SMTP.)

  drivers/crypto/caam/caampkc.c | 54 +++
  drivers/crypto/caam/caampkc.h |  8 +++
  2 files changed, 62 insertions(+)

diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index 7a897209f181..979072b25eaa 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -166,18 +166,71 @@ static void rsa_priv_f3_done(struct device *dev, u32 
*desc, u32 err,
akcipher_request_complete(req, err);
  }
  
+static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,

+   unsigned int nbytes,
+   unsigned int flags)
+{
+   struct sg_mapping_iter miter;
+   int lzeros, ents;
+   unsigned int len;
+   unsigned int tbytes = nbytes;
+   const u8 *buff;
+
+   ents = sg_nents_for_len(sgl, nbytes);
+   if (ents < 0)
+   return ents;
+
+   sg_miter_start(, sgl, ents, SG_MITER_FROM_SG | flags);
+
+   lzeros = 0;
+   len = 0;
+   while (nbytes > 0) {
+   while (len && !*buff) {
+   lzeros++;
+   len--;
+   buff++;
+   }
+
+   if (len && *buff)
+   break;
+
+   sg_miter_next();
+   buff = miter.addr;
+   len = miter.length;
+
+   nbytes -= lzeros;
+   lzeros = 0;
+   }
+
+   miter.consumed = lzeros;
+   sg_miter_stop();
+   nbytes -= lzeros;
+
+   return tbytes - nbytes;
+}
+
  static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 size_t desclen)
  {
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
struct caam_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
struct device *dev = ctx->dev;
+   struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
struct rsa_edesc *edesc;
gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
   GFP_KERNEL : GFP_ATOMIC;
+   int sg_flags = (flags == GFP_ATOMIC) ? SG_MITER_ATOMIC : 0;
int sgc;
int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
int src_nents, dst_nents;
+   int lzeros;
+
+   lzeros = caam_rsa_count_leading_zeros(req->src, req->src_len, sg_flags);
+   if (lzeros < 0)
+   return ERR_PTR(lzeros);
+
+   req->src_len -= lzeros;
+   req->src = scatterwalk_ffwd(req_ctx->src, req->src, lzeros);


This is interesting, you are overwriting the request's src sg. I kept
wondering if one could have a problem if we are modifying its src sg. I
couldn't find any failing scenario, so:

Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>

Re: [RFC PATCH 0/5] KEYS: add kpp keyctl operations

[Tudor Ambarus]

Hi,

There was a long discussion about which interface to chose to export
akcipher and kpp to user-space. This series came as an alternative to
what Stephan proposed for af_alg[1]. I would like some feedback before
diving into tpm.

Best,
ta

[1] https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg27255.html

On 02/28/2018 06:52 PM, Tudor Ambarus wrote:

This series provides keyctl access for kpp operations, including
a query function, a function to generate the public key that is
associated with the private key and a function to compute the
shared secret.

I've added a KPP ecdh parser so that you can load an ECDH private
key into the kernel. The ECDH private keys are expected to be encoded
with the ecdh helpers from kernel. If the private key is valid, the
parser will allocate a tfm and set the private key. There is a
one-to-one binding between the private key and the tfm. The tfm will be
associated with the key for the entire life of the key. The tfm is
allocated once and used as many times as needed.

The kernel patches can be found here also:

https://github.com/ambarus/linux/commits/keys-kpp

The keyutils changes can be found here:

https://github.com/ambarus/keyutils/commits/keys-kpp

These patches are similar to what David Howells proposed for akcipher:

https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19915.html

I've ported David's patches on top of keys-next and then I've added my
patches on top of them.

Both proposals, David's and mine, lack support for accessing TPM.
Before getting familiar with TPM, please let me know how you feel
about this series.

Tudor Ambarus (5):
   KEYS: Provide key type operations for kpp ops
   KEYS: Provide keyctls to drive the new key type ops for kpp
   KEYS: Provide missing asym kpp subops for new key type ops
   KEYS: add asymmetric kpp subtype
   KEYS: add KPP ecdh parser

  Documentation/security/keys/core.rst | 113 +
  crypto/asymmetric_keys/Kconfig   |  15 +++
  crypto/asymmetric_keys/Makefile  |   6 +
  crypto/asymmetric_keys/asym_kpp.c| 142 +
  crypto/asymmetric_keys/asymmetric_type.c |  77 
  crypto/asymmetric_keys/kpp_parser.c  | 124 +++
  include/crypto/asym_kpp_subtype.h|  14 +++
  include/keys/asymmetric-subtype.h|  12 ++
  include/linux/key-type.h |   7 ++
  include/linux/keyctl.h   |  11 ++
  include/uapi/linux/keyctl.h  |  19 +++
  security/keys/Makefile   |   1 +
  security/keys/compat.c   |  10 ++
  security/keys/internal.h |  28 +
  security/keys/keyctl.c   |  13 ++
  security/keys/keyctl_kpp.c   | 205 +++
  16 files changed, 797 insertions(+)
  create mode 100644 crypto/asymmetric_keys/asym_kpp.c
  create mode 100644 crypto/asymmetric_keys/kpp_parser.c
  create mode 100644 include/crypto/asym_kpp_subtype.h
  create mode 100644 security/keys/keyctl_kpp.c

[PATCH v3 2/2] crypto: authencesn - don't leak pointers to authenc keys

[Tudor Ambarus]

In crypto_authenc_esn_setkey we save pointers to the authenc keys
in a local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/authencesn.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/authencesn.c b/crypto/authencesn.c
index 15f91dd..50b8047 100644
--- a/crypto/authencesn.c
+++ b/crypto/authencesn.c
@@ -90,6 +90,7 @@ static int crypto_authenc_esn_setkey(struct crypto_aead 
*authenc_esn, const u8 *
   CRYPTO_TFM_RES_MASK);
 
 out:
+   memzero_explicit(, sizeof(keys));
return err;
 
 badkey:
-- 
2.9.4

[PATCH v3 1/2] crypto: authenc - don't leak pointers to authenc keys

[Tudor Ambarus]

In crypto_authenc_setkey we save pointers to the authenc keys in
a local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/authenc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/authenc.c b/crypto/authenc.c
index d3d6d72..4fa8d40 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -108,6 +108,7 @@ static int crypto_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
   CRYPTO_TFM_RES_MASK);
 
 out:
+   memzero_explicit(, sizeof(keys));
return err;
 
 badkey:
-- 
2.9.4

Re: [PATCH] crypto: rsa - remove unneeded initializations

[Tudor Ambarus]

On 03/30/2018 08:27 PM, Herbert Xu wrote:

On Mon, Mar 26, 2018 at 02:59:06PM +0300, Tudor Ambarus wrote:

Remove useless assignment of ret to -ENOMEM in rsa_verify.
Remove useless initialization of ret to zero at declaration in
rsa_enc/dec/sign/verify.

Benefit of the power of undefined values and set ret in branches in
rsa_enc/dec/sign.

Reported-by: Benjamin Bales <techsupp...@mycode.ai>
Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>


The existing code looks fine.  If anything we should move the
assignments out of the if clause.


I set the err inside the if branch so that the compiler will
warn me in case of undefined value for err. Like here:

https://rusty.ozlabs.org/?p=232

Best,
ta

Re: in-kernel user of ecdsa

[Tudor Ambarus]

On 03/12/2018 07:07 PM, Tudor Ambarus wrote:


Would you consider using ECDSA in the kernel module signing facility?


Any feedback is good. I can invest some time to make this happen, if
needed.


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
--
To unsubscribe from this list: send the line "unsubscribe keyrings" 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 - remove unneeded initializations

[Tudor Ambarus]

Remove useless assignment of ret to -ENOMEM in rsa_verify.
Remove useless initialization of ret to zero at declaration in
rsa_enc/dec/sign/verify.

Benefit of the power of undefined values and set ret in branches in
rsa_enc/dec/sign.

Reported-by: Benjamin Bales <techsupp...@mycode.ai>
Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/rsa.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/crypto/rsa.c b/crypto/rsa.c
index b067f3a..e75ce09 100644
--- a/crypto/rsa.c
+++ b/crypto/rsa.c
@@ -88,7 +88,7 @@ static int rsa_enc(struct akcipher_request *req)
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
const struct rsa_mpi_key *pkey = rsa_get_key(tfm);
MPI m, c = mpi_alloc(0);
-   int ret = 0;
+   int ret;
int sign;
 
if (!c)
@@ -99,10 +99,11 @@ static int rsa_enc(struct akcipher_request *req)
goto err_free_c;
}
 
-   ret = -ENOMEM;
m = mpi_read_raw_from_sgl(req->src, req->src_len);
-   if (!m)
+   if (!m) {
+   ret = -ENOMEM;
goto err_free_c;
+   }
 
ret = _rsa_enc(pkey, c, m);
if (ret)
@@ -127,7 +128,7 @@ static int rsa_dec(struct akcipher_request *req)
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
const struct rsa_mpi_key *pkey = rsa_get_key(tfm);
MPI c, m = mpi_alloc(0);
-   int ret = 0;
+   int ret;
int sign;
 
if (!m)
@@ -138,10 +139,11 @@ static int rsa_dec(struct akcipher_request *req)
goto err_free_m;
}
 
-   ret = -ENOMEM;
c = mpi_read_raw_from_sgl(req->src, req->src_len);
-   if (!c)
+   if (!c) {
+   ret = -ENOMEM;
goto err_free_m;
+   }
 
ret = _rsa_dec(pkey, m, c);
if (ret)
@@ -165,7 +167,7 @@ static int rsa_sign(struct akcipher_request *req)
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
const struct rsa_mpi_key *pkey = rsa_get_key(tfm);
MPI m, s = mpi_alloc(0);
-   int ret = 0;
+   int ret;
int sign;
 
if (!s)
@@ -176,10 +178,11 @@ static int rsa_sign(struct akcipher_request *req)
goto err_free_s;
}
 
-   ret = -ENOMEM;
m = mpi_read_raw_from_sgl(req->src, req->src_len);
-   if (!m)
+   if (!m) {
+   ret = -ENOMEM;
goto err_free_s;
+   }
 
ret = _rsa_sign(pkey, s, m);
if (ret)
@@ -204,7 +207,7 @@ static int rsa_verify(struct akcipher_request *req)
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
const struct rsa_mpi_key *pkey = rsa_get_key(tfm);
MPI s, m = mpi_alloc(0);
-   int ret = 0;
+   int ret;
int sign;
 
if (!m)
@@ -215,7 +218,6 @@ static int rsa_verify(struct akcipher_request *req)
goto err_free_m;
}
 
-   ret = -ENOMEM;
s = mpi_read_raw_from_sgl(req->src, req->src_len);
if (!s) {
ret = -ENOMEM;
-- 
2.9.4

[PATCH v2 8/9] crypto: qat - don't leak pointers to authenc keys

[Tudor Ambarus]

In qat_alg_aead_init_sessions we save pointers to the authenc keys
in a local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/qat/qat_common/qat_algs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c 
b/drivers/crypto/qat/qat_common/qat_algs.c
index baffae8..1138e41 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -546,11 +546,14 @@ static int qat_alg_aead_init_sessions(struct crypto_aead 
*tfm, const u8 *key,
if (qat_alg_aead_init_dec_session(tfm, alg, , mode))
goto error;
 
+   memzero_explicit(, sizeof(keys));
return 0;
 bad_key:
crypto_aead_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 error:
+   memzero_explicit(, sizeof(keys));
return -EFAULT;
 }
 
-- 
2.9.4

[PATCH v2 9/9] crypto: talitos - don't leak pointers to authenc keys

[Tudor Ambarus]

In talitos's aead_setkey we save pointers to the authenc keys in a
local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
Reviewed-by: Christophe Leroy <christophe.le...@c-s.fr>
---
 drivers/crypto/talitos.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 447cb8b..c92efc7 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -904,10 +904,12 @@ static int aead_setkey(struct crypto_aead *authenc,
ctx->dma_key = dma_map_single(dev, ctx->key, ctx->keylen,
  DMA_TO_DEVICE);
 
+   memzero_explicit(, sizeof(keys));
return 0;
 
 badkey:
crypto_aead_set_flags(authenc, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH v2 3/9] crypto: caam - don't leak pointers to authenc keys

[Tudor Ambarus]

In caam's aead_setkey we save pointers to the authenc keys in a
local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/caam/caamalg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 584a6c1..7207a53 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -570,9 +570,11 @@ static int aead_setkey(struct crypto_aead *aead,
 
 skip_split_key:
ctx->cdata.keylen = keys.enckeylen;
+   memzero_explicit(, sizeof(keys));
return aead_set_sh_desc(aead);
 badkey:
crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH v2 4/9] crypto: caam/qi - don't leak pointers to authenc keys

[Tudor Ambarus]

In caam/qi's aead_setkey we save pointers to the authenc keys in
a local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/caam/caamalg_qi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
index c2b5762..cacda08 100644
--- a/drivers/crypto/caam/caamalg_qi.c
+++ b/drivers/crypto/caam/caamalg_qi.c
@@ -278,9 +278,11 @@ static int aead_setkey(struct crypto_aead *aead, const u8 
*key,
}
}
 
+   memzero_explicit(, sizeof(keys));
return ret;
 badkey:
crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH v2 7/9] crypto: picoxcell - don't leak pointers to authenc keys

[Tudor Ambarus]

In spacc_aead_setkey we save pointers to the authenc keys in a
local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
Reviewed-by: Jamie Iles <ja...@jamieiles.com>
---
 drivers/crypto/picoxcell_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/picoxcell_crypto.c 
b/drivers/crypto/picoxcell_crypto.c
index 4ef52c9..a4df966 100644
--- a/drivers/crypto/picoxcell_crypto.c
+++ b/drivers/crypto/picoxcell_crypto.c
@@ -499,10 +499,12 @@ static int spacc_aead_setkey(struct crypto_aead *tfm, 
const u8 *key,
memcpy(ctx->hash_ctx, keys.authkey, keys.authkeylen);
ctx->hash_key_len = keys.authkeylen;
 
+   memzero_explicit(, sizeof(keys));
return 0;
 
 badkey:
crypto_aead_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH v2 5/9] crypto: chcr - don't leak pointers to authenc keys

[Tudor Ambarus]

In chcr_authenc_setkey and chcr_aead_digest_null_setkey we save
pointers to the authenc keys in local variables of type
struct crypto_authenc_keys and we don't zeroize them after use.
Fix this and don't leak pointers to the authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/chelsio/chcr_algo.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/crypto/chelsio/chcr_algo.c 
b/drivers/crypto/chelsio/chcr_algo.c
index 4617c7a..91bc77a 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -3457,6 +3457,7 @@ static int chcr_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
if (IS_ERR(base_hash)) {
pr_err("chcr : Base driver cannot be loaded\n");
aeadctx->enckey_len = 0;
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
}
{
@@ -3508,10 +3509,12 @@ static int chcr_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
actx->auth_mode = param.auth_mode;
chcr_free_shash(base_hash);
 
+   memzero_explicit(, sizeof(keys));
return 0;
}
 out:
aeadctx->enckey_len = 0;
+   memzero_explicit(, sizeof(keys));
if (!IS_ERR(base_hash))
chcr_free_shash(base_hash);
return -EINVAL;
@@ -3574,9 +3577,11 @@ static int chcr_aead_digest_null_setkey(struct 
crypto_aead *authenc,
aeadctx->key_ctx_hdr = FILL_KEY_CTX_HDR(ck_size, CHCR_KEYCTX_NO_KEY, 0,
0, key_ctx_len >> 4);
actx->auth_mode = CHCR_SCMD_AUTH_MODE_NOP;
+   memzero_explicit(, sizeof(keys));
return 0;
 out:
aeadctx->enckey_len = 0;
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH v2 6/9] crypto: ixp4xx - don't leak pointers to authenc keys

[Tudor Ambarus]

In ixp4xx's aead_setkey we save pointers to the authenc keys in a
local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/ixp4xx_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 717a266..27f7dad 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -1167,9 +1167,11 @@ static int aead_setkey(struct crypto_aead *tfm, const u8 
*key,
ctx->authkey_len = keys.authkeylen;
ctx->enckey_len = keys.enckeylen;
 
+   memzero_explicit(, sizeof(keys));
return aead_setup(tfm, crypto_aead_authsize(tfm));
 badkey:
crypto_aead_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH v2 1/9] crypto: authenc - don't leak pointers to authenc keys

[Tudor Ambarus]

In crypto_authenc_setkey we save pointers to the authenc keys in
a local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/authenc.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/crypto/authenc.c b/crypto/authenc.c
index d3d6d72..480a08b 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -87,8 +87,10 @@ static int crypto_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
struct crypto_authenc_keys keys;
int err = -EINVAL;
 
-   if (crypto_authenc_extractkeys(, key, keylen) != 0)
-   goto badkey;
+   if (crypto_authenc_extractkeys(, key, keylen) != 0) {
+   crypto_aead_set_flags(authenc, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   goto out;
+   }
 
crypto_ahash_clear_flags(auth, CRYPTO_TFM_REQ_MASK);
crypto_ahash_set_flags(auth, crypto_aead_get_flags(authenc) &
@@ -108,11 +110,8 @@ static int crypto_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
   CRYPTO_TFM_RES_MASK);
 
 out:
+   memzero_explicit(, sizeof(keys));
return err;
-
-badkey:
-   crypto_aead_set_flags(authenc, CRYPTO_TFM_RES_BAD_KEY_LEN);
-   goto out;
 }
 
 static void authenc_geniv_ahash_done(struct crypto_async_request *areq, int 
err)
-- 
2.9.4

[PATCH v2 2/9] crypto: authencesn - don't leak pointers to authenc keys

[Tudor Ambarus]

In crypto_authenc_esn_setkey we save pointers to the authenc keys
in a local variable of type struct crypto_authenc_keys and we don't
zeroize it after use. Fix this and don't leak pointers to the
authenc keys.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/authencesn.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/crypto/authencesn.c b/crypto/authencesn.c
index 15f91dd..3bed6d1 100644
--- a/crypto/authencesn.c
+++ b/crypto/authencesn.c
@@ -69,8 +69,10 @@ static int crypto_authenc_esn_setkey(struct crypto_aead 
*authenc_esn, const u8 *
struct crypto_authenc_keys keys;
int err = -EINVAL;
 
-   if (crypto_authenc_extractkeys(, key, keylen) != 0)
-   goto badkey;
+   if (crypto_authenc_extractkeys(, key, keylen) != 0) {
+   crypto_aead_set_flags(authenc_esn, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   goto out;
+   }
 
crypto_ahash_clear_flags(auth, CRYPTO_TFM_REQ_MASK);
crypto_ahash_set_flags(auth, crypto_aead_get_flags(authenc_esn) &
@@ -90,11 +92,8 @@ static int crypto_authenc_esn_setkey(struct crypto_aead 
*authenc_esn, const u8 *
   CRYPTO_TFM_RES_MASK);
 
 out:
+   memzero_explicit(, sizeof(keys));
return err;
-
-badkey:
-   crypto_aead_set_flags(authenc_esn, CRYPTO_TFM_RES_BAD_KEY_LEN);
-   goto out;
 }
 
 static int crypto_authenc_esn_genicv_tail(struct aead_request *req,
-- 
2.9.4

[PATCH v2 0/9] crypto: don't leak pointers to authenc keys

[Tudor Ambarus]

There are few places in crypto where we save pointers to the
authenc keys to a local variable of type struct crypto_authenc_keys
and we don't zeroize it after use. Fix all those cases and don't
leak pointers to the authenc keys.

--

Changes in v2:
- add commit message on each patch
- add Jamie's and Christophe's Reviewed-by tag

Tudor Ambarus (9):
  crypto: authenc - don't leak pointers to authenc keys
  crypto: authencesn - don't leak pointers to authenc keys
  crypto: caam - don't leak pointers to authenc keys
  crypto: caam/qi - don't leak pointers to authenc keys
  crypto: chcr - don't leak pointers to authenc keys
  crypto: ixp4xx - don't leak pointers to authenc keys
  crypto: picoxcell - don't leak pointers to authenc keys
  crypto: qat - don't leak pointers to authenc keys
  crypto: talitos - don't leak pointers to authenc keys

 crypto/authenc.c | 11 +--
 crypto/authencesn.c  | 11 +--
 drivers/crypto/caam/caamalg.c|  2 ++
 drivers/crypto/caam/caamalg_qi.c |  2 ++
 drivers/crypto/chelsio/chcr_algo.c   |  5 +
 drivers/crypto/ixp4xx_crypto.c   |  2 ++
 drivers/crypto/picoxcell_crypto.c|  2 ++
 drivers/crypto/qat/qat_common/qat_algs.c |  3 +++
 drivers/crypto/talitos.c |  2 ++
 9 files changed, 28 insertions(+), 12 deletions(-)

-- 
2.9.4

[PATCH 6/9] crypto: ixp4xx - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/ixp4xx_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 717a266..27f7dad 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -1167,9 +1167,11 @@ static int aead_setkey(struct crypto_aead *tfm, const u8 
*key,
ctx->authkey_len = keys.authkeylen;
ctx->enckey_len = keys.enckeylen;
 
+   memzero_explicit(, sizeof(keys));
return aead_setup(tfm, crypto_aead_authsize(tfm));
 badkey:
crypto_aead_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH 7/9] crypto: picoxcell - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/picoxcell_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/picoxcell_crypto.c 
b/drivers/crypto/picoxcell_crypto.c
index 4ef52c9..a4df966 100644
--- a/drivers/crypto/picoxcell_crypto.c
+++ b/drivers/crypto/picoxcell_crypto.c
@@ -499,10 +499,12 @@ static int spacc_aead_setkey(struct crypto_aead *tfm, 
const u8 *key,
memcpy(ctx->hash_ctx, keys.authkey, keys.authkeylen);
ctx->hash_key_len = keys.authkeylen;
 
+   memzero_explicit(, sizeof(keys));
return 0;
 
 badkey:
crypto_aead_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH 2/9] crypto: authencesn - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/authencesn.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/crypto/authencesn.c b/crypto/authencesn.c
index 15f91dd..3bed6d1 100644
--- a/crypto/authencesn.c
+++ b/crypto/authencesn.c
@@ -69,8 +69,10 @@ static int crypto_authenc_esn_setkey(struct crypto_aead 
*authenc_esn, const u8 *
struct crypto_authenc_keys keys;
int err = -EINVAL;
 
-   if (crypto_authenc_extractkeys(, key, keylen) != 0)
-   goto badkey;
+   if (crypto_authenc_extractkeys(, key, keylen) != 0) {
+   crypto_aead_set_flags(authenc_esn, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   goto out;
+   }
 
crypto_ahash_clear_flags(auth, CRYPTO_TFM_REQ_MASK);
crypto_ahash_set_flags(auth, crypto_aead_get_flags(authenc_esn) &
@@ -90,11 +92,8 @@ static int crypto_authenc_esn_setkey(struct crypto_aead 
*authenc_esn, const u8 *
   CRYPTO_TFM_RES_MASK);
 
 out:
+   memzero_explicit(, sizeof(keys));
return err;
-
-badkey:
-   crypto_aead_set_flags(authenc_esn, CRYPTO_TFM_RES_BAD_KEY_LEN);
-   goto out;
 }
 
 static int crypto_authenc_esn_genicv_tail(struct aead_request *req,
-- 
2.9.4

[PATCH 9/9] crypto: talitos - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/talitos.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 447cb8b..c92efc7 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -904,10 +904,12 @@ static int aead_setkey(struct crypto_aead *authenc,
ctx->dma_key = dma_map_single(dev, ctx->key, ctx->keylen,
  DMA_TO_DEVICE);
 
+   memzero_explicit(, sizeof(keys));
return 0;
 
 badkey:
crypto_aead_set_flags(authenc, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH 8/9] crypto: qat - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/qat/qat_common/qat_algs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c 
b/drivers/crypto/qat/qat_common/qat_algs.c
index baffae8..1138e41 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -546,11 +546,14 @@ static int qat_alg_aead_init_sessions(struct crypto_aead 
*tfm, const u8 *key,
if (qat_alg_aead_init_dec_session(tfm, alg, , mode))
goto error;
 
+   memzero_explicit(, sizeof(keys));
return 0;
 bad_key:
crypto_aead_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 error:
+   memzero_explicit(, sizeof(keys));
return -EFAULT;
 }
 
-- 
2.9.4

[PATCH 0/9] don't leak pointers to authenc keys

[Tudor Ambarus]

There are few places in crypto where we save pointers to the
authenc keys to a local variable of type struct crypto_authenc_keys
and we don't zeroize it after use. Fix all those cases and don't
leak pointers to the authenc keys.

Tudor Ambarus (9):
  crypto: authenc - don't leak pointers to authenc keys
  crypto: authencesn - don't leak pointers to authenc keys
  crypto: caam - don't leak pointers to authenc keys
  crypto: caam/qi - don't leak pointers to authenc keys
  crypto: chcr - don't leak pointers to authenc keys
  crypto: ixp4xx - don't leak pointers to authenc keys
  crypto: picoxcell - don't leak pointers to authenc keys
  crypto: qat - don't leak pointers to authenc keys
  crypto: talitos - don't leak pointers to authenc keys

 crypto/authenc.c | 11 +--
 crypto/authencesn.c  | 11 +--
 drivers/crypto/caam/caamalg.c|  2 ++
 drivers/crypto/caam/caamalg_qi.c |  2 ++
 drivers/crypto/chelsio/chcr_algo.c   |  5 +
 drivers/crypto/ixp4xx_crypto.c   |  2 ++
 drivers/crypto/picoxcell_crypto.c|  2 ++
 drivers/crypto/qat/qat_common/qat_algs.c |  3 +++
 drivers/crypto/talitos.c |  2 ++
 9 files changed, 28 insertions(+), 12 deletions(-)

-- 
2.9.4

[PATCH 4/9] crypto: caam/qi - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/caam/caamalg_qi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
index c2b5762..cacda08 100644
--- a/drivers/crypto/caam/caamalg_qi.c
+++ b/drivers/crypto/caam/caamalg_qi.c
@@ -278,9 +278,11 @@ static int aead_setkey(struct crypto_aead *aead, const u8 
*key,
}
}
 
+   memzero_explicit(, sizeof(keys));
return ret;
 badkey:
crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH 5/9] crypto: chcr - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/chelsio/chcr_algo.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/crypto/chelsio/chcr_algo.c 
b/drivers/crypto/chelsio/chcr_algo.c
index 4617c7a..91bc77a 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -3457,6 +3457,7 @@ static int chcr_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
if (IS_ERR(base_hash)) {
pr_err("chcr : Base driver cannot be loaded\n");
aeadctx->enckey_len = 0;
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
}
{
@@ -3508,10 +3509,12 @@ static int chcr_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
actx->auth_mode = param.auth_mode;
chcr_free_shash(base_hash);
 
+   memzero_explicit(, sizeof(keys));
return 0;
}
 out:
aeadctx->enckey_len = 0;
+   memzero_explicit(, sizeof(keys));
if (!IS_ERR(base_hash))
chcr_free_shash(base_hash);
return -EINVAL;
@@ -3574,9 +3577,11 @@ static int chcr_aead_digest_null_setkey(struct 
crypto_aead *authenc,
aeadctx->key_ctx_hdr = FILL_KEY_CTX_HDR(ck_size, CHCR_KEYCTX_NO_KEY, 0,
0, key_ctx_len >> 4);
actx->auth_mode = CHCR_SCMD_AUTH_MODE_NOP;
+   memzero_explicit(, sizeof(keys));
return 0;
 out:
aeadctx->enckey_len = 0;
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH 3/9] crypto: caam - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/caam/caamalg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 584a6c1..7207a53 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -570,9 +570,11 @@ static int aead_setkey(struct crypto_aead *aead,
 
 skip_split_key:
ctx->cdata.keylen = keys.enckeylen;
+   memzero_explicit(, sizeof(keys));
return aead_set_sh_desc(aead);
 badkey:
crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   memzero_explicit(, sizeof(keys));
return -EINVAL;
 }
 
-- 
2.9.4

[PATCH 1/9] crypto: authenc - don't leak pointers to authenc keys

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/authenc.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/crypto/authenc.c b/crypto/authenc.c
index d3d6d72..480a08b 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -87,8 +87,10 @@ static int crypto_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
struct crypto_authenc_keys keys;
int err = -EINVAL;
 
-   if (crypto_authenc_extractkeys(, key, keylen) != 0)
-   goto badkey;
+   if (crypto_authenc_extractkeys(, key, keylen) != 0) {
+   crypto_aead_set_flags(authenc, CRYPTO_TFM_RES_BAD_KEY_LEN);
+   goto out;
+   }
 
crypto_ahash_clear_flags(auth, CRYPTO_TFM_REQ_MASK);
crypto_ahash_set_flags(auth, crypto_aead_get_flags(authenc) &
@@ -108,11 +110,8 @@ static int crypto_authenc_setkey(struct crypto_aead 
*authenc, const u8 *key,
   CRYPTO_TFM_RES_MASK);
 
 out:
+   memzero_explicit(, sizeof(keys));
return err;
-
-badkey:
-   crypto_aead_set_flags(authenc, CRYPTO_TFM_RES_BAD_KEY_LEN);
-   goto out;
 }
 
 static void authenc_geniv_ahash_done(struct crypto_async_request *areq, int 
err)
-- 
2.9.4

Re: [PATCH] crypto: atmel-aes - fix the keys zeroing on errors

[Tudor Ambarus]

Hi, Antoine,

On 02/23/2018 02:37 PM, Antoine Tenart wrote:

Hi Tudor,

On Fri, Feb 23, 2018 at 02:04:33PM +0200, Tudor Ambarus wrote:


There are few other places in crypto where we extract the authenc keys
in the same type of local variable, struct crypto_authenc_keys keys, and
we don't zeroize it after use. I think we should fix those cases too.


You're right, I spotted other places where keys weren't zeroed. I
haven't got the time to do anything about it so far :)



Will you send a patch to fix those cases? We will forget about it.
I can do it myself, but it's better to ask first.

best,
ta

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 v2] crypto/ecc: Remove stack VLA usage

[Tudor Ambarus]

On 03/08/2018 11:57 PM, Kees Cook wrote:

On the quest to remove all VLAs from the kernel[1], this switches to
a pair of kmalloc regions instead of using the stack. This also moves
the get_random_bytes() after all allocations (and drops the needless
"nbytes" variable).

[1]https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook<keesc...@chromium.org>


Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>

Re: [PATCH] crypto/ecc: Remove stack VLA usage

[Tudor Ambarus]

On 03/08/2018 11:55 PM, Kees Cook wrote:

Looks like there are few intermediate buffers in ecc
that should be zeroized as well.

Can you send a patch for those?


Yeah, I'll take a look.

Best,
ta

Re: [PATCH] crypto/ecc: Remove stack VLA usage

[Tudor Ambarus]

Hi, Kees,

On 03/07/2018 11:56 PM, Kees Cook wrote:

On the quest to remove all VLAs from the kernel[1], this switches to
a pair of kmalloc regions instead of using the stack. This also moves
the get_random_bytes() after all allocations (and drops the needless
"nbytes" variable).

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook 
---
  crypto/ecc.c | 23 +--
  1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 18f32f2a5e1c..5bfa63603da0 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
  {
int ret = 0;
struct ecc_point *product, *pk;
-   u64 priv[ndigits];
-   u64 rand_z[ndigits];
-   unsigned int nbytes;
+   u64 *priv, *rand_z;
const struct ecc_curve *curve = ecc_get_curve(curve_id);
  
  	if (!private_key || !public_key || !curve) {

@@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
goto out;
}
  
-	nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;

+   priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   ret = -ENOMEM;
+   goto out;
+   }
  
-	get_random_bytes(rand_z, nbytes);

+   rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
+   if (!rand_z) {
+   ret = -ENOMEM;
+   goto kfree_out;
+   }
  
  	pk = ecc_alloc_point(ndigits);

if (!pk) {
ret = -ENOMEM;
-   goto out;
+   goto kfree_out;
}
  
  	product = ecc_alloc_point(ndigits);

@@ -1051,6 +1057,8 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
goto err_alloc_product;
}
  
+	get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);

+
ecc_swap_digits(public_key, pk->x, ndigits);
ecc_swap_digits(_key[ndigits], pk->y, ndigits);
ecc_swap_digits(private_key, priv, ndigits);
@@ -1065,6 +1073,9 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
ecc_free_point(product);
  err_alloc_product:
ecc_free_point(pk);
+kfree_out:
+   kfree(priv);


I think we should use kzfree here.


+   kfree(rand_z);


Probably here too. Looks like there are few intermediate buffers in ecc
that should be zeroized as well.

Best,
ta

  out:
return ret;
  }

[RFC PATCH 5/5] KEYS: add KPP ecdh parser

[Tudor Ambarus]

The ECDH private keys are expected to be encoded with the ecdh
helpers from kernel.

Use the ecdh helpers to check if the key is valid. If valid,
allocate a tfm and set the private key. There is a one-to-one
binding between the private key and the tfm. The tfm is allocated
once and used as many times as needed.

ECDH keys can be loaded like this:
# echo -n 
020028000200200024d121ebe5cf2d83f6621b6e43843aa38be086c32019da92505303e1c0eab882
 \
| xxd -r -p | keyctl padd asymmetric private @s

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/asymmetric_keys/Kconfig  |   8 +++
 crypto/asymmetric_keys/Makefile |   5 ++
 crypto/asymmetric_keys/kpp_parser.c | 124 
 include/crypto/asym_kpp_subtype.h   |   2 +
 4 files changed, 139 insertions(+)
 create mode 100644 crypto/asymmetric_keys/kpp_parser.c

diff --git a/crypto/asymmetric_keys/Kconfig b/crypto/asymmetric_keys/Kconfig
index 1884570..e46a185 100644
--- a/crypto/asymmetric_keys/Kconfig
+++ b/crypto/asymmetric_keys/Kconfig
@@ -57,6 +57,14 @@ config PKCS7_MESSAGE_PARSER
  This option provides support for parsing PKCS#7 format messages for
  signature data and provides the ability to verify the signature.
 
+config KPP_KEY_PARSER
+   tristate "KPP private key parser"
+   depends on ASYMMETRIC_KPP_SUBTYPE
+   help
+ This option provides support for parsing KPP format blobs for
+ private key data and provides the ability to instantiate a crypto key
+ from that data.
+
 config PKCS7_TEST_KEY
tristate "PKCS#7 testing key type"
depends on SYSTEM_DATA_VERIFICATION
diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile
index d884cf1..e709aee 100644
--- a/crypto/asymmetric_keys/Makefile
+++ b/crypto/asymmetric_keys/Makefile
@@ -62,6 +62,11 @@ $(obj)/pkcs7-asn1.o: $(obj)/pkcs7-asn1.c $(obj)/pkcs7-asn1.h
 clean-files+= pkcs7-asn1.c pkcs7-asn1.h
 
 #
+# KPP private key handling
+#
+obj-$(CONFIG_KPP_KEY_PARSER) += kpp_parser.o
+
+#
 # PKCS#7 parser testing key
 #
 obj-$(CONFIG_PKCS7_TEST_KEY) += pkcs7_test_key.o
diff --git a/crypto/asymmetric_keys/kpp_parser.c 
b/crypto/asymmetric_keys/kpp_parser.c
new file mode 100644
index 000..a38da7b
--- /dev/null
+++ b/crypto/asymmetric_keys/kpp_parser.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) "KPP-PARSER: "fmt
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int decode_kpp_ecdh_key(struct asym_kpp_ctx *ctx,
+  const void *data, size_t datalen)
+{
+   struct ecdh params;
+   int ret;
+
+   /* check if the key is valid */
+   ret = crypto_ecdh_decode_key(data, datalen, );
+   if (ret)
+   return ret;
+
+   ctx->alg_name = "ecdh";
+   return ret;
+}
+
+static int decode_kpp_key(struct asym_kpp_ctx *ctx,
+ const void *data, size_t datalen)
+{
+   if (decode_kpp_ecdh_key(ctx, data, datalen))
+   return -EBADMSG;
+   return 0;
+}
+
+static int kpp_parser_setkey(struct asym_kpp_ctx *ctx,
+const void *data, size_t datalen)
+{
+   struct crypto_kpp *tfm;
+   int ret;
+
+   tfm = crypto_alloc_kpp(ctx->alg_name, 0, 0);
+   if (IS_ERR(tfm))
+   return PTR_ERR(tfm);
+
+   ret = crypto_kpp_set_secret(tfm, data, datalen);
+   if (ret) {
+   crypto_free_kpp(tfm);
+   return ret;
+   }
+
+   ctx->tfm = tfm;
+   return ret;
+}
+
+static struct asym_kpp_ctx *kpp_parse(const void *data, size_t datalen)
+{
+   struct asym_kpp_ctx *ctx;
+   long ret;
+
+   ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+   if (!ctx)
+   return ERR_PTR(-ENOMEM);
+
+   ret = decode_kpp_key(ctx, data, datalen);
+   if (ret)
+   goto free_ctx;
+
+   ret = kpp_parser_setkey(ctx, data, datalen);
+   if (ret)
+   goto free_ctx;
+
+   return ctx;
+
+free_ctx:
+   kfree(ctx);
+   return ERR_PTR(ret);
+}
+
+/*
+ * Attempt to parse a data blob for a key as a KPP private key.
+ */
+static int kpp_key_preparse(struct key_preparsed_payload *prep)
+{
+   struct asym_kpp_ctx *ctx;
+
+   ctx = kpp_parse(prep->data, prep->datalen);
+   if (IS_ERR(ctx))
+   return PTR_ERR(ctx);
+
+   pr_devel("Algorithm name: %s\n", ctx->alg_name);
+
+   /* We're pinning the module by being linked against it */
+   __module_get(asym_kpp_subtype.owner);
+   prep->payload.data[asym_subtype] = _kpp_subtype;
+   prep->payload.data[asym_key_ids] = NULL;
+   prep->payload.data[asym_crypto] = ctx;
+   prep->payload.data[asym_auth] = NULL;
+   prep->quotalen = 100;
+   return 0;
+}
+
+static struct asymmetric_k

[RFC PATCH 3/5] KEYS: Provide missing asym kpp subops for new key type ops

[Tudor Ambarus]

Includes kpp_query, kpp_gen_pubkey and kpp_compute_ss.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/asymmetric_keys/asymmetric_type.c | 77 
 include/keys/asymmetric-subtype.h| 12 +
 2 files changed, 89 insertions(+)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c 
b/crypto/asymmetric_keys/asymmetric_type.c
index 3cd4315..cdc0974e 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -578,6 +578,80 @@ static int asymmetric_key_verify_signature(struct 
kernel_pkey_params *params,
return verify_signature(params->key, );
 }
 
+int query_asym_kpp_key(const struct kernel_kpp_params *params,
+  struct kernel_kpp_query *res)
+{
+   const struct asymmetric_key_subtype *subtype;
+   struct key *key = params->key;
+   int ret;
+
+   pr_devel("==>%s()\n", __func__);
+
+   if (key->type != _type_asymmetric)
+   return -EINVAL;
+
+   subtype = asymmetric_key_subtype(key);
+   if (!subtype || !key->payload.data[0])
+   return -EINVAL;
+
+   if (!subtype->kpp_query)
+   return -ENOTSUPP;
+
+   ret = subtype->kpp_query(params, res);
+
+   pr_devel("<==%s() = %d\n", __func__, ret);
+   return ret;
+}
+
+int asymmetric_key_kpp_gen_pubkey(struct kernel_kpp_params *params, void *out)
+{
+   const struct asymmetric_key_subtype *subtype;
+   struct key *key = params->key;
+   int ret;
+
+   pr_devel("==>%s()\n", __func__);
+
+   if (key->type != _type_asymmetric)
+   return -EINVAL;
+
+   subtype = asymmetric_key_subtype(key);
+   if (!subtype || !key->payload.data[0])
+   return -EINVAL;
+
+   if (!subtype->kpp_gen_pubkey)
+   return -ENOTSUPP;
+
+   ret = subtype->kpp_gen_pubkey(params, out);
+
+   pr_devel("<==%s() = %d\n", __func__, ret);
+   return ret;
+}
+
+int asymmetric_key_kpp_compute_ss(struct kernel_kpp_params *params,
+ const void *in, void *out)
+{
+   const struct asymmetric_key_subtype *subtype;
+   struct key *key = params->key;
+   int ret;
+
+   pr_devel("==>%s()\n", __func__);
+
+   if (key->type != _type_asymmetric)
+   return -EINVAL;
+
+   subtype = asymmetric_key_subtype(key);
+   if (!subtype || !key->payload.data[0])
+   return -EINVAL;
+
+   if (!subtype->kpp_compute_ss)
+   return -ENOTSUPP;
+
+   ret = subtype->kpp_compute_ss(params, in, out);
+
+   pr_devel("<==%s() = %d\n", __func__, ret);
+   return ret;
+}
+
 struct key_type key_type_asymmetric = {
.name   = "asymmetric",
.preparse   = asymmetric_key_preparse,
@@ -591,6 +665,9 @@ struct key_type key_type_asymmetric = {
.asym_query = query_asymmetric_key,
.asym_eds_op= asymmetric_key_eds_op,
.asym_verify_signature  = asymmetric_key_verify_signature,
+   .asym_kpp_query = query_asym_kpp_key,
+   .asym_kpp_gen_pubkey= asymmetric_key_kpp_gen_pubkey,
+   .asym_kpp_compute_ss= asymmetric_key_kpp_compute_ss,
 };
 EXPORT_SYMBOL_GPL(key_type_asymmetric);
 
diff --git a/include/keys/asymmetric-subtype.h 
b/include/keys/asymmetric-subtype.h
index bd12733..5f9bece 100644
--- a/include/keys/asymmetric-subtype.h
+++ b/include/keys/asymmetric-subtype.h
@@ -20,6 +20,8 @@
 struct kernel_pkey_query;
 struct kernel_pkey_params;
 struct public_key_signature;
+struct kernel_kpp_query;
+struct kernel_kpp_params;
 
 /*
  * Keys of this type declare a subtype that indicates the handlers and
@@ -46,6 +48,16 @@ struct asymmetric_key_subtype {
/* Verify the signature on a key of this subtype (optional) */
int (*verify_signature)(const struct key *key,
const struct public_key_signature *sig);
+
+   int (*kpp_query)(const struct kernel_kpp_params *params,
+struct kernel_kpp_query *res);
+
+   /* Generate public key */
+   int (*kpp_gen_pubkey)(struct kernel_kpp_params *params, void *out);
+
+   /* Compute shared secret */
+   int (*kpp_compute_ss)(struct kernel_kpp_params *params,
+ const void *in, void *out);
 };
 
 /**
-- 
2.9.4

[RFC PATCH 4/5] KEYS: add asymmetric kpp subtype

[Tudor Ambarus]

Includes generation of public key and computation of shared secret.
This mostly involves offloading the calls to the crypto layer.

The crypto tfm was allocated and the private key was set when parsing
the private key. This permits us to use a single tfm whatever the
number of operation calls.

The private key, the corresponding public key, the pair's public key
and the shared secret are the same as those in crypto/testmgr.h:

# echo -n 
020028000200200024d121ebe5cf2d83f6621b6e43843aa38be086c32019da92505303e1c0eab882
 \
| xxd -r -p | keyctl padd asymmetric private @s
# keyctl kpp_query 266205365
max_size=64
kpp_gen_pubkey=y
kpp_compute_ss=y
# keyctl kpp_gen_pubkey 266205365 | xxd -p
1a7feb5200bd3c317db670c186a6c7c43bc55f6c6f583cf5b66382773324
a15f6aca436ff77eff023708cc405e7afd6a6a026e4187683877faa94443
2def09df
# echo -n 
ccb4da74b1473fea6c709e382dc7aab729b2470319abdd34bda82c93e1a474d96463f770202fa4e69f4a38ccc02c492fb132bbaf2261dacb6fdba9aafc7781f3
 | xxd -r -p > /tmp/bpub
# keyctl kpp_compute_ss 266205365 0 /tmp/bpub | xxd -p
ea176f7e6e5726388bfb41ebbac86da5a872d1ffc9473daa58439f340f8c
f3c9

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/asymmetric_keys/Kconfig|   7 ++
 crypto/asymmetric_keys/Makefile   |   1 +
 crypto/asymmetric_keys/asym_kpp.c | 142 ++
 include/crypto/asym_kpp_subtype.h |  12 
 4 files changed, 162 insertions(+)
 create mode 100644 crypto/asymmetric_keys/asym_kpp.c
 create mode 100644 include/crypto/asym_kpp_subtype.h

diff --git a/crypto/asymmetric_keys/Kconfig b/crypto/asymmetric_keys/Kconfig
index 66a7dad..1884570 100644
--- a/crypto/asymmetric_keys/Kconfig
+++ b/crypto/asymmetric_keys/Kconfig
@@ -21,6 +21,13 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
  appropriate hash algorithms (such as SHA-1) must be available.
  ENOPKG will be reported if the requisite algorithm is unavailable.
 
+config ASYMMETRIC_KPP_SUBTYPE
+   tristate "Asymmetric Key Protocol Primitives (KPP) subtype"
+   select CRYPTO_ECDH
+   help
+ This option provides support for KPP handling.
+ ENOPKG will be reported if the requisite algorithm is unavailable.
+
 config X509_CERTIFICATE_PARSER
tristate "X.509 certificate parser"
depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile
index a67ad83..d884cf1 100644
--- a/crypto/asymmetric_keys/Makefile
+++ b/crypto/asymmetric_keys/Makefile
@@ -11,6 +11,7 @@ asymmetric_keys-y := \
signature.o
 
 obj-$(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o
+obj-$(CONFIG_ASYMMETRIC_KPP_SUBTYPE) += asym_kpp.o
 
 #
 # X.509 Certificate handling
diff --git a/crypto/asymmetric_keys/asym_kpp.c 
b/crypto/asymmetric_keys/asym_kpp.c
new file mode 100644
index 000..77e085f
--- /dev/null
+++ b/crypto/asymmetric_keys/asym_kpp.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) "ASYM-KPP: "fmt
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * Provide a part of a description of the key for /proc/keys.
+ */
+static void asym_kpp_describe(const struct key *asymmetric_key,
+ struct seq_file *m)
+{
+   struct asym_kpp_ctx *ctx = asymmetric_key->payload.data[asym_crypto];
+
+   if (ctx)
+   seq_printf(m, "%s", ctx->alg_name);
+}
+
+static void free_kpp_ctx(struct asym_kpp_ctx *ctx)
+{
+   if (ctx) {
+   if (ctx->tfm)
+   crypto_free_kpp(ctx->tfm);
+   kfree(ctx);
+   }
+}
+
+static void asym_kpp_destroy(void *payload0, void *payload3)
+{
+   free_kpp_ctx(payload0);
+}
+
+/*
+ * Query information about a key.
+ */
+static int software_kpp_query(const struct kernel_kpp_params *params,
+ struct kernel_kpp_query *res)
+{
+   struct asym_kpp_ctx *ctx = params->key->payload.data[asym_crypto];
+
+   res->max_size = crypto_kpp_maxsize(ctx->tfm);
+   res->supported_ops = KEYCTL_KPP_GEN_PUBKEY | KEYCTL_KPP_COMPUTE_SS;
+
+   pr_devel("<==%s() = %d\n", __func__, 0);
+   return 0;
+}
+
+/*
+ * Generate public key.
+ */
+static int software_kpp_gen_pubkey(struct kernel_kpp_params *params, void *out)
+{
+   struct crypto_wait cwait;
+   const struct asym_kpp_ctx *ctx = params->key->payload.data[asym_crypto];
+   struct kpp_request *req;
+   struct scatterlist out_sg;
+   int ret;
+
+   pr_devel("==>%s()\n", __func__);
+
+   req = kpp_request_alloc(ctx->tfm, GFP_KERNEL);
+   if (!req)
+   return -ENOMEM;
+
+   kpp_request_set_input(req, NULL, 0);
+   sg_init_one(_sg, out, params->out_len);
+   kpp_request_set_output(req, _

[RFC PATCH 2/5] KEYS: Provide keyctls to drive the new key type ops for kpp

[Tudor Ambarus]

Provide three keyctl functions that permit userspace to make use of the new
key type ops for accessing and driving asymmetric kpp keys.

(*) Query an asymmetric kpp key.

long keyctl(KEYCTL_KPP_QUERY,
key_serial_t key, struct keyctl_kpp_query *res);

Get information about an asymmetric kpp key. The information is
returned in the keyctl_kpp_query struct:

__u32   supported_ops;

A bit mask of flags indicating which ops are supported. This
is constructed from a bitwise-OR of:

KEYCTL_SUPPORTS_{GEN_PUBKEY, COMPUTE_SS}

__u32   max_size;

The maximum size in bytes of the key.

__spare must be set to 0.  This is intended for future use to hand
over one or more passphrases needed to unlock a key.

If successful, 0 is returned.  If the key is not an asymmetric kpp key,
EOPNOTSUPP is returned.

(*) Generate the public key or compute the shared secret using an
asymmetric kpp key.

long keyctl(KEYCTL_KPP_GEN_PUBKEY,
const struct keyctl_kpp_params *params,
void *out);

long keyctl(KEYCTL_KPP_GEN_PUBKEY,
const struct keyctl_kpp_params *params,
const void *in, void *out);

The parameter block pointed by params contains a number of integer
values:
__s32   key_id;
__u32   in_len;
__u32   out_len;

For a given operation, the in and out buffers are used as follows:

Operation IDin,in_lenout,out_len
=== ===  
KEYCTL_KPP_GEN_PUBKEY   -Corresponding public key
KEYCTL_KPP_COMPUTE_SS   Pair's public keyShared Secret

The __spare space in the parameter block must be set to 0.  This is
intended, amongst other things, to allow the passing of passphrases
required to unlock a key.

If successful, the public key generation and the shared secret computation
will return the amount of data written into the output buffer.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 Documentation/security/keys/core.rst |  59 ++
 include/uapi/linux/keyctl.h  |  16 +++
 security/keys/Makefile   |   1 +
 security/keys/compat.c   |  10 ++
 security/keys/internal.h |  28 +
 security/keys/keyctl.c   |  13 +++
 security/keys/keyctl_kpp.c   | 205 +++
 7 files changed, 332 insertions(+)
 create mode 100644 security/keys/keyctl_kpp.c

diff --git a/Documentation/security/keys/core.rst 
b/Documentation/security/keys/core.rst
index 9b69a1f..31b9501 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -994,6 +994,65 @@ The keyctl syscall functions are:
  If successful, encrypt, decrypt and sign all return the amount of data
  written into the output buffer.  Verification returns 0 on success.
 
+  *  Query an asymmetric kpp key::
+
+   long keyctl(KEYCTL_KPP_QUERY,
+   key_serial_t key, struct keyctl_kpp_query *res);
+
+ Get information about an asymmetric kpp key. The information is
+ returned in the keyctl_kpp_query struct::
+
+   struct keyctl_kpp_query {
+   __u32   supported_ops;  /* Which ops are supported */
+   __u32   max_size;   /* Maximum size of the output 
buffer in bytes */
+   __u32   __spare[10];
+   };
+
+ ``__u32   supported_ops;`` is a bit mask of flags indicating which ops are
+ supported.  This is constructed from a bitwise-OR of::
+
+KEYCTL_SUPPORTS_{GEN_PUBKEY, COMPUTE_SS}
+
+ ``__spare`` must be set to 0.  This is intended for future use to hand
+ over one or more passphrases needed to unlock a key.
+
+ If successful, 0 is returned.  If the key is not an asymmetric kpp key,
+ EOPNOTSUPP is returned.
+
+  *  Generate the public key or compute the shared secret using an asymmetric
+ kpp key::
+
+   long keyctl(KEYCTL_KPP_GEN_PUBKEY,
+   const struct keyctl_kpp_params *params,
+   void *out);
+
+   long keyctl(KEYCTL_KPP_GEN_PUBKEY,
+   const struct keyctl_kpp_params *params,
+   const void *in, void *out);
+
+ The parameter block pointed by params contains a number of integer
+ values::
+__s32   key_id;
+__u32   in_len;
+__u32   out_len;
+
+ ``key_id`` is the key tp be used and ``in_len`` and ``out_len`` are the
+ lengths in bytes of the input and output buffers.
+
+ For a given operation, the in and out buffers are used as follows::
+
+   Operation IDin,in_lenout,o

[RFC PATCH 1/5] KEYS: Provide key type operations for kpp ops

[Tudor Ambarus]

Provide three new operations in the key_type struct that can be used to
provide access to kpp operations. These will be implemented for the
asymmetric key type in a later patch and may refer to a key retained in
RAM by the kernel or a key retained in crypto hardware.

int (*asym_kpp_query)(const struct kernel_kpp_params *params,
  struct kernel_kpp_query *res);
int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params,
   void *out);
int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params,
   const void *in, void *out);

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 Documentation/security/keys/core.rst | 54 
 include/linux/key-type.h |  7 +
 include/linux/keyctl.h   | 11 
 include/uapi/linux/keyctl.h  |  3 ++
 4 files changed, 75 insertions(+)

diff --git a/Documentation/security/keys/core.rst 
b/Documentation/security/keys/core.rst
index d224fd0..9b69a1f 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1688,6 +1688,60 @@ The structure has a number of fields, some of which are 
mandatory:
  If successful, 0 will be returned.  If the key doesn't support this,
  EOPNOTSUPP will be returned.
 
+  *  ``int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params, void 
*out);``
+  *  ``int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params, const void 
*in, void *out);``
+
+ These methods are optional. If provided the first allows to generate the
+ public key pair corresponding to the private key. The second method allows
+ to generate a shared secret by  combining the private key and the other
+ party's public key.
+
+ In all cases, the following information is provided in the params block::
+
+   struct kernel_kpp_query {
+   struct key  *key;
+   __u32   in_len; /* Input data size */
+   __u32   out_len;/* Output buffer size */
+   }
+
+ This includes the key to be used and the sizes in bytes of the input and
+ output buffers.
+
+ For a given operation, the in and out buffers are used as follows::
+
+   Operation IDin,in_lenout,out_len
+   === ===  
+   KEYCTL_KPP_GEN_PUBKEY   -Corresponding public key
+   KEYCTL_KPP_COMPUTE_SS   Pair's public keyShared Secret
+
+ If successful, the public key generation and the shared secret computation
+ will return the amount of data written into the output buffer.
+
+  *  ``int (*asym_kpp_query)(const struct kernel_kpp_params *params, struct 
kernel_kpp_query *res);``
+
+ This method is optional. If provided it allows information about the
+ asymmetric KPP (Key Protocol Primitives) key held in the key to be
+ determined.
+
+ The ``params`` block will contain the key to be queried. ``in_len`` and
+ ``out_len`` are unused.
+
+ If successful, the following information is filled in::
+
+   struct kernel_kpp_query {
+   __u32   supported_ops;  /* Which ops are supported */
+   __u32   max_size;   /* Maximum size of the output 
buffer */
+   };
+
+ The supported_ops field will contain a bitmask indicating what operations
+ are supported by the key, including public key generation and shared
+ secret computation. The following constants are defined for this::
+
+   KEYCTL_SUPPORTS_{GEN_PUBKEY, COMPUTE_SS};
+
+ If successful, 0 is returned.  If the key is not an asymmetric kpp key,
+ EOPNOTSUPP is returned.
+
 
 Request-Key Callback Service
 
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index bc9af55..d354b6b 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -19,6 +19,8 @@
 
 struct kernel_pkey_query;
 struct kernel_pkey_params;
+struct kernel_kpp_query;
+struct kernel_kpp_params;
 
 /*
  * key under-construction record
@@ -165,6 +167,11 @@ struct key_type {
   const void *in, void *out);
int (*asym_verify_signature)(struct kernel_pkey_params *params,
 const void *in, const void *in2);
+   int (*asym_kpp_query)(const struct kernel_kpp_params *params,
+ struct kernel_kpp_query *res);
+   int (*asym_kpp_gen_pubkey)(struct kernel_kpp_params *params, void *out);
+   int (*asym_kpp_compute_ss)(struct kernel_kpp_params *params,
+  const void *in, void *out);
 
/* internal fields */
struct list_headlink;   /* link in types list */
diff --git a/include/linux/keyctl.h b/include/linux/keyctl.h
index e89b4a4..8f464ea 100644
--- a/include/linux/keyctl.h

[RFC PATCH 0/5] KEYS: add kpp keyctl operations

[Tudor Ambarus]

This series provides keyctl access for kpp operations, including
a query function, a function to generate the public key that is
associated with the private key and a function to compute the
shared secret.

I've added a KPP ecdh parser so that you can load an ECDH private
key into the kernel. The ECDH private keys are expected to be encoded
with the ecdh helpers from kernel. If the private key is valid, the
parser will allocate a tfm and set the private key. There is a
one-to-one binding between the private key and the tfm. The tfm will be
associated with the key for the entire life of the key. The tfm is
allocated once and used as many times as needed.

The kernel patches can be found here also:

https://github.com/ambarus/linux/commits/keys-kpp

The keyutils changes can be found here:

https://github.com/ambarus/keyutils/commits/keys-kpp

These patches are similar to what David Howells proposed for akcipher:

https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19915.html

I've ported David's patches on top of keys-next and then I've added my
patches on top of them.

Both proposals, David's and mine, lack support for accessing TPM.
Before getting familiar with TPM, please let me know how you feel
about this series.

Tudor Ambarus (5):
  KEYS: Provide key type operations for kpp ops
  KEYS: Provide keyctls to drive the new key type ops for kpp
  KEYS: Provide missing asym kpp subops for new key type ops
  KEYS: add asymmetric kpp subtype
  KEYS: add KPP ecdh parser

 Documentation/security/keys/core.rst | 113 +
 crypto/asymmetric_keys/Kconfig   |  15 +++
 crypto/asymmetric_keys/Makefile  |   6 +
 crypto/asymmetric_keys/asym_kpp.c| 142 +
 crypto/asymmetric_keys/asymmetric_type.c |  77 
 crypto/asymmetric_keys/kpp_parser.c  | 124 +++
 include/crypto/asym_kpp_subtype.h|  14 +++
 include/keys/asymmetric-subtype.h|  12 ++
 include/linux/key-type.h |   7 ++
 include/linux/keyctl.h   |  11 ++
 include/uapi/linux/keyctl.h  |  19 +++
 security/keys/Makefile   |   1 +
 security/keys/compat.c   |  10 ++
 security/keys/internal.h |  28 +
 security/keys/keyctl.c   |  13 ++
 security/keys/keyctl_kpp.c   | 205 +++
 16 files changed, 797 insertions(+)
 create mode 100644 crypto/asymmetric_keys/asym_kpp.c
 create mode 100644 crypto/asymmetric_keys/kpp_parser.c
 create mode 100644 include/crypto/asym_kpp_subtype.h
 create mode 100644 security/keys/keyctl_kpp.c

-- 
2.9.4

Re: [PATCH] crypto: atmel-aes - fix the keys zeroing on errors

[Tudor Ambarus]

Thanks Antoine,

On 02/23/2018 11:01 AM, Antoine Tenart wrote:

The Atmel AES driver uses memzero_explicit on the keys on error, but the
variable zeroed isn't the right one because of a typo. Fix this by using
the right variable.

Fixes: 89a82ef87e01 ("crypto: atmel-authenc - add support to authenc(hmac(shaX), 
Y(aes)) modes")
Signed-off-by: Antoine Tenart<antoine.ten...@bootlin.com>


Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>

There are few other places in crypto where we extract the authenc keys
in the same type of local variable, struct crypto_authenc_keys keys, and
we don't zeroize it after use. I think we should fix those cases too.

Bye,
ta

Re: [PATCH] Remove useless assignment in rsa_verify

[Tudor Ambarus]

Hi, Ben,

On 02/22/2018 07:16 PM, C0deAi wrote:

Hi my name is Benjamin Bales.

I am the founder and creator of CodeAI,
the first non-human contributor to your software project. CodeAI finds
and fixes security defects for you. It fixed 327. It wants to merge a
fix for a useless assignment. To view all 327 fixed issues from the
run claim your free open source account at mycode.ai and the
Dockerfile used to build and run your project in CodeAI, here-
https://drive.google.com/drive/folders/1KB9WQQyWQgYccmiSjy2E1JWJ4vWuoLYd
.
It is always free for open source projects.

If you have any questions about these results or have general
inquiries about CodeAI, please send an email to techsupp...@mycode.ai

Signed-off-by: Benjamin Bales 
---
  crypto/rsa.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/crypto/rsa.c b/crypto/rsa.c
index b067f3a..4167980 100644
--- a/crypto/rsa.c
+++ b/crypto/rsa.c
@@ -215,7 +215,6 @@ static int rsa_verify(struct akcipher_request *req)
goto err_free_m;
}
  
-	ret = -ENOMEM;

s = mpi_read_raw_from_sgl(req->src, req->src_len);
if (!s) {
ret = -ENOMEM;



ret is also initialized at declaration, we can remove the assignment to
zero too. This applies to rsa_enc/dec/sign/verify. Also, I think it's
better to benefit of the power of undefined values and set the err/ret
in every branch (see https://rusty.ozlabs.org/?p=232).

Take care,
ta

Re: [PATCH] crypto: atmel: Delete error messages for a failed memory allocation in six functions

[Tudor Ambarus]

On 02/15/2018 02:24 PM, SF Markus Elfring wrote:

From: Markus Elfring<elfr...@users.sourceforge.net>
Date: Thu, 15 Feb 2018 11:38:30 +0100

Omit extra messages for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring<elfr...@users.sourceforge.net>


Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>

[PATCH] crypto: tcrypt - set assoc in sg_init_aead()

[Tudor Ambarus]

Results better code readability.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
Should be applied after:
crypto: tcrypt - fix S/G table for test_aead_speed()

 crypto/tcrypt.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 3ced1ba..28b4882 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -185,7 +185,8 @@ static void testmgr_free_buf(char *buf[XBUFSIZE])
 }
 
 static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE],
-   unsigned int buflen)
+unsigned int buflen, const void *assoc,
+unsigned int aad_size)
 {
int np = (buflen + PAGE_SIZE - 1)/PAGE_SIZE;
int k, rem;
@@ -198,6 +199,9 @@ static void sg_init_aead(struct scatterlist *sg, char 
*xbuf[XBUFSIZE],
}
 
sg_init_table(sg, np + 1);
+
+   sg_set_buf([0], assoc, aad_size);
+
if (rem)
np--;
for (k = 0; k < np; k++)
@@ -318,14 +322,12 @@ static void test_aead_speed(const char *algo, int enc, 
unsigned int secs,
goto out;
}
 
-   sg_init_aead(sg, xbuf,
-   *b_size + (enc ? 0 : authsize));
+   sg_init_aead(sg, xbuf, *b_size + (enc ? 0 : authsize),
+assoc, aad_size);
 
sg_init_aead(sgout, xoutbuf,
-   *b_size + (enc ? authsize : 0));
-
-   sg_set_buf([0], assoc, aad_size);
-   sg_set_buf([0], assoc, aad_size);
+*b_size + (enc ? authsize : 0), assoc,
+aad_size);
 
aead_request_set_crypt(req, sg, sgout,
   *b_size + (enc ? 0 : authsize),
-- 
2.9.4

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Tudor Ambarus]

Hi,

On 11/12/2017 06:26 PM, Horia Geantă wrote:


-sg[0] - (1 entry) reserved for associated data, filled outside
sg_init_aead()


Let's fill the sg[0] with aad inside sg_init_aead()!

Cheers,
ta

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Tudor Ambarus]

Hi,

On 10/10/2017 01:21 PM, Robert Baronescu wrote:

In case buffer length is a multiple of PAGE_SIZE,
the S/G table is incorrectly generated.
Fix this by handling buflen = k * PAGE_SIZE separately.

Signed-off-by: Robert Baronescu 
---
  crypto/tcrypt.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)


This patch fixes the segmentation fault listed below. The NULL
dereference can be seen starting with:
7aacbfc crypto: tcrypt - fix buffer lengths in test_aead_speed()

Cheers,
ta

# insmod tcrypt.ko mode=212

testing speed of rfc4309(ccm(aes)) 
(rfc4309(ccm_base(ctr(aes-generic),cbcmac(aes-generic encryption

test 0 (152 bit key, 16 byte blocks):
1 operation in 0 cycles (16 bytes)
test 1 (152 bit key, 64 byte blocks):
1 operation in 0 cycles (64 bytes)
test 2 (152 bit key, 256 byte blocks):
1 operation in 0 cycles (256 bytes)
test 3 (152 bit key, 512 byte blocks):
1 operation in 0 cycles (512 bytes)
test 4 (152 bit key, 1024 byte blocks):
1 operation in 0 cycles (1024 bytes)
test 5 (152 bit key, 2048 byte blocks):
1 operation in 0 cycles (2048 bytes)
test 6 (152 bit key, 4096 byte blocks):
Unable to handle kernel NULL pointer dereference at virtual address 0004
pgd = deee
[0004] *pgd=3f6b8831, *pte=, *ppte=
Internal error: Oops: 17 [#1] ARM
Modules linked in: tcrypt(+)
CPU: 0 PID: 795 Comm: insmod Not tainted 4.14.0-rc3+ #15
Hardware name: Atmel SAMA5
task: def4d000 task.stack: def4a000
PC is at scatterwalk_copychunks+0x14c/0x18c
LR is at scatterwalk_copychunks+0x144/0x18c
pc : []lr : []psr: 2013
sp : def4bbf8  ip :   fp : def4bcb4
r10: c02d1e5c  r9 :   r8 : def4a000
r7 : defd0090  r6 : def4bc58  r5 : 0010  r4 : 
r3 : dffe71e2  r2 : def4d000  r1 :   r0 : 
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c53c7d  Table: 3eee0059  DAC: 0051
Process insmod (pid: 795, stack limit = 0xdef4a208)
Stack: (0xdef4bbf8 to 0xdef4c000)
bbe0:   def4bc48 
0010
bc00: def4bcbc  0010  c02d1e5c c02c47f0 0010 
def4bc28
bc20: deefe110  deefe200 def11800 c02d1e5c c02cc178 00e7 
def4bc38
bc40: 0010 def4bcbc dffd8fc0 defd0090 dffd8fc0 defd0080  

bc60: 1000 def7e2a0  1000  defd0080 deefe200 
0010
bc80:  0010 0001   c02cc0bc  
ded1a4c0
bca0: 1000 deefe200 deefe0c0 deefe134 deefe164 c02c509c 1000 
deda5280
bcc0: deefe200 0400 deefe100 c02cec9c def4bd70 deefe000  
deefe000
bce0:  0004  def7e200 bf007144 ded19300  
bf001950
bd00: 014000c0 bf007234  0010 bf0075c0 def7e290 deda7a80 
0006
bd20: c0a4bd38 1000  ded19300 bf007140 ded19340  
defd0f00
bd40:  def4bd44 def4bd44 c0176ea4 df60f000 def5c000 def5e000 
deff1000
bd60: df4a5000 df651000 df648000 df646000 deebe000 dee59000 deeae000 
defd1000
bd80: deda defd3000 de806000 def82000 def63000 def78000 deec7000 
deeff000
bda0: deeb9000 deef2000 deeba000 deebd000   0004 
bf0075c0
bdc0: bf007440 defd0f00 bf007488 0001 2102f11c bf005238 df4ac000 
75c0
bde0: 0003 bf0075c0 bf007440 bf0075c0 0004 bf0075c0 bf007440 
defd0f00
be00: bf007488 bf00a054 bf007440 bf00a000  c01018e8  
ded17780
be20: df4ac000 c0a3a72c df42 c0844a4c c07df704 c01a5054 bf007488 
c0684d38
be40: 0012 deda7440 defd0f08 a013 deda7640 e0a7e000 0001 
defd0f00
be60: bf007440 defd0f08 deda7640 defd0f00 bf007488 c016203c bf007488 
0001
be80: def4bf50 defd0f08 0001 c0161390 bf00744c 7fff bf007440 
c015ea8c
bea0:  bf007590 0578 bf007528 c0844c7c c07018f0 c01b1060 
bf00
bec0: dcfb dcfb      

bee0:        

bf00:   7fff  0003 00099008 017b 
c0107964
bf20: def4a000   c0161a68 7fff  0003 
a013
bf40: dedd1c00 e0a7e000 dcfb  e0a83d03 e0a7e000 dcfb 
e0a85238
bf60: e0a850dd e0a8b258 8000 81d0    
2e84
bf80: 0021 0022 0019  0013  00099008 
bebd1f45
bfa0: 0003 c01077a0 00099008 bebd1f45 0003 00099008  
bebd1f45
bfc0: 00099008 bebd1f45 0003 017b bebd1f45   

bfe0: bebd1ca8 bebd1c98 0001f99d b6f3f2c4 8030 0003  

[] (scatterwalk_copychunks) from [] 
(blkcipher_walk_next+0x3a0/0x44c)
[] (blkcipher_walk_next) from [] 
(crypto_ctr_crypt+0xbc/0x1cc)
[] (crypto_ctr_crypt) from [] 
(skcipher_encrypt_blkcipher+0x44/0x4c)
[] (skcipher_encrypt_blkcipher) from [] 
(crypto_ccm_encrypt+0xc8/0xf8)
[] (crypto_ccm_encrypt) from [] 
(test_aead_speed.constprop.2+0x3e8/0x5a8 [tcrypt])
[] 

Re: [PATCH v2 5/5] crypto: dh - Remove pointless checks for NULL 'p' and 'g'

[Tudor Ambarus]

On 11/06/2017 04:30 AM, Eric Biggers wrote:

From: Eric Biggers <ebigg...@google.com>

Neither 'p' nor 'g' can be NULL, as they were unpacked using
crypto_dh_decode_key().  And it makes no sense for them to be optional.
So remove the NULL checks that were copy-and-pasted into both modules.



Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>


Signed-off-by: Eric Biggers <ebigg...@google.com>
---
  crypto/dh.c   | 3 ---
  drivers/crypto/qat/qat_common/qat_asym_algs.c | 3 ---
  2 files changed, 6 deletions(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index aadaf36fb56f..5659fe7f446d 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -53,9 +53,6 @@ static int dh_check_params_length(unsigned int p_len)
  
  static int dh_set_params(struct dh_ctx *ctx, struct dh *params)

  {
-   if (unlikely(!params->p || !params->g))
-   return -EINVAL;
-
if (dh_check_params_length(params->p_size << 3))
return -EINVAL;
  
diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c

index 7655fdb499de..13c52d6bf630 100644
--- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
@@ -443,9 +443,6 @@ static int qat_dh_set_params(struct qat_dh_ctx *ctx, struct 
dh *params)
struct qat_crypto_instance *inst = ctx->inst;
struct device *dev = _DEV(inst->accel_dev);
  
-	if (unlikely(!params->p || !params->g))

-   return -EINVAL;
-
if (qat_dh_check_params_length(params->p_size << 3))
return -EINVAL;
  

Re: [PATCH v2 3/5] crypto: dh - Don't permit 'key' or 'g' size longer than 'p'

[Tudor Ambarus]

On 11/06/2017 04:30 AM, Eric Biggers wrote:

From: Eric Biggers <ebigg...@google.com>

The "qat-dh" DH implementation assumes that 'key' and 'g' can be copied
into a buffer with size 'p_size'.  However it was never checked that
that was actually the case, which most likely allowed users to cause a
buffer underflow via KEYCTL_DH_COMPUTE.

Fix this by updating crypto_dh_decode_key() to verify this precondition
for all DH implementations.

Fixes: c9839143ebbf ("crypto: qat - Add DH support")
Cc: <sta...@vger.kernel.org> # v4.8+


Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>


Signed-off-by: Eric Biggers <ebigg...@google.com>
---
  crypto/dh_helper.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c
index 708ae20d2d3c..7f00c771fe8d 100644
--- a/crypto/dh_helper.c
+++ b/crypto/dh_helper.c
@@ -83,6 +83,14 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, 
struct dh *params)
if (secret.len != crypto_dh_key_len(params))
return -EINVAL;
  
+	/*

+* Don't permit the buffer for 'key' or 'g' to be larger than 'p', since
+* some drivers assume otherwise.
+*/
+   if (params->key_size > params->p_size ||
+   params->g_size > params->p_size)
+   return -EINVAL;
+
/* Don't allocate memory. Set pointers to data within
 * the given buffer
 */

Re: [PATCH v2 1/5] crypto: dh - Fix double free of ctx->p

[Tudor Ambarus]

On 11/06/2017 04:30 AM, Eric Biggers wrote:

From: Eric Biggers <ebigg...@google.com>

When setting the secret with the software Diffie-Hellman implementation,
if allocating 'g' failed (e.g. if it was longer than
MAX_EXTERN_MPI_BITS), then 'p' was freed twice: once immediately, and
once later when the crypto_kpp tfm was destroyed.

Fix it by using dh_free_ctx() (renamed to dh_clear_ctx()) in the error
paths, as that correctly sets the pointers to NULL.

KASAN report:

 MPI: mpi too large (32760 bits)
 ==
 BUG: KASAN: use-after-free in mpi_free+0x131/0x170
 Read of size 4 at addr 88006c7cdf90 by task reproduce_doubl/367

 CPU: 1 PID: 367 Comm: reproduce_doubl Not tainted 
4.14.0-rc7-00040-g05298abde6fe #7
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
01/01/2011
 Call Trace:
  dump_stack+0xb3/0x10b
  ? mpi_free+0x131/0x170
  print_address_description+0x79/0x2a0
  ? mpi_free+0x131/0x170
  kasan_report+0x236/0x340
  ? akcipher_register_instance+0x90/0x90
  __asan_report_load4_noabort+0x14/0x20
  mpi_free+0x131/0x170
  ? akcipher_register_instance+0x90/0x90
  dh_exit_tfm+0x3d/0x140
  crypto_kpp_exit_tfm+0x52/0x70
  crypto_destroy_tfm+0xb3/0x250
  __keyctl_dh_compute+0x640/0xe90
  ? kasan_slab_free+0x12f/0x180
  ? dh_data_from_key+0x240/0x240
  ? key_create_or_update+0x1ee/0xb20
  ? key_instantiate_and_link+0x440/0x440
  ? lock_contended+0xee0/0xee0
  ? kfree+0xcf/0x210
  ? SyS_add_key+0x268/0x340
  keyctl_dh_compute+0xb3/0xf1
  ? __keyctl_dh_compute+0xe90/0xe90
  ? SyS_add_key+0x26d/0x340
  ? entry_SYSCALL_64_fastpath+0x5/0xbe
  ? trace_hardirqs_on_caller+0x3f4/0x560
  SyS_keyctl+0x72/0x2c0
  entry_SYSCALL_64_fastpath+0x1f/0xbe
 RIP: 0033:0x43ccf9
 RSP: 002b:7ffeeec96158 EFLAGS: 0246 ORIG_RAX: 00fa
 RAX: ffda RBX: 0248b9b9 RCX: 0043ccf9
 RDX: 7ffeeec96170 RSI: 7ffeeec96160 RDI: 0017
 RBP: 0046 R08:  R09: 0248b9b9143dc936
 R10: 1000 R11: 0246 R12: 
 R13: 00409670 R14: 00409700 R15: 

 Allocated by task 367:
  save_stack_trace+0x16/0x20
  kasan_kmalloc+0xeb/0x180
  kmem_cache_alloc_trace+0x114/0x300
  mpi_alloc+0x4b/0x230
  mpi_read_raw_data+0xbe/0x360
  dh_set_secret+0x1dc/0x460
  __keyctl_dh_compute+0x623/0xe90
  keyctl_dh_compute+0xb3/0xf1
  SyS_keyctl+0x72/0x2c0
  entry_SYSCALL_64_fastpath+0x1f/0xbe

 Freed by task 367:
  save_stack_trace+0x16/0x20
  kasan_slab_free+0xab/0x180
  kfree+0xb5/0x210
  mpi_free+0xcb/0x170
  dh_set_secret+0x2d7/0x460
  __keyctl_dh_compute+0x623/0xe90
  keyctl_dh_compute+0xb3/0xf1
  SyS_keyctl+0x72/0x2c0
  entry_SYSCALL_64_fastpath+0x1f/0xbe

Fixes: 802c7f1c84e4 ("crypto: dh - Add DH software implementation")
Cc: <sta...@vger.kernel.org> # v4.8+
Signed-off-by: Eric Biggers <ebigg...@google.com>


Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>


---
  crypto/dh.c | 33 +
  1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index b1032a5c1bfa..aadaf36fb56f 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -21,19 +21,12 @@ struct dh_ctx {
MPI xa;
  };
  
-static inline void dh_clear_params(struct dh_ctx *ctx)

+static void dh_clear_ctx(struct dh_ctx *ctx)
  {
mpi_free(ctx->p);
mpi_free(ctx->g);
-   ctx->p = NULL;
-   ctx->g = NULL;
-}
-
-static void dh_free_ctx(struct dh_ctx *ctx)
-{
-   dh_clear_params(ctx);
mpi_free(ctx->xa);
-   ctx->xa = NULL;
+   memset(ctx, 0, sizeof(*ctx));
  }
  
  /*

@@ -71,10 +64,8 @@ static int dh_set_params(struct dh_ctx *ctx, struct dh 
*params)
return -EINVAL;
  
  	ctx->g = mpi_read_raw_data(params->g, params->g_size);

-   if (!ctx->g) {
-   mpi_free(ctx->p);
+   if (!ctx->g)
return -EINVAL;
-   }
  
  	return 0;

  }
@@ -86,21 +77,23 @@ static int dh_set_secret(struct crypto_kpp *tfm, const void 
*buf,
struct dh params;
  
  	/* Free the old MPI key if any */

-   dh_free_ctx(ctx);
+   dh_clear_ctx(ctx);
  
  	if (crypto_dh_decode_key(buf, len, ) < 0)

-   return -EINVAL;
+   goto err_clear_ctx;
  
  	if (dh_set_params(ctx, ) < 0)

-   return -EINVAL;
+   goto err_clear_ctx;
  
  	ctx->xa = mpi_read_raw_data(params.key, params.key_size);

-   if (!ctx->xa) {
-   dh_clear_params(ctx);
-   return -EINVAL;
-   }
+   if (!ctx->xa)
+   goto err_clear_ctx;
  
  	return 0;

+
+

Re: [PATCH 2/4] crypto: dh - don't permit 'p' to be 0

[Tudor Ambarus]

On 11/02/2017 12:25 AM, Eric Biggers wrote:

From: Eric Biggers <ebigg...@google.com>

If 'p' is 0 for the software Diffie-Hellman implementation, then
dh_max_size() returns 0.  In the case of KEYCTL_DH_COMPUTE, this causes
ZERO_SIZE_POINTER to be passed to sg_init_one(), which with
CONFIG_DEBUG_SG=y triggers the 'BUG_ON(!virt_addr_valid(buf));' in
sg_set_buf().

Fix this by making crypto_dh_decode_key() reject 0 for 'p'.  p=0 makes
no sense for any DH implementation because 'p' is supposed to be a prime
number.  Moreover, 'mod 0' is not mathematically defined.

Bug report:

 kernel BUG at ./include/linux/scatterlist.h:140!
 invalid opcode:  [#1] SMP KASAN
 CPU: 0 PID: 27112 Comm: syz-executor2 Not tainted 
4.14.0-rc7-00010-gf5dbb5d0ce32-dirty #7
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.3-20171021_125229-anatol 04/01/2014
 task: 88006caac0c0 task.stack: 88006c7c8000
 RIP: 0010:sg_set_buf include/linux/scatterlist.h:140 [inline]
 RIP: 0010:sg_init_one+0x1b3/0x240 lib/scatterlist.c:156
 RSP: 0018:88006c7cfb08 EFLAGS: 00010216
 RAX: 0001 RBX: 88006c7cfe30 RCX: 64ee
 RDX: 81cf64c3 RSI: c9d72000 RDI: 92e937e0
 RBP: 88006c7cfb30 R08: ed000d8f9fab R09: 88006c7cfd30
 R10: 0005 R11: ed000d8f9faa R12: 88006c7cfd30
 R13:  R14: 0010 R15: 88006c7cfc50
 FS:  7fce190fa700() GS:88003ea0() 
knlGS:
 CS:  0010 DS:  ES:  CR0: 80050033
 CR2: 7fffc6b33db8 CR3: 3cf64000 CR4: 06f0
 Call Trace:
  __keyctl_dh_compute+0xa95/0x19b0 security/keys/dh.c:360
  keyctl_dh_compute+0xac/0x100 security/keys/dh.c:434
  SYSC_keyctl security/keys/keyctl.c:1745 [inline]
  SyS_keyctl+0x72/0x2c0 security/keys/keyctl.c:1641
  entry_SYSCALL_64_fastpath+0x1f/0xbe
 RIP: 0033:0x4585c9
 RSP: 002b:7fce190f9bd8 EFLAGS: 0216 ORIG_RAX: 00fa
 RAX: ffda RBX: 00738020 RCX: 004585c9
 RDX: 2000d000 RSI: 2ff4 RDI: 0017
 RBP: 0046 R08: 20008000 R09: 
 R10:  R11: 0216 R12: 7fff6e610cde
 R13: 7fff6e610cdf R14: 7fce190fa700 R15: 
 Code: 03 0f b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 04 84 d2 75 33 5b 45 89 
6c 24 14 41 5c 41 5d 41 5e 41 5f 5d c3 e8 fd 8f 68 ff <0f> 0b e8 f6 8f 68 ff 0f 
0b e8 ef 8f 68 ff 0f 0b e8 e8 8f 68 ff 20
 RIP: sg_set_buf include/linux/scatterlist.h:140 [inline] RSP: 
88006c7cfb08
 RIP: sg_init_one+0x1b3/0x240 lib/scatterlist.c:156 RSP: 88006c7cfb08

Fixes: 802c7f1c84e4 ("crypto: dh - Add DH software implementation")
Cc: <sta...@vger.kernel.org> # v4.8+
Signed-off-by: Eric Biggers <ebigg...@google.com>


Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>

[v3 PATCH 2/3] crypto: atmel-aes/tdes - remove empty functions

[Tudor Ambarus]

Pointer members of an object with static storage duration, if not
explicitly initialized, will be initialized to a NULL pointer.
The crypto API checks if these pointers are not NULL before using them,
therefore we can safely remove these empty functions.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
changes in v3:
- update the commit message
changes in v2:
- remove empty atmel_aes_gcm_exit()

 drivers/crypto/atmel-aes.c  | 20 
 drivers/crypto/atmel-tdes.c | 18 --
 2 files changed, 38 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 9659759..c2f0a12 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -1237,10 +1237,6 @@ static int atmel_aes_ctr_cra_init(struct crypto_tfm *tfm)
return 0;
 }
 
-static void atmel_aes_cra_exit(struct crypto_tfm *tfm)
-{
-}
-
 static struct crypto_alg aes_algs[] = {
 {
.cra_name   = "ecb(aes)",
@@ -1253,7 +1249,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1273,7 +1268,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1294,7 +1288,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1315,7 +1308,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1336,7 +1328,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1357,7 +1348,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1378,7 +1368,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1399,7 +1388,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_ctr_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1422,7 +1410,6 @@ static struct crypto_alg aes_cfb64_alg = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1809,18 +1796,12 @@ static int atmel_aes_gcm_init(struct crypto_aead *tfm)
return 0;
 }
 
-static void atmel_aes_gcm_exit(struct crypto_aead *tfm)
-{
-
-}
-
 static struct aead_alg aes_gcm_alg = {
.setkey = atmel_aes_gcm_setkey,
.setauthsize= atmel_aes_gcm_setauthsize,
.encrypt= atmel_aes_gcm_encrypt,

[PATCH] crypto: ecdh - remove empty exit()

[Tudor Ambarus]

Pointer members of an object with static storage duration, if not
explicitly initialized, will be initialized to a NULL pointer. The crypto
API checks if this pointer is not NULL before using it, we are safe to
remove the function.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/ecdh.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index 4271fc7..3aca093 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -131,17 +131,11 @@ static unsigned int ecdh_max_size(struct crypto_kpp *tfm)
return ctx->ndigits << (ECC_DIGITS_TO_BYTES_SHIFT + 1);
 }
 
-static void no_exit_tfm(struct crypto_kpp *tfm)
-{
-   return;
-}
-
 static struct kpp_alg ecdh = {
.set_secret = ecdh_set_secret,
.generate_public_key = ecdh_compute_value,
.compute_shared_secret = ecdh_compute_value,
.max_size = ecdh_max_size,
-   .exit = no_exit_tfm,
.base = {
.cra_name = "ecdh",
.cra_driver_name = "ecdh-generic",
-- 
2.9.4

Re: [PATCH] crypto: ccm - preserve the IV buffer

[Tudor Ambarus]

On 10/31/2017 04:42 PM, Romain Izard wrote:

The IV buffer used during CCM operations is used twice, during both the
hashing step and the ciphering step.

When using a hardware accelerator that updates the contents of the IV
buffer at the end of ciphering operations, the value will be modified.
In the decryption case, the subsequent setup of the hashing algorithm
will interpret the updated IV instead of the original value, which can
lead to out-of-bounds writes.

Reuse the idata buffer, only used in the hashing step, to preserve the
IV's value during the ciphering step in the decryption case.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
  crypto/ccm.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/ccm.c b/crypto/ccm.c
index 1ce37ae0ce56..0a083342ec8c 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -363,7 +363,7 @@ static int crypto_ccm_decrypt(struct aead_request *req)
unsigned int cryptlen = req->cryptlen;
u8 *authtag = pctx->auth_tag;
u8 *odata = pctx->odata;
-   u8 *iv = req->iv;
+   u8 *iv = pctx->idata;
int err;
  
  	cryptlen -= authsize;

@@ -379,6 +379,8 @@ static int crypto_ccm_decrypt(struct aead_request *req)
if (req->src != req->dst)
dst = pctx->dst;
  
+	memcpy(iv, req->iv, 16);

+
skcipher_request_set_tfm(skreq, ctx->ctr);
skcipher_request_set_callback(skreq, pctx->flags,
  crypto_ccm_decrypt_done, req);



Reviewed-by: Tudor Ambarus <tudor.amba...@microchip.com>

Re: [PATCH 2/4] crypto: dh - don't permit 'p' to be 0

[Tudor Ambarus]

Hi, Eric,

On 11/02/2017 12:25 AM, Eric Biggers wrote:

If 'p' is 0 for the software Diffie-Hellman implementation, then
dh_max_size() returns 0.


dh_set_secret() returns -EINVAL if p_len < 1536, see
dh_check_params_length(). What am I missing?

Cheers,
ta

Re: [PATCH 1/4] crypto: dh - fix double free of ctx->p

[Tudor Ambarus]

Hi, Eric,

On 11/02/2017 12:25 AM, Eric Biggers wrote:

When setting the secret with the software Diffie-Hellman implementation,
if allocating 'g' failed (e.g. if it was longer than
MAX_EXTERN_MPI_BITS), then 'p' was freed twice: once immediately, and
once later when the crypto_kpp tfm was destroyed.  Fix it by using
dh_free_ctx() in the error paths, as that sets the pointers to NULL.


Other solution would be to have just an one-line patch that sets p to
NULL after freeing it. The benefit of just setting p to NULL and not
using dh_free_ctx() is that you'll spare some cpu cycles. If g fails,
g and a will already be NULL, so freeing them and set them to NULL is
redundant.

However, if you decide to always use dh_free_ctx(), you'll have to get
rid of dh_clear_params(), because it will be used just in dh_free_ctx().
You can copy dh_clear_params() body to dh_free_ctx().

Cheers,
ta

[v2 PATCH 2/3] crypto: atmel-aes/tdes - remove empty functions

[Tudor Ambarus]

This empty functions were used to initialize a member of a static structure.
Pointer members of an object with static storage duration, if not
explicitly initialized, will be initialized to a NULL pointer. The crypto
API checks if this pointer is not NULL before using it, so we are safe to
remove this empty functions along with all the references to them.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
changes in v2:
- remove empty atmel_aes_gcm_exit()

 drivers/crypto/atmel-aes.c  | 20 
 drivers/crypto/atmel-tdes.c | 18 --
 2 files changed, 38 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 9659759..c2f0a12 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -1237,10 +1237,6 @@ static int atmel_aes_ctr_cra_init(struct crypto_tfm *tfm)
return 0;
 }
 
-static void atmel_aes_cra_exit(struct crypto_tfm *tfm)
-{
-}
-
 static struct crypto_alg aes_algs[] = {
 {
.cra_name   = "ecb(aes)",
@@ -1253,7 +1249,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1273,7 +1268,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1294,7 +1288,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1315,7 +1308,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1336,7 +1328,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1357,7 +1348,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1378,7 +1368,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1399,7 +1388,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_ctr_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1422,7 +1410,6 @@ static struct crypto_alg aes_cfb64_alg = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1809,18 +1796,12 @@ static int atmel_aes_gcm_init(struct crypto_aead *tfm)
return 0;
 }
 
-static void atmel_aes_gcm_exit(struct crypto_aead *tfm)
-{
-
-}
-
 static struct aead_alg aes_gcm_alg = {
.setkey = atmel_aes_gcm_setkey,
.setauthsize= atmel_aes

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Tudor Ambarus]

Hi, Romain,

On 10/18/2017 04:32 PM, Romain Izard wrote:

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 29e20c37f3a6..f3eabe1f1490 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -80,6 +80,7 @@
  #define AES_FLAGS_BUSY BIT(3)
  #define AES_FLAGS_DUMP_REG BIT(4)
  #define AES_FLAGS_OWN_SHA  BIT(5)
+#define AES_FLAGS_CIPHERTAIL   BIT(6)


not really needed, see below.



  #define AES_FLAGS_PERSISTENT   (AES_FLAGS_INIT | AES_FLAGS_BUSY)

@@ -156,6 +157,7 @@ struct atmel_aes_authenc_ctx {

  struct atmel_aes_reqctx {
 unsigned long   mode;
+   u32 ciphertail[AES_BLOCK_SIZE / sizeof(u32)];

How about renaming this variable to "lastc"? Ci, i=1..n is the usual
notation for the ciphertext blocks.

The assumption that the last ciphertext block is of AES_BLOCK_SIZE size
is correct, this driver is meant just for AES.


  };

  #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
@@ -496,6 +498,11 @@ static void atmel_aes_authenc_complete(struct
atmel_aes_dev *dd, int err);

  static inline int atmel_aes_complete(struct atmel_aes_dev *dd, int err)
  {
+   struct ablkcipher_request *req = ablkcipher_request_cast(dd->areq);
+   struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
+   struct atmel_aes_reqctx *rctx = ablkcipher_request_ctx(req);
+   int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+
  #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
 atmel_aes_authenc_complete(dd, err);
  #endif
@@ -503,6 +510,17 @@ static inline int atmel_aes_complete(struct
atmel_aes_dev *dd, int err)
 clk_disable(dd->iclk);
 dd->flags &= ~AES_FLAGS_BUSY;

+   if (rctx->mode & AES_FLAGS_CIPHERTAIL) {
+   if (rctx->mode & AES_FLAGS_ENCRYPT) {
+   scatterwalk_map_and_copy(req->info, req->dst,
+req->nbytes - ivsize,
+ivsize, 0);
+   } else {
+   memcpy(req->info, rctx->ciphertail, ivsize);


why don't we make the same assumption and replace ivsize with
AES_BLOCK_SIZE? Is there any chance that req->info was allocated with
less than AES_BLOCK_SIZE size?


+   memset(rctx->ciphertail, 0, ivsize);


memset to zero is not necessary.


+   }
+   }
+
 if (dd->is_async)
 dd->areq->complete(dd->areq, err);

@@ -1071,11 +1089,13 @@ static int atmel_aes_ctr_start(struct atmel_aes_dev *dd)

  static int atmel_aes_crypt(struct ablkcipher_request *req, unsigned long mode)
  {
+   struct crypto_ablkcipher *ablkcipher;
 struct atmel_aes_base_ctx *ctx;
 struct atmel_aes_reqctx *rctx;
 struct atmel_aes_dev *dd;

-   ctx = crypto_ablkcipher_ctx(crypto_ablkcipher_reqtfm(req));
+   ablkcipher = crypto_ablkcipher_reqtfm(req);
+   ctx = crypto_ablkcipher_ctx(ablkcipher);


you can initialize these variables at declaration.


 switch (mode & AES_FLAGS_OPMODE_MASK) {
 case AES_FLAGS_CFB8:
 ctx->block_size = CFB8_BLOCK_SIZE;
@@ -1103,7 +1123,15 @@ static int atmel_aes_crypt(struct
ablkcipher_request *req, unsigned long mode)
 return -ENODEV;

 rctx = ablkcipher_request_ctx(req);


while here, please initialize this at declaration.

Also, this one:
'''
dd = atmel_aes_find_dev(ctx);
if (!dd)
return -ENODEV;

'''
should be moved at the beginning of the function. If an initialization
might fail, let's check it as soon as we can, so that we don't waste
resources.


-   rctx->mode = mode;
+
+   if (!(mode & AES_FLAGS_ENCRYPT)) {
+   int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+
+   scatterwalk_map_and_copy(rctx->ciphertail, req->src,
+   (req->nbytes - ivsize), ivsize, 0);
+   }
+   rctx->mode = mode | AES_FLAGS_CIPHERTAIL;


saving the last ciphertext block is a requirement for skcipher. This
flag is redundant, there's no need to use it.

Thank you, Romain!
ta

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Tudor Ambarus]

Hi, Romain,

On 10/18/2017 04:32 PM, Romain Izard wrote:

diff --git a/crypto/ccm.c b/crypto/ccm.c
index 1ce37ae0ce56..e7c2121a3ab2 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -47,6 +47,7 @@ struct crypto_ccm_req_priv_ctx {
 u8 odata[16];
 u8 idata[16];
 u8 auth_tag[16];
+   u8 iv[16];
 u32 flags;
 struct scatterlist src[3];
 struct scatterlist dst[3];
@@ -248,32 +249,22 @@ static void crypto_ccm_encrypt_done(struct
crypto_async_request *areq, int err)
 aead_request_complete(req, err);
  }

-static inline int crypto_ccm_check_iv(const u8 *iv)
-{
-   /* 2 <= L <= 8, so 1 <= L' <= 7. */
-   if (1 > iv[0] || iv[0] > 7)
-   return -EINVAL;
-
-   return 0;
-}
-
-static int crypto_ccm_init_crypt(struct aead_request *req, u8 *tag)
+static int crypto_ccm_init_crypt(struct aead_request *req, u8 *tag, u8* iv)
  {
 struct crypto_ccm_req_priv_ctx *pctx = crypto_ccm_reqctx(req);
 struct scatterlist *sg;
-   u8 *iv = req->iv;
-   int err;
+   u8 L = req->iv[0] + 1;

-   err = crypto_ccm_check_iv(iv);
-   if (err)
-   return err;
-
-   pctx->flags = aead_request_flags(req);
+   if (2 > L || L > 8)
+   return -EINVAL;

  /* Note: rfc 3610 and NIST 800-38C require counter of
  * zero to encrypt auth tag.
  */
-   memset(iv + 15 - iv[0], 0, iv[0] + 1);
+   memcpy(iv, req->iv, 16 - L);
+   memset(iv + 16 - L, 0, L);
+
+   pctx->flags = aead_request_flags(req);

 sg_init_table(pctx->src, 3);
 sg_set_buf(pctx->src, tag, 16);
@@ -301,10 +292,10 @@ static int crypto_ccm_encrypt(struct aead_request *req)
 struct scatterlist *dst;
 unsigned int cryptlen = req->cryptlen;
 u8 *odata = pctx->odata;
-   u8 *iv = req->iv;
+   u8 *iv = pctx->iv;
 int err;

-   err = crypto_ccm_init_crypt(req, odata);
+   err = crypto_ccm_init_crypt(req, odata, iv);
 if (err)
 return err;

@@ -363,12 +354,12 @@ static int crypto_ccm_decrypt(struct aead_request *req)
 unsigned int cryptlen = req->cryptlen;
 u8 *authtag = pctx->auth_tag;
 u8 *odata = pctx->odata;
-   u8 *iv = req->iv;
+   u8 *iv = pctx->iv;
 int err;

 cryptlen -= authsize;

-   err = crypto_ccm_init_crypt(req, authtag);
+   err = crypto_ccm_init_crypt(req, authtag, iv);
 if (err)
 return err;


Looks good. Can you please submit with a commit message?

Thanks,
ta

[PATCH 3/3] crypto: atmel-aes/tdes/sha - remove useless irq init

[Tudor Ambarus]

irq would be set to -1 and then unused, if we failed to get IORESOURCE_MEM.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/atmel-aes.c  | 2 --
 drivers/crypto/atmel-sha.c  | 2 --
 drivers/crypto/atmel-tdes.c | 2 --
 3 files changed, 6 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index bc859ab..2048292 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -2644,8 +2644,6 @@ static int atmel_aes_probe(struct platform_device *pdev)
 
crypto_init_queue(_dd->queue, ATMEL_AES_QUEUE_LENGTH);
 
-   aes_dd->irq = -1;
-
/* Get the base address */
aes_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!aes_res) {
diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index 42c1f74..8874aa5 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -2777,8 +2777,6 @@ static int atmel_sha_probe(struct platform_device *pdev)
 
crypto_init_queue(_dd->queue, ATMEL_SHA_QUEUE_LENGTH);
 
-   sha_dd->irq = -1;
-
/* Get the base address */
sha_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!sha_res) {
diff --git a/drivers/crypto/atmel-tdes.c b/drivers/crypto/atmel-tdes.c
index 48e8653..592124f 100644
--- a/drivers/crypto/atmel-tdes.c
+++ b/drivers/crypto/atmel-tdes.c
@@ -1363,8 +1363,6 @@ static int atmel_tdes_probe(struct platform_device *pdev)
 
crypto_init_queue(_dd->queue, ATMEL_TDES_QUEUE_LENGTH);
 
-   tdes_dd->irq = -1;
-
/* Get the base address */
tdes_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!tdes_res) {
-- 
2.9.4

[PATCH 1/3] crypto: atmel-aes/tdes/sha - return appropriate error code

[Tudor Ambarus]

Return -ENODEV when dma_request_slave_channel_compat() fails.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/atmel-aes.c  | 3 +--
 drivers/crypto/atmel-sha.c  | 3 +--
 drivers/crypto/atmel-tdes.c | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 903fd43..9659759 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -2383,7 +2383,6 @@ static int atmel_aes_dma_init(struct atmel_aes_dev *dd,
  struct crypto_platform_data *pdata)
 {
struct at_dma_slave *slave;
-   int err = -ENOMEM;
dma_cap_mask_t mask;
 
dma_cap_zero(mask);
@@ -2408,7 +2407,7 @@ static int atmel_aes_dma_init(struct atmel_aes_dev *dd,
dma_release_channel(dd->src.chan);
 err_dma_in:
dev_warn(dd->dev, "no DMA channel available\n");
-   return err;
+   return -ENODEV;
 }
 
 static void atmel_aes_dma_cleanup(struct atmel_aes_dev *dd)
diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index 3e2f41b..42c1f74 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -2628,7 +2628,6 @@ static bool atmel_sha_filter(struct dma_chan *chan, void 
*slave)
 static int atmel_sha_dma_init(struct atmel_sha_dev *dd,
struct crypto_platform_data *pdata)
 {
-   int err = -ENOMEM;
dma_cap_mask_t mask_in;
 
/* Try to grab DMA channel */
@@ -2639,7 +2638,7 @@ static int atmel_sha_dma_init(struct atmel_sha_dev *dd,
atmel_sha_filter, >dma_slave->rxdata, dd->dev, 
"tx");
if (!dd->dma_lch_in.chan) {
dev_warn(dd->dev, "no DMA channel available\n");
-   return err;
+   return -ENODEV;
}
 
dd->dma_lch_in.dma_conf.direction = DMA_MEM_TO_DEV;
diff --git a/drivers/crypto/atmel-tdes.c b/drivers/crypto/atmel-tdes.c
index f4b335d..0ece4b8 100644
--- a/drivers/crypto/atmel-tdes.c
+++ b/drivers/crypto/atmel-tdes.c
@@ -720,7 +720,6 @@ static bool atmel_tdes_filter(struct dma_chan *chan, void 
*slave)
 static int atmel_tdes_dma_init(struct atmel_tdes_dev *dd,
struct crypto_platform_data *pdata)
 {
-   int err = -ENOMEM;
dma_cap_mask_t mask;
 
dma_cap_zero(mask);
@@ -765,7 +764,7 @@ static int atmel_tdes_dma_init(struct atmel_tdes_dev *dd,
dma_release_channel(dd->dma_lch_in.chan);
 err_dma_in:
dev_warn(dd->dev, "no DMA channel available\n");
-   return err;
+   return -ENODEV;
 }
 
 static void atmel_tdes_dma_cleanup(struct atmel_tdes_dev *dd)
-- 
2.9.4

[PATCH 2/3] crypto: atmel-aes/tdes - remove empty function

[Tudor Ambarus]

This empty function was used to initialize a member of a static structure.
Pointer members of an object with static storage duration, if not
explicitly initialized, will be initialized to a NULL pointer. The crypto
API checks if this pointer is not NULL before using it, so we are safe to
remove this empty function along with all the references to it.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 drivers/crypto/atmel-aes.c  | 14 --
 drivers/crypto/atmel-tdes.c | 18 --
 2 files changed, 32 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 9659759..bc859ab 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -1237,10 +1237,6 @@ static int atmel_aes_ctr_cra_init(struct crypto_tfm *tfm)
return 0;
 }
 
-static void atmel_aes_cra_exit(struct crypto_tfm *tfm)
-{
-}
-
 static struct crypto_alg aes_algs[] = {
 {
.cra_name   = "ecb(aes)",
@@ -1253,7 +1249,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1273,7 +1268,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1294,7 +1288,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1315,7 +1308,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1336,7 +1328,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1357,7 +1348,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1378,7 +1368,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1399,7 +1388,6 @@ static struct crypto_alg aes_algs[] = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_ctr_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1422,7 +1410,6 @@ static struct crypto_alg aes_cfb64_alg = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= AES_MIN_KEY_SIZE,
.max_keysize= AES_MAX_KEY_SIZE,
@@ -1956,7 +1943,6 @@ static struct crypto_alg aes_xts_alg = {
.cra_type   = _ablkcipher_type,
.cra_module = THIS_MODULE,
.cra_init   = atmel_aes_xts_cra_init,
-   .cra_exit   = atmel_aes_cra_exit,
.cra_u.ablkcipher = {
.min_keysize= 2 * 

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Tudor Ambarus]

Hi, Romain,

On 10/18/2017 04:32 PM, Romain Izard wrote:

my fix also led to a
systematic oops when running the ccm(aes) test case.


The NULL deference appears because of a memory corruption issue.

atmel-aes does not implement ccm(aes), so the algorithm will be in the
following form: ccm_base(atmel-ctr-aes,cbcmac(aes-generic)).

ccm auth uses the first byte of the IV as length and eventually will
memset memory to zero based on that length (see set_msg_len()). CTR
overwrites the iv with the last ciphertext block and the length will be
wrong.

I will propose a fix, but I'm taking my time to better understand why
CTR requires to overwrite the iv with the last ciphertext block.

Cheers,
ta

Re: [PATCH 1/3] crypto: dh_helper - return unsigned int for dh_data_size()

[Tudor Ambarus]

Hi, David,

On 10/03/2017 12:06 PM, David Howells wrote:

Tudor Ambarus <tudor.amba...@microchip.com> wrote:


-static inline int dh_data_size(const struct dh *p)
+static inline unsigned int dh_data_size(const struct dh *p)
  {
return p->key_size + p->p_size + p->g_size;
  }


If this is a problem, do you need to do range checking?


The algorithm does not impose any constraint in this direction, as far
as I'm aware of.

It's unnatural to return a signed integer in a function which just sums
unsigned integers. No checking is needed, the function should return the
unsigned result.

Cheers,
ta

[PATCH 2/3] crypto: dh_helper - return unsigned value for crypto_dh_key_len()

[Tudor Ambarus]

DH_KPP_SECRET_MIN_SIZE and dh_data_size() are both returning
unsigned values.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/dh_helper.c  | 2 +-
 include/crypto/dh.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c
index 69869da..a413b31 100644
--- a/crypto/dh_helper.c
+++ b/crypto/dh_helper.c
@@ -33,7 +33,7 @@ static inline unsigned int dh_data_size(const struct dh *p)
return p->key_size + p->p_size + p->g_size;
 }
 
-int crypto_dh_key_len(const struct dh *p)
+unsigned int crypto_dh_key_len(const struct dh *p)
 {
return DH_KPP_SECRET_MIN_SIZE + dh_data_size(p);
 }
diff --git a/include/crypto/dh.h b/include/crypto/dh.h
index f638998..71e1bb2 100644
--- a/include/crypto/dh.h
+++ b/include/crypto/dh.h
@@ -53,7 +53,7 @@ struct dh {
  *
  * Return: size of the key in bytes
  */
-int crypto_dh_key_len(const struct dh *params);
+unsigned int crypto_dh_key_len(const struct dh *params);
 
 /**
  * crypto_dh_encode_key() - encode the private key
-- 
2.9.4

[PATCH 3/3] KEYS: dh - make some length variables unsigned

[Tudor Ambarus]

Both crypto_kpp_maxsize() and crypto_dh_key_len() are returning
unsigned integers.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 security/keys/dh.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/keys/dh.c b/security/keys/dh.c
index d1ea9f3..89e9255 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -242,8 +242,8 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user 
*params,
 {
long ret;
ssize_t dlen;
-   int secretlen;
-   int outlen;
+   unsigned int secretlen;
+   unsigned int outlen;
struct keyctl_dh_params pcopy;
struct dh dh_inputs;
struct scatterlist outsg;
-- 
2.9.4

[PATCH] crypto: ecdh_helper - return unsigned value for crypto_ecdh_key_len()

[Tudor Ambarus]

ECDH_KPP_SECRET_MIN_SIZE and params->key_size are both returning
unsigned values.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 crypto/ecdh_helper.c  | 2 +-
 include/crypto/ecdh.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/ecdh_helper.c b/crypto/ecdh_helper.c
index f05bea5..d3af8e8 100644
--- a/crypto/ecdh_helper.c
+++ b/crypto/ecdh_helper.c
@@ -28,7 +28,7 @@ static inline const u8 *ecdh_unpack_data(void *dst, const 
void *src, size_t sz)
return src + sz;
 }
 
-int crypto_ecdh_key_len(const struct ecdh *params)
+unsigned int crypto_ecdh_key_len(const struct ecdh *params)
 {
return ECDH_KPP_SECRET_MIN_SIZE + params->key_size;
 }
diff --git a/include/crypto/ecdh.h b/include/crypto/ecdh.h
index 1aff2a8..d696317 100644
--- a/include/crypto/ecdh.h
+++ b/include/crypto/ecdh.h
@@ -54,7 +54,7 @@ struct ecdh {
  *
  * Return: size of the key in bytes
  */
-int crypto_ecdh_key_len(const struct ecdh *params);
+unsigned int crypto_ecdh_key_len(const struct ecdh *params);
 
 /**
  * crypto_ecdh_encode_key() - encode the private key
-- 
2.9.4

Re: [v2 PATCH 5/5] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

Hi, Marcel,

On 09/28/2017 07:50 PM, Marcel Holtmann wrote:

Hi Tudor,


That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch, the Bluetooth SMP will stop keeping a copy of the
ecdh private key and will let the crypto subsystem to generate and
handle the ecdh private key, potentially benefiting of hardware
ecc private key generation and retention.

The loop that tries to generate a correct private key is now removed and
we trust the crypto subsystem to generate a correct private key. This
backup logic should be done in crypto, if really needed.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
net/bluetooth/ecdh_helper.c | 186 
net/bluetooth/ecdh_helper.h |   9 ++-
net/bluetooth/selftest.c|  14 +++-
net/bluetooth/smp.c |  66 +++-
4 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index 16e022f..2155ce8 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -49,15 +49,21 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned 
int ndigits)
out[i] = __swab64(in[ndigits - 1 - i]);
}

+/* compute_ecdh_secret() - function assumes that the private key was
+ * already set.
+ * @tfm:  KPP tfm handle allocated with crypto_alloc_kpp().
+ * @public_key:   pair's ecc public key.
+ * secret:memory where the ecdh computed shared secret will be saved.
+ *
+ * Return: zero on success; error code in case of error.
+ */
int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
-   const u8 private_key[32], u8 secret[32])
+   u8 secret[32])
{
struct kpp_request *req;
-   struct ecdh p;
+   u8 *tmp;
struct ecdh_completion result;
struct scatterlist src, dst;
-   u8 *tmp, *buf;
-   unsigned int buf_len;
int err;

tmp = kmalloc(64, GFP_KERNEL);
@@ -72,28 +78,6 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],

init_completion();

-   /* Security Manager Protocol holds digits in litte-endian order
-* while ECC API expect big-endian data
-*/
-   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
-   p.key = (char *)tmp;
-   p.key_size = 32;
-   /* Set curve_id */
-   p.curve_id = ECC_CURVE_NIST_P256;
-   buf_len = crypto_ecdh_key_len();
-   buf = kmalloc(buf_len, GFP_KERNEL);
-   if (!buf) {
-   err = -ENOMEM;
-   goto free_req;
-   }
-
-   crypto_ecdh_encode_key(buf, buf_len, );
-
-   /* Set A private Key */
-   err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
-   if (err)
-   goto free_all;
-
swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */

@@ -118,26 +102,76 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
memcpy(secret, tmp, 32);

free_all:
-   kzfree(buf);
-free_req:
kpp_request_free(req);
free_tmp:
kzfree(tmp);
return err;
}

-int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
-  u8 private_key[32])
+/* set_ecdh_privkey() - set or generate ecc private key.
+ *
+ * Function generates an ecc private key in the crypto subsystem when receiving
+ * a NULL private key or sets the received key when not NULL.
+ *
+ * @tfm:   KPP tfm handle allocated with crypto_alloc_kpp().
+ * @private_key:   user's ecc private key. When not NULL, the key is expected
+ * in little endian format.
+ *
+ * Return: zero on success; error code in case of error.
+ */
+int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32])
+{
+   u8 *buf, *tmp = NULL;
+   unsigned int buf_len;
+   int err;
+   struct ecdh p = {0};
+
+   p.curve_id = ECC_CURVE_NIST_P256;
+
+   if (private_key) {
+   tmp = kmalloc(32, GFP_KERNEL);
+   if (!tmp)
+   return -ENOMEM;
+   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
+   p.key = tmp;
+   p.key_size = 32;
+   }
+
+   buf_len = crypto_ecdh_key_len();
+   buf = kmalloc(buf_len, GFP_KERNEL);
+   if (!buf) {
+   err = -ENOMEM;
+   goto free_tmp;
+   }
+
+   err = crypto_ecdh_encode_key(buf, buf_len, );
+   if (err)
+   goto free_all;
+
+   err = crypto_kpp_set_secret(tfm, buf, buf_len);
+   /* fall through */
+free_all:
+   kzfree(buf);
+free_tmp:
+   kzfree(tmp);
+   return err;
+}
+
+/* generate_ecdh_public_key() - function assumes that the private key was
+ *  already set.
+ *
+ * @tfm:  KPP tfm 

[v2 PATCH 5/5] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch, the Bluetooth SMP will stop keeping a copy of the
ecdh private key and will let the crypto subsystem to generate and
handle the ecdh private key, potentially benefiting of hardware
ecc private key generation and retention.

The loop that tries to generate a correct private key is now removed and
we trust the crypto subsystem to generate a correct private key. This
backup logic should be done in crypto, if really needed.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 net/bluetooth/ecdh_helper.c | 186 
 net/bluetooth/ecdh_helper.h |   9 ++-
 net/bluetooth/selftest.c|  14 +++-
 net/bluetooth/smp.c |  66 +++-
 4 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index 16e022f..2155ce8 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -49,15 +49,21 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned 
int ndigits)
out[i] = __swab64(in[ndigits - 1 - i]);
 }
 
+/* compute_ecdh_secret() - function assumes that the private key was
+ * already set.
+ * @tfm:  KPP tfm handle allocated with crypto_alloc_kpp().
+ * @public_key:   pair's ecc public key.
+ * secret:memory where the ecdh computed shared secret will be saved.
+ *
+ * Return: zero on success; error code in case of error.
+ */
 int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
-   const u8 private_key[32], u8 secret[32])
+   u8 secret[32])
 {
struct kpp_request *req;
-   struct ecdh p;
+   u8 *tmp;
struct ecdh_completion result;
struct scatterlist src, dst;
-   u8 *tmp, *buf;
-   unsigned int buf_len;
int err;
 
tmp = kmalloc(64, GFP_KERNEL);
@@ -72,28 +78,6 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
 
init_completion();
 
-   /* Security Manager Protocol holds digits in litte-endian order
-* while ECC API expect big-endian data
-*/
-   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
-   p.key = (char *)tmp;
-   p.key_size = 32;
-   /* Set curve_id */
-   p.curve_id = ECC_CURVE_NIST_P256;
-   buf_len = crypto_ecdh_key_len();
-   buf = kmalloc(buf_len, GFP_KERNEL);
-   if (!buf) {
-   err = -ENOMEM;
-   goto free_req;
-   }
-
-   crypto_ecdh_encode_key(buf, buf_len, );
-
-   /* Set A private Key */
-   err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
-   if (err)
-   goto free_all;
-
swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */
 
@@ -118,26 +102,76 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
memcpy(secret, tmp, 32);
 
 free_all:
-   kzfree(buf);
-free_req:
kpp_request_free(req);
 free_tmp:
kzfree(tmp);
return err;
 }
 
-int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
-  u8 private_key[32])
+/* set_ecdh_privkey() - set or generate ecc private key.
+ *
+ * Function generates an ecc private key in the crypto subsystem when receiving
+ * a NULL private key or sets the received key when not NULL.
+ *
+ * @tfm:   KPP tfm handle allocated with crypto_alloc_kpp().
+ * @private_key:   user's ecc private key. When not NULL, the key is expected
+ * in little endian format.
+ *
+ * Return: zero on success; error code in case of error.
+ */
+int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32])
+{
+   u8 *buf, *tmp = NULL;
+   unsigned int buf_len;
+   int err;
+   struct ecdh p = {0};
+
+   p.curve_id = ECC_CURVE_NIST_P256;
+
+   if (private_key) {
+   tmp = kmalloc(32, GFP_KERNEL);
+   if (!tmp)
+   return -ENOMEM;
+   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
+   p.key = tmp;
+   p.key_size = 32;
+   }
+
+   buf_len = crypto_ecdh_key_len();
+   buf = kmalloc(buf_len, GFP_KERNEL);
+   if (!buf) {
+   err = -ENOMEM;
+   goto free_tmp;
+   }
+
+   err = crypto_ecdh_encode_key(buf, buf_len, );
+   if (err)
+   goto free_all;
+
+   err = crypto_kpp_set_secret(tfm, buf, buf_len);
+   /* fall through */
+free_all:
+   kzfree(buf);
+free_tmp:
+   kzfree(tmp);
+   return err;
+}
+
+/* generate_ecdh_public_key() - function assumes that the private key was
+ *  already set.
+ *
+ * @tfm:  KPP tfm handle allocated with crypto_alloc_kpp().
+ * @publ

[v2 PATCH 3/5] Bluetooth: selftest - check for errors when computing ZZ

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 net/bluetooth/selftest.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
index 126bdc5..ce99648 100644
--- a/net/bluetooth/selftest.c
+++ b/net/bluetooth/selftest.c
@@ -143,7 +143,7 @@ static int __init test_ecdh_sample(struct crypto_kpp *tfm, 
const u8 priv_a[32],
   const u8 pub_b[64], const u8 dhkey[32])
 {
u8 *tmp, *dhkey_a, *dhkey_b;
-   int ret = 0;
+   int ret;
 
tmp = kmalloc(64, GFP_KERNEL);
if (!tmp)
@@ -152,8 +152,13 @@ static int __init test_ecdh_sample(struct crypto_kpp *tfm, 
const u8 priv_a[32],
dhkey_a = [0];
dhkey_b = [32];
 
-   compute_ecdh_secret(tfm, pub_b, priv_a, dhkey_a);
-   compute_ecdh_secret(tfm, pub_a, priv_b, dhkey_b);
+   ret = compute_ecdh_secret(tfm, pub_b, priv_a, dhkey_a);
+   if (ret)
+   goto out;
+
+   ret = compute_ecdh_secret(tfm, pub_a, priv_b, dhkey_b);
+   if (ret)
+   goto out;
 
if (memcmp(dhkey_a, dhkey, 32)) {
ret = -EINVAL;
-- 
2.9.4

[v2 PATCH 4/5] Bluetooth: ecdh_helper - fix leak of private key

[Tudor Ambarus]

tmp buffer contains the swapped private key. In case the setkey call
failed, the tmp buffer was freed without clearing the private key.

Zeroize the temporary buffer so we don't leak the private key.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 net/bluetooth/ecdh_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index 22c8daa..16e022f 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -122,7 +122,7 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
 free_req:
kpp_request_free(req);
 free_tmp:
-   kfree(tmp);
+   kzfree(tmp);
return err;
 }
 
-- 
2.9.4

[v2 PATCH 2/5] Bluetooth: ecdh_helper - reveal error codes

[Tudor Ambarus]

ecdh_helper functions were hiding the error codes and chose to return
the return value of an relational operator, "==". Remove the unnecessary
query and reveal the error codes.

While updating the return values, code in a way that compilers will
warn in case of uninitialized err.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 net/bluetooth/ecdh_helper.c | 32 +++-
 net/bluetooth/ecdh_helper.h |  8 
 net/bluetooth/smp.c | 17 ++---
 3 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index ac2c708..22c8daa 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -49,8 +49,8 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned 
int ndigits)
out[i] = __swab64(in[ndigits - 1 - i]);
 }
 
-bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
-const u8 private_key[32], u8 secret[32])
+int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
+   const u8 private_key[32], u8 secret[32])
 {
struct kpp_request *req;
struct ecdh p;
@@ -58,15 +58,17 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
struct scatterlist src, dst;
u8 *tmp, *buf;
unsigned int buf_len;
-   int err = -ENOMEM;
+   int err;
 
tmp = kmalloc(64, GFP_KERNEL);
if (!tmp)
-   return false;
+   return -ENOMEM;
 
req = kpp_request_alloc(tfm, GFP_KERNEL);
-   if (!req)
+   if (!req) {
+   err = -ENOMEM;
goto free_tmp;
+   }
 
init_completion();
 
@@ -80,8 +82,10 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
p.curve_id = ECC_CURVE_NIST_P256;
buf_len = crypto_ecdh_key_len();
buf = kmalloc(buf_len, GFP_KERNEL);
-   if (!buf)
+   if (!buf) {
+   err = -ENOMEM;
goto free_req;
+   }
 
crypto_ecdh_encode_key(buf, buf_len, );
 
@@ -119,11 +123,11 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
kpp_request_free(req);
 free_tmp:
kfree(tmp);
-   return (err == 0);
+   return err;
 }
 
-bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
-   u8 private_key[32])
+int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
+  u8 private_key[32])
 {
struct kpp_request *req;
struct ecdh p;
@@ -131,17 +135,19 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
public_key[64],
struct scatterlist dst;
u8 *tmp, *buf;
unsigned int buf_len;
-   int err = -ENOMEM;
+   int err;
const unsigned short max_tries = 16;
unsigned short tries = 0;
 
tmp = kmalloc(64, GFP_KERNEL);
if (!tmp)
-   return false;
+   return -ENOMEM;
 
req = kpp_request_alloc(tfm, GFP_KERNEL);
-   if (!req)
+   if (!req) {
+   err = -ENOMEM;
goto free_tmp;
+   }
 
init_completion();
 
@@ -202,5 +208,5 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
public_key[64],
kpp_request_free(req);
 free_tmp:
kfree(tmp);
-   return (err == 0);
+   return err;
 }
diff --git a/net/bluetooth/ecdh_helper.h b/net/bluetooth/ecdh_helper.h
index 5cde37d..50e6676 100644
--- a/net/bluetooth/ecdh_helper.h
+++ b/net/bluetooth/ecdh_helper.h
@@ -23,7 +23,7 @@
 #include 
 #include 
 
-bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pub_a[64],
-const u8 priv_b[32], u8 secret[32]);
-bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
-   u8 private_key[32]);
+int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pub_a[64],
+   const u8 priv_b[32], u8 secret[32]);
+int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
+  u8 private_key[32]);
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 31b64bc..af7e610 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -577,9 +577,10 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 
rand[16])
get_random_bytes(smp->local_sk, 32);
 
/* Generate local key pair for Secure Connections */
-   if (!generate_ecdh_keys(smp->tfm_ecdh, smp->local_pk,
-   smp->local_sk))
-   return -EIO;
+   err = generate_ecdh_keys(smp->tfm_ecdh, smp->local_pk,
+smp->local_sk);
+   if (err)
+   return err;
 
/* T

[v2 PATCH 1/5] Bluetooth: move ecdh allocation outside of ecdh_helper

[Tudor Ambarus]

Before this change, a new crypto tfm was allocated, each time,
for both key generation and shared secret computation.

Allocate a single tfm for both cases.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 net/bluetooth/ecdh_helper.c | 32 -
 net/bluetooth/ecdh_helper.h |  8 --
 net/bluetooth/selftest.c| 29 +--
 net/bluetooth/smp.c | 68 -
 4 files changed, 87 insertions(+), 50 deletions(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index c7b1a9a..ac2c708 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -23,7 +23,6 @@
 #include "ecdh_helper.h"
 
 #include 
-#include 
 #include 
 
 struct ecdh_completion {
@@ -50,10 +49,9 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned 
int ndigits)
out[i] = __swab64(in[ndigits - 1 - i]);
 }
 
-bool compute_ecdh_secret(const u8 public_key[64], const u8 private_key[32],
-u8 secret[32])
+bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
+const u8 private_key[32], u8 secret[32])
 {
-   struct crypto_kpp *tfm;
struct kpp_request *req;
struct ecdh p;
struct ecdh_completion result;
@@ -66,16 +64,9 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8 
private_key[32],
if (!tmp)
return false;
 
-   tfm = crypto_alloc_kpp("ecdh", CRYPTO_ALG_INTERNAL, 0);
-   if (IS_ERR(tfm)) {
-   pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
-  PTR_ERR(tfm));
-   goto free_tmp;
-   }
-
req = kpp_request_alloc(tfm, GFP_KERNEL);
if (!req)
-   goto free_kpp;
+   goto free_tmp;
 
init_completion();
 
@@ -126,16 +117,14 @@ bool compute_ecdh_secret(const u8 public_key[64], const 
u8 private_key[32],
kzfree(buf);
 free_req:
kpp_request_free(req);
-free_kpp:
-   crypto_free_kpp(tfm);
 free_tmp:
kfree(tmp);
return (err == 0);
 }
 
-bool generate_ecdh_keys(u8 public_key[64], u8 private_key[32])
+bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
+   u8 private_key[32])
 {
-   struct crypto_kpp *tfm;
struct kpp_request *req;
struct ecdh p;
struct ecdh_completion result;
@@ -150,16 +139,9 @@ bool generate_ecdh_keys(u8 public_key[64], u8 
private_key[32])
if (!tmp)
return false;
 
-   tfm = crypto_alloc_kpp("ecdh", CRYPTO_ALG_INTERNAL, 0);
-   if (IS_ERR(tfm)) {
-   pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
-  PTR_ERR(tfm));
-   goto free_tmp;
-   }
-
req = kpp_request_alloc(tfm, GFP_KERNEL);
if (!req)
-   goto free_kpp;
+   goto free_tmp;
 
init_completion();
 
@@ -218,8 +200,6 @@ bool generate_ecdh_keys(u8 public_key[64], u8 
private_key[32])
kzfree(buf);
 free_req:
kpp_request_free(req);
-free_kpp:
-   crypto_free_kpp(tfm);
 free_tmp:
kfree(tmp);
return (err == 0);
diff --git a/net/bluetooth/ecdh_helper.h b/net/bluetooth/ecdh_helper.h
index 7a423fa..5cde37d 100644
--- a/net/bluetooth/ecdh_helper.h
+++ b/net/bluetooth/ecdh_helper.h
@@ -20,8 +20,10 @@
  * COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
  * SOFTWARE IS DISCLAIMED.
  */
+#include 
 #include 
 
-bool compute_ecdh_secret(const u8 pub_a[64], const u8 priv_b[32],
-u8 secret[32]);
-bool generate_ecdh_keys(u8 public_key[64], u8 private_key[32]);
+bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pub_a[64],
+const u8 priv_b[32], u8 secret[32]);
+bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
+   u8 private_key[32]);
diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
index 34a1227..126bdc5 100644
--- a/net/bluetooth/selftest.c
+++ b/net/bluetooth/selftest.c
@@ -138,9 +138,9 @@ static const u8 dhkey_3[32] __initconst = {
0x7c, 0x1c, 0xf9, 0x49, 0xe6, 0xd7, 0xaa, 0x70,
 };
 
-static int __init test_ecdh_sample(const u8 priv_a[32], const u8 priv_b[32],
-  const u8 pub_a[64], const u8 pub_b[64],
-  const u8 dhkey[32])
+static int __init test_ecdh_sample(struct crypto_kpp *tfm, const u8 priv_a[32],
+  const u8 priv_b[32], const u8 pub_a[64],
+  const u8 pub_b[64], const u8 dhkey[32])
 {
u8 *tmp, *dhkey_a, *dhkey_b;
int ret = 0;
@@ -152,8 +152,8 @@ static int __init test_ecdh_sample(const u8 priv_a[32], 
const u8 priv_b[32],
dhkey_a = [0];
dhkey_b = [32];
 
-   compute_ecdh_secret(pub_b, priv_a, dhke

[v2 PATCH 0/5] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch set, the Bluetooth SMP will stop keeping a copy of the
ecdh private key. We let the crypto subsystem to generate and handle
the ecdh private key, potentially benefiting of hardware ecc private key
generation and retention.

Tested with selftest and with btmon and smp-tester on top of hci_vhci,
with ecdh done in both software and hardware (through atmel-ecc driver).
All tests passed.

RFC version can be found at:
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28036.html

Changes in v2:
- add patches 2, 3, 4.
- adress Marcel's suggestions:
  - revive the check for accidentally generated debug keys
  - bypass the handling of private key to the crypto subsytem,
even when using debug keys.


Tudor Ambarus (5):
  Bluetooth: move ecdh allocation outside of ecdh_helper
  Bluetooth: ecdh_helper - reveal error codes
  Bluetooth: selftest - check for errors when computing ZZ
  Bluetooth: ecdh_helper - fix leak of private key
  Bluetooth: let the crypto subsystem generate the ecc privkey

 net/bluetooth/ecdh_helper.c | 228 ++--
 net/bluetooth/ecdh_helper.h |   9 +-
 net/bluetooth/selftest.c|  46 +++--
 net/bluetooth/smp.c | 127 +++-
 4 files changed, 240 insertions(+), 170 deletions(-)

-- 
2.9.4

Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

Hi, Marcel,

Agreed on all suggestions, I will send a v2 patch set.

Thanks,
ta

Re: [RESEND RFC PATCH 1/2] Bluetooth: move ecdh allocation outside of ecdh_helper

[Tudor Ambarus]

Hi, Marcel,

On 09/25/2017 04:02 PM, Marcel Holtmann wrote:


diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index a0ef897..6532689 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c


[cut]


@@ -2677,7 +2695,16 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, 
struct sk_buff *skb)
SMP_DBG("Remote Public Key X: %32phN", smp->remote_pk);
SMP_DBG("Remote Public Key Y: %32phN", smp->remote_pk + 32);

-   if (!compute_ecdh_secret(smp->remote_pk, smp->local_sk, smp->dhkey))
+   if (test_bit(SMP_FLAG_LOCAL_OOB, >flags)) {
+   struct smp_dev *smp_dev = chan->data;
+
+   tfm = smp_dev->tfm_ecdh;
+   } else {
+   tfm = smp->tfm_ecdh;
+   }


this looks all good, but I do not understand the need of this extra if clause 
here. Can you explain why OOB case is different or maybe add some comment in 
the code to make this clear.


This particular change should have been in patch 2/2.
It's needed because I need to compute the shared secret on the
same tfm on which the private key was set/generated.

Thanks,
ta

[RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch, the Bluetooth SMP will stop keeping a copy of the
ecdh private key, except when using debug keys. This way we let the
crypto subsystem to generate and handle the ecdh private key,
potentially benefiting of hardware ecc private key generation and
retention.

The loop that tries to generate a correct private key is now removed and
we trust the crypto subsystem to generate a correct private key. This
backup logic should be done in crypto, if really needed.

Keeping the private key in the crypto subsystem implies that we can't
check if we accidentally generated a debug key. As this event is
unlikely we can live with it when comparing with the benefit of the
overall change: hardware private key generation and retention.

Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
---
 net/bluetooth/ecdh_helper.c | 102 +---
 net/bluetooth/smp.c |  55 +---
 2 files changed, 67 insertions(+), 90 deletions(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index ac2c708..56e9374 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -53,10 +53,10 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
 const u8 private_key[32], u8 secret[32])
 {
struct kpp_request *req;
-   struct ecdh p;
+   struct ecdh p = {0};
struct ecdh_completion result;
struct scatterlist src, dst;
-   u8 *tmp, *buf;
+   u8 *tmp, *buf = NULL;
unsigned int buf_len;
int err = -ENOMEM;
 
@@ -70,25 +70,29 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
 
init_completion();
 
-   /* Security Manager Protocol holds digits in litte-endian order
-* while ECC API expect big-endian data
-*/
-   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
-   p.key = (char *)tmp;
-   p.key_size = 32;
/* Set curve_id */
p.curve_id = ECC_CURVE_NIST_P256;
-   buf_len = crypto_ecdh_key_len();
-   buf = kmalloc(buf_len, GFP_KERNEL);
-   if (!buf)
-   goto free_req;
 
-   crypto_ecdh_encode_key(buf, buf_len, );
+   if (private_key) {
+   /* Security Manager Protocol holds digits in little-endian order
+* while ECC API expect big-endian data.
+*/
+   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
+   p.key = (char *)tmp;
+   p.key_size = 32;
 
-   /* Set A private Key */
-   err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
-   if (err)
-   goto free_all;
+   buf_len = crypto_ecdh_key_len();
+   buf = kmalloc(buf_len, GFP_KERNEL);
+   if (!buf)
+   goto free_req;
+
+   crypto_ecdh_encode_key(buf, buf_len, );
+
+   /* Set A private Key */
+   err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
+   if (err)
+   goto free_all;
+   }
 
swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */
@@ -126,14 +130,12 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
public_key[64],
u8 private_key[32])
 {
struct kpp_request *req;
-   struct ecdh p;
+   struct ecdh p = {0};
struct ecdh_completion result;
struct scatterlist dst;
u8 *tmp, *buf;
unsigned int buf_len;
int err = -ENOMEM;
-   const unsigned short max_tries = 16;
-   unsigned short tries = 0;
 
tmp = kmalloc(64, GFP_KERNEL);
if (!tmp)
@@ -145,56 +147,48 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
public_key[64],
 
init_completion();
 
+   /* Set private Key */
+   if (private_key) {
+   p.key = (char *)private_key;
+   p.key_size = 32;
+   }
+
/* Set curve_id */
p.curve_id = ECC_CURVE_NIST_P256;
-   p.key_size = 32;
buf_len = crypto_ecdh_key_len();
buf = kmalloc(buf_len, GFP_KERNEL);
if (!buf)
goto free_req;
 
-   do {
-   if (tries++ >= max_tries)
-   goto free_all;
-
-   /* Set private Key */
-   p.key = (char *)private_key;
-   crypto_ecdh_encode_key(buf, buf_len, );
-   err = crypto_kpp_set_secret(tfm, buf, buf_len);
-   if (err)
-   goto free_all;
-
-   sg_init_one(, tmp, 64);
-   kpp_request_set_input(req, NULL, 0);
-   kpp_request_set_output(req, , 64);
-   kpp_request_set_callback(req, CRYPTO_TFM_REQ_MA

[RESEND RFC PATCH 0/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch set, the Bluetooth SMP will stop keeping a copy of the
ecdh private key, except when using debug keys. This way we let the
crypto subsystem to generate and handle the ecdh private key,
potentially benefiting of hardware ecc private key generation and
retention.

Tested with selftest and with btmon and smp-tester on top of hci_vhci,
with ecdh done in both software and hardware (through atmel-ecc driver).
All tests passed.


Tudor Ambarus (2):
  Bluetooth: move ecdh allocation outside of ecdh_helper
  Bluetooth: let the crypto subsystem generate the ecc privkey

 net/bluetooth/ecdh_helper.c | 134 ++--
 net/bluetooth/ecdh_helper.h |   8 ++-
 net/bluetooth/selftest.c|  29 +++---
 net/bluetooth/smp.c | 120 +--
 4 files changed, 157 insertions(+), 134 deletions(-)

-- 
2.9.4

[RFC PATCH 0/2] let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch set, the Bluetooth SMP will stop keeping a copy of the
ecdh private key, except when using debug keys. This way we let the
crypto subsystem to generate and handle the ecdh private key,
potentially benefiting of hardware ecc private key generation and
retention.

Tested with selftest and with btmon and smp-tester on top of hci_vhci,
with ecdh done in both software and hardware (through atmel-ecc driver).
All tests passed.

Tudor Ambarus (2):
  Bluetooth: move ecdh allocation outside of ecdh_helper
  Bluetooth: let the crypto subsystem generate the ecc privkey

 net/bluetooth/ecdh_helper.c | 138 ++--
 net/bluetooth/ecdh_helper.h |   8 ++-
 net/bluetooth/selftest.c|  29 +++---
 net/bluetooth/smp.c | 120 --
 4 files changed, 159 insertions(+), 136 deletions(-)

-- 
2.9.4

Re: KPP questions and confusion

[Tudor Ambarus]

Hi, Marcel,

On 08/03/2017 11:40 AM, Marcel Holtmann wrote:

Essentially we do what all other key exchange procedure do. Generate a 
private/public key pair, give the public key to the other side, run DH with the 
value from the other side. That Bluetooth SMP knows about the private key is 
really pointless. Since the detection of debug key usage is actually via the 
public key portion.


I'm working on letting the bluetooth smp benefit of the ecc private key
generation from the crypto subsystem. I will send some patches soon.

Cheers,
ta

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Tudor Ambarus]

Hi, Marcel,

On 08/30/2017 10:21 AM, Marcel Holtmann wrote:

you still need to get the public key out of the kernel if you want to use it 
from user space. Or feed the remote public key if you plan to use some sort of 
key derivation function.



The crypto hardware that I'm working on, generates the private key
internally within the device and never reveals it to software and
immediately returns the public key pair. The user can retrieve the
public key from hardware.


I am saying this again, if you only have a hammer, everything looks like a 
nail. What about actually looking at how this would be used from user space in 
real crypto cases.

My point is that the usages here are key generation, some sort of 
key-exchange-agreement (aka DH) and key derivation into a symmetric key. 
Frankly the focus with asymmetric ciphers are the keys and the key derivation. 
They are not encryption and decryption of massive amounts of data.


The hardware uses it's own private key and the public key received from
the other end and computes the ecdh shared secret. The hardware computed
shared secret can then be used for key derivation.

Cheers,
ta

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Tudor Ambarus]

Hi, Herbert, all,

akcipher can work with its own internal keys, now that we have crypto
accelerators that can generate keys that never leave the hardware. Going
through the kernel's key subsystem seems superfluous in this case.

I also understand the need of going through the kernel's key subsystem
when the user wants to refer to a key which exists elsewhere, such as in
TPM or within an SGX software enclave, but this seems orthogonal with
crypto accelerators with key generation and retention support.

How should we interface akcipher/kpp with user-space?

Thanks,
ta

On 08/17/2017 04:17 PM, Tudor Ambarus wrote:

Hi, all,

On 08/11/2017 07:05 PM, Marcel Holtmann wrote:

Hi Stephan,

AF_ALG is best suited for crypto use cases where a socket is set up 
once

and there are lots of reads and writes to justify the setup cost. With
asymmetric crypto, the setup cost is high when you might only use the
socket for a brief time to do one verify or encrypt operation.


To me, the entire AF_ALG purpose is solely to export hardware support 
to user
space. That said, if user space wants an accelerator, a socket would 
be opened

once followed by numerous read/write requests.

Besides, I am aware of Tadeusz' patch to link algif_akcipher to the 
keyring
and I planned to port it to the current implementation. But I thought 
I offer

a small patch focusing on the externalization of the akcipher API first.

I think the keyctl and AF_ALG are no opponents, but rather are 
orthogonal to
each other. The statement I made for the KPP AF_ALG RFC applies here 
too:


"""
I am aware and in fact supported development of the DH support in the 
keys
subsystem. The question will be raised whether the AF_ALG KPP 
interface is
superfluous in light of the keys DH support. My answer is that AF_ALG 
KPP is

orthogonal to the keys DH support. The keys DH support use case is that
the keys are managed inside the kernel and thus has the focus on the
key management. Contrary, AF_ALG KPP does not really focus on key 
management
but simply externalizes the DH/ECDH support found in the kernel 
including
hardware acceleration. User space is in full control of the key life 
cycle.

This way, AF_ALG could be used to complement user-space network protocol
implementations like TLS or IKE to offload the resource intense DH
calculations to accelerators.
“""


we do not need two interfaces for doing the same thing. Especially not 
one that can not handle hardware backed keys. And more important if 
you can not abstract an accelerator that doesn’t expose the private 
key at all.


I'm working with a crypto accelerator (find it at [1]) that is capable
of generating random ecc private keys internally within the device that
are never revealed outside of it. The keys can be further used for ECDH
and ECDSA.

The simplest way to access my device from user-space is to go through
af_alg. We can permit the users to provide NULL keys, and if so, we can
generate the keys inside the kernel/hardware. If hardware supports
key generation and retention, it will use it, else the keys
will be generated inside the kernel. Either way it's a win, we don't
reveal the private keys to user-space. Going through the keyring
subsystem seems superfluous in this case.

My use case compared with the one of using keyring subsystem to access
keys from TPMs or Intel's sgx enclave seem orthogonal. What do you
think?

Cheers,
ta

[1] http://www.microchip.com/wwwproducts/en/ATECC508A
The driver can be found in drivers/crypto/atmel-ecc* in Herbert's tree.

Re: [PATCH 7/8] crypto: ecdh - constify key

[Tudor Ambarus]

On 04/19/2017 02:07 AM, Stephan Müller wrote:

Signed-off-by: Stephan Mueller <smuel...@chronox.de>
---
  include/crypto/ecdh.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/crypto/ecdh.h b/include/crypto/ecdh.h
index 03a64f6..b5bb149 100644
--- a/include/crypto/ecdh.h
+++ b/include/crypto/ecdh.h
@@ -40,7 +40,7 @@
   */
  struct ecdh {
unsigned short curve_id;
-   char *key;
+   const char *key;
unsigned short key_size;
  };
  



I just came across this and remembered that Stephan already
made a patch, so:

Acked-by: Tudor Ambarus <tudor.amba...@microchip.com>

Re: [PATCH 0/6] Add support for ECDSA algorithm

[Tudor Ambarus]

Hi, Sandy,

On 08/22/2017 08:22 PM, Sandy Harris wrote:

On Tue, Aug 22, 2017 at 12:14 PM, Tudor Ambarus
<tudor.amba...@microchip.com> wrote:

Hi, Herbert,

On 02/02/2017 03:57 PM, Herbert Xu wrote:


Yes but RSA had an in-kernel user in the form of module signature
verification.  We don't add algorithms to the kernel without
actual users.  So this patch-set needs to come with an actual
in-kernel user of ECDSA.



ECDSA can be used by the kernel module signing facility too. Is there
any interest in using ECDSA by the kernel module signing facility?


I'd say keep it simple wherever possible; adding an algorithm should
need "is required by" not just "can be used by".

Even then, there is room for questions. In particular, whether such a
fragile algorithm should be trusted at all, let alone for signatures
on infrastructure modules that the whole OS will trust.
https://en.wikipedia.org/wiki/Elliptic_Curve_Digital_Signature_Algorithm#Security



ECDSA is a better alternative to RSA for digital signatures assuming
that you don't have implementation bugs.

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

security strength | RSA key length (bits) | ECDSA key lengths (bits)
   112   2048224-255
   128   3072256-383

When comparing to RSA, ECDSA promises better computational efficiency,
signature size and bandwith (see [2]).

Cheers,
ta

[1] NIST.SP.800-57pt1r4, section 5.6.1,  table 2
[2] rfc4754, rfc6979