| Message ID | 20180605134950.6605-7-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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 <linus.walleij@linaro.org> > --- > 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 <linux/scatterlist.h> > #include <linux/slab.h> > #include <linux/workqueue.h> > +#include <linux/random.h> includes should be ordered alphabetically. > #include <crypto/internal/kpp.h> > #include <crypto/ecdh.h> > #include <crypto/kpp.h> > @@ -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(&client->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(&client->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(&client->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(&client->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(&driver_data.i2c_list_lock); > list_add_tail(&i2c_priv->i2c_client_list_node, > &driver_data.i2c_client_list); >
On Tue, Jun 12, 2018 at 3:13 PM Tudor Ambarus <tudor.ambarus@microchip.com> wrote: > > +#include <linux/random.h> > > includes should be ordered alphabetically. OK fixed. > > +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; Is your point trying to help the compiler to lay things out on the stack? Isn't that premature optimization? I was under the impression that we should leave this stuff to the compiler. But sure, it is a small change so I did it anyways :) > > + > > + 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); Whoops added a goto contruction to do proper free:ing of cmd, thanks. > > + dev_err(&client->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]? OK. > > + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_4_7); > > can't we read the serial_no once? Sorry not following. Do you mean we need an accessor to read several config words in succession? I think I read somewhere that the device only supports reading one config word at the time and the serial number is spread out over three config words... > > + for (i = 0; i < 4; i++) > > + serial[4+i] = cmd->data[RSP_DATA_IDX + i]; > > spaces preferred around that '+' OK. > > + dev_info(&client->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? Cuddly feeling I guess. If there is a problem with the device the user might want to report the serial number to the manufacturer? Yours, Linus Walleij
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 <linux/scatterlist.h> #include <linux/slab.h> #include <linux/workqueue.h> +#include <linux/random.h> #include <crypto/internal/kpp.h> #include <crypto/ecdh.h> #include <crypto/kpp.h> @@ -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 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) { + dev_err(&client->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]; + + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_4_7); + ret = atmel_ecc_send_receive(client, cmd); + if (ret) { + dev_err(&client->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]; + + + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_8_I2CEN); + ret = atmel_ecc_send_receive(client, cmd); + if (ret) { + dev_err(&client->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(&client->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]); + 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(&driver_data.i2c_list_lock); list_add_tail(&i2c_priv->i2c_client_list_node, &driver_data.i2c_client_list);
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 <linus.walleij@linaro.org> --- drivers/crypto/atmel-ecc.c | 56 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)