| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <52D01572.5010509@samsung.com>
Date: Fri, 10 Jan 2014 16:44:50 +0100
From: Tomasz Figa <t.figa@...sung.com>
To: Naveen Krishna Chatradhi <ch.naveen@...sung.com>,
linux-crypto@...r.kernel.org, linux-samsung-soc@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, vzapolskiy@...il.com,
herbert@...dor.apana.org.au, naveenkrishna.ch@...il.com,
cpgs@...sung.com, tomasz.figa@...il.com,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 3/8 v3] crypto:s5p-sss: Add support for SSS module on Exynos
Hi Naveen,
Please see my comments inline.
On 10.01.2014 12:42, Naveen Krishna Chatradhi wrote:
> This patch adds new compatible and variant struct to support the SSS
> module on Exynos4 (Exynos4210), Exynos5 (Exynos5420 and Exynos5250)
> for which
> 1. AES register are at an offset of 0x200 and
> 2. hash interrupt is not available
>
> Signed-off-by: Naveen Krishna Ch <ch.naveen@...sung.com>
> CC: Herbert Xu <herbert@...dor.apana.org.au>
> CC: David S. Miller <davem@...emloft.net>
> CC: Vladimir Zapolskiy <vzapolskiy@...il.com>
> TO: <linux-crypto@...r.kernel.org>
> CC: <linux-samsung-soc@...r.kernel.org>
> ---
> Changes since v2:
> 1. Added variant struct to handle the differences in SSS modules
> 2. Changed the compatible strings to exynos4210-secss
> 3. Other changes suggested by Tomasz
>
> .../devicetree/bindings/crypto/samsung-sss.txt | 20 ++++
> drivers/crypto/s5p-sss.c | 110 +++++++++++++++-----
> 2 files changed, 106 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/crypto/samsung-sss.txt b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
> index 2f9d7e4..fdc7d8b 100644
> --- a/Documentation/devicetree/bindings/crypto/samsung-sss.txt
> +++ b/Documentation/devicetree/bindings/crypto/samsung-sss.txt
> @@ -8,13 +8,33 @@ The SSS module in S5PV210 SoC supports the following:
> -- SHA-1/SHA-256/MD5/HMAC (SHA-1/SHA-256/MD5)/PRNG
> -- PRNG: Pseudo Random Number Generator
>
> +The SSS module in Exynos4 (Exynos4210) and
> +Exynos5 (Exynos5420 and Exynos5250) SoCs
> +supports the following also:
> +-- ARCFOUR (ARC4)
> +-- True Random Number Generator (TRNG)
> +-- Secure Key Manager
> +
> Required properties:
>
> - compatible : Should contain entries for this and backward compatible
> SSS versions:
> - "samsung,s5pv210-secss" for S5PV210 SoC.
> + - "samsung,exynos4210-secss" for Exynos4210, Exynos5250 and Exynos5420 SoCs.
You can also add Exynos4212/4412 to the list.
> - reg : Offset and length of the register set for the module
> - interrupts : the interrupt-specifier for the SSS module.
> Two interrupts "feed control and hash" in case of S5PV210
> + One interrupts "feed control" in case of Exynos4210,
> + Exynos5250 and Exynos5420 SoCs.
You can refer to compatible string here instead of listing all the SoCs.
> - clocks : the required gating clock for the SSS module.
> - clock-names : the gating clock name to be requested in the SSS driver.
Again, please specify name of the clock in property description. The
proper description for both clock properties should be:
- clock-names : list of device clock input names; should contain one
entry - "secss".
- clocks : list of clock phandle and specifier pairs for all clocks
listed in clock-names property.
> +
> +Example:
> + /* SSS_VER_5 */
> + sss@...30000 {
> + compatible = "samsung,exynos4210-secss";
> + reg = <0x10830000 0x10000>;
> + interrupts = <0 112 0>;
> + clocks = <&clock 471>;
> + clock-names = "secss";
> + };
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 2da5617..f274f5f 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -106,7 +106,7 @@
> #define SSS_REG_FCPKDMAO 0x005C
>
> /* AES registers */
> -#define SSS_REG_AES_CONTROL 0x4000
> +#define SSS_REG_AES_CONTROL 0x00
> #define SSS_AES_BYTESWAP_DI _BIT(11)
> #define SSS_AES_BYTESWAP_DO _BIT(10)
> #define SSS_AES_BYTESWAP_IV _BIT(9)
> @@ -122,21 +122,26 @@
> #define SSS_AES_CHAIN_MODE_CTR _SBF(1, 0x02)
> #define SSS_AES_MODE_DECRYPT _BIT(0)
>
> -#define SSS_REG_AES_STATUS 0x4004
> +#define SSS_REG_AES_STATUS 0x04
> #define SSS_AES_BUSY _BIT(2)
> #define SSS_AES_INPUT_READY _BIT(1)
> #define SSS_AES_OUTPUT_READY _BIT(0)
>
> -#define SSS_REG_AES_IN_DATA(s) (0x4010 + (s << 2))
> -#define SSS_REG_AES_OUT_DATA(s) (0x4020 + (s << 2))
> -#define SSS_REG_AES_IV_DATA(s) (0x4030 + (s << 2))
> -#define SSS_REG_AES_CNT_DATA(s) (0x4040 + (s << 2))
> -#define SSS_REG_AES_KEY_DATA(s) (0x4080 + (s << 2))
> +#define SSS_REG_AES_IN_DATA(off, s) ((off + 0x10) + (s << 2))
> +#define SSS_REG_AES_OUT_DATA(off, s) ((off + 0x20) + (s << 2))
> +#define SSS_REG_AES_IV_DATA(off, s) ((off + 0x30) + (s << 2))
> +#define SSS_REG_AES_CNT_DATA(off, s) ((off + 0x40) + (s << 2))
> +#define SSS_REG_AES_KEY_DATA(off, s) ((off + 0x80) + (s << 2))
I still somehow don't like this. Such macros are only hiding operations
performed by the driver. See my comment below, in the code that
references them, to see my proposal.
>
> #define SSS_REG(dev, reg) ((dev)->ioaddr + (SSS_REG_##reg))
> #define SSS_READ(dev, reg) __raw_readl(SSS_REG(dev, reg))
> #define SSS_WRITE(dev, reg, val) __raw_writel((val), SSS_REG(dev, reg))
>
> +#define SSS_AES_REG(dev, reg) ((dev)->ioaddr + SSS_REG_##reg + \
> + dev->variant->aes_offset)
> +#define SSS_AES_WRITE(dev, reg, val) __raw_writel((val), \
> + SSS_AES_REG(dev, reg))
> +
> /* HW engine modes */
> #define FLAGS_AES_DECRYPT _BIT(0)
> #define FLAGS_AES_MODE_MASK _SBF(1, 0x03)
> @@ -146,6 +151,20 @@
> #define AES_KEY_LEN 16
> #define CRYPTO_QUEUE_LEN 1
>
> +/**
> + * struct samsung_aes_variant - platform specific SSS driver data
> + * @has_hash_irq: true if SSS module uses hash interrupt, false otherwise
> + * @aes_offset: AES register offset from SSS module's base.
> + *
> + * Specifies platform specific configuration of SSS module.
> + * Note: A structure for driver specific platform data is used for future
> + * expansion of its usage.
> + */
> +struct samsung_aes_variant {
> + bool has_hash_irq;
> + unsigned int aes_offset;
> +};
> +
> struct s5p_aes_reqctx {
> unsigned long mode;
> };
> @@ -174,16 +193,48 @@ struct s5p_aes_dev {
> struct crypto_queue queue;
> bool busy;
> spinlock_t lock;
> +
> + struct samsung_aes_variant *variant;
> };
>
> static struct s5p_aes_dev *s5p_dev;
>
> +static const struct samsung_aes_variant s5p_aes_data = {
> + .has_hash_irq = true,
> + .aes_offset = 0x4000,
> +};
> +
> +static const struct samsung_aes_variant exynos_aes_data = {
> + .has_hash_irq = false,
> + .aes_offset = 0x200,
> +};
> +
> static const struct of_device_id s5p_sss_dt_match[] = {
> - { .compatible = "samsung,s5pv210-secss", },
> + {
> + .compatible = "samsung,s5pv210-secss",
> + .data = &s5p_aes_data,
> + },
> + {
> + .compatible = "samsung,exynos4210-secss",
> + .data = &exynos_aes_data,
> + },
> { },
> };
> MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
>
> +static inline struct samsung_aes_variant *find_s5p_sss_version
> + (struct platform_device *pdev)
> +{
> + if (IS_ENABLED(CONFIG_OF) && (pdev->dev.of_node)) {
> + const struct of_device_id *match;
> + match = of_match_node(s5p_sss_dt_match,
> + pdev->dev.of_node);
> + return (struct samsung_aes_variant *)match->data;
> + }
> + return (struct samsung_aes_variant *)
> + platform_get_device_id(pdev)->driver_data;
> +}
> +
> static void s5p_set_dma_indata(struct s5p_aes_dev *dev, struct scatterlist *sg)
> {
> SSS_WRITE(dev, FCBRDMAS, sg_dma_address(sg));
> @@ -327,16 +378,21 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
> static void s5p_set_aes(struct s5p_aes_dev *dev,
> uint8_t *key, uint8_t *iv, unsigned int keylen)
> {
> + struct samsung_aes_variant *var = dev->variant;
> void __iomem *keystart;
>
> - memcpy(dev->ioaddr + SSS_REG_AES_IV_DATA(0), iv, 0x10);
> + memcpy(dev->ioaddr + SSS_REG_AES_IV_DATA
> + (var->aes_offset, 0), iv, 0x10);
What about adding aes_ioaddr to s5p_aes_dev struct? Then you could
access the registers as follows:
memcpy(dev->aes_ioaddr + SSS_REG_AES_IV_DATA(0), iv, 0x10);
The registers would be defined as offsets of AES base, e.g:
#define SSS_REG_AES_IV_DATA(s) (0x10 + (s << 2))
>
> if (keylen == AES_KEYSIZE_256)
> - keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(0);
> + keystart = dev->ioaddr +
> + SSS_REG_AES_KEY_DATA(var->aes_offset, 0);
> else if (keylen == AES_KEYSIZE_192)
> - keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(2);
> + keystart = dev->ioaddr +
> + SSS_REG_AES_KEY_DATA(var->aes_offset, 2);
> else
> - keystart = dev->ioaddr + SSS_REG_AES_KEY_DATA(4);
> + keystart = dev->ioaddr +
> + SSS_REG_AES_KEY_DATA(var->aes_offset, 4);
>
> memcpy(keystart, key, keylen);
> }
> @@ -386,7 +442,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
> if (err)
> goto outdata_error;
>
> - SSS_WRITE(dev, AES_CONTROL, aes_control);
> + SSS_AES_WRITE(dev, AES_CONTROL, aes_control);
SSS_AES_WRITE would be define using dev->aes_ioaddr instead of dev->ioaddr.
Otherwise the patch looks fine.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists