Re: [PATCH V3 1/2] evm: Don't deadlock if a crypto algorithm is unavailable
On Fri, Jun 8, 2018 at 2:57 PM Matthew Garrett wrote: > > When EVM attempts to appraise a file signed with a crypto algorithm the > kernel doesn't have support for, it will cause the kernel to trigger a > module load. If the EVM policy includes appraisal of kernel modules this > will in turn call back into EVM - since EVM is holding a lock until the > crypto initialisation is complete, this triggers a deadlock. Add a > CRYPTO_NOLOAD flag and skip module loading if it's set, and add that flag > in the EVM case in order to fail gracefully with an error message > instead of deadlocking. Hi Herbert, Does this explain the problem sufficiently?
Re: [PATCH 8/9] crypto: atmel-ecc: Detail what is unlocked
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
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
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
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
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
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
