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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Mon, 9 Nov 2015 11:46:52 +0800
From:	Zain <zain.wang@...k-chips.com>
To:	Heiko Stuebner <heiko@...ech.de>
Cc:	zhengsq@...k-chips.com, hl@...k-chips.com,
	herbert@...dor.apana.org.au, davem@...emloft.net,
	mturquette@...libre.com, pawel.moll@....com,
	ijc+devicetree@...lion.org.uk, robh+dt@...nel.org,
	galak@...eaurora.org, linux@....linux.org.uk, mark.rutland@....com,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org,
	eddie.cai@...k-chips.com
Subject: Re: [PATCH v2 1/4] Crypto: Crypto driver support aes/des/des3 for
 rk3288

Hi Heiko,
Thanks for your serious comments always.

On 2015年11月08日 07:19, Heiko Stuebner wrote:
> Hi Zain,
>
> looks like my comment on v1 came later than your v2 submission,
> so here it is again :-)
>
> Am Freitag, 6. November 2015, 09:17:21 schrieb Zain Wang:
>> The names registered are:
>>     ecb(aes) cbc(aes) ecb(des) cbc(des) ecb(des3_ede) cbc(des3_ede)
>> You can alloc tags above in your case.
>>
>> And other algorithms and platforms will be added later on.
>>
>> Signed-off-by: Zain Wang <zain.wang@...k-chips.com>
>> ---
>>
> [...]
>
>> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c
>> new file mode 100644
>> index 0000000..c2a419b
>> --- /dev/null
>> +++ b/drivers/crypto/rockchip/rk3288_crypto.c
> [...]
>
>> +static int rk_crypto_probe(struct platform_device *pdev)
>> +{
>> +	int err = 0;
>> +	struct resource *res;
>> +	struct device *dev = &pdev->dev;
>> +	struct crypto_info_t *crypto_info;
>> +
> rk3288 chromebooks use the crypto-engine to validate the boot images and
> seem to leave it in a half-on state. This results in an irq pending
> during probe and thus a null-pointer dereference in the irq-handler, as
> it runs before the crypto-device is fully initialized.
>
> resetting the crypto block, successfull fixed that issue, so I did the
> following change:
>
> -------------------
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 121b6d5..e978fb2 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -182,6 +182,8 @@
>                               "hclk",
>                               "sclk",
>                               "apb_pclk";
> +               resets = <&cru SRST_CRYPTO>;
> +               reset-names = "crypto";
>                 status = "okay";
>         };
>  
> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c
> index 02830f2..2245d3d 100644
> --- a/drivers/crypto/rockchip/rk3288_crypto.c
> +++ b/drivers/crypto/rockchip/rk3288_crypto.c
> @@ -18,6 +18,7 @@
>  #include <linux/of.h>
>  #include <linux/clk.h>
>  #include <linux/crypto.h>
> +#include <linux/reset.h>
>  
>  struct crypto_info_t *crypto_p;
>  
> @@ -266,6 +267,15 @@ static int rk_crypto_probe(struct platform_device *pdev)
>         struct resource *res;
>         struct device *dev = &pdev->dev;
>         struct crypto_info_t *crypto_info;
> +       struct reset_control *rst;
> +
> +       /* reset the block to remove any pending actions */
> +       rst = devm_reset_control_get(dev, "crypto");
> +       if (!IS_ERR(rst)) {
> +               reset_control_assert(rst);
> +               usleep_range(10, 20);
> +               reset_control_deassert(rst);
> +       }
>  
>         crypto_info = devm_kzalloc(&pdev->dev,
>                         sizeof(*crypto_info), GFP_KERNEL);
> -------------------
>
Ok! done!
>> +	crypto_info = devm_kzalloc(&pdev->dev,
>> +				   sizeof(*crypto_info), GFP_KERNEL);
>> +	if (!crypto_info)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&crypto_info->lock);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	crypto_info->reg = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(crypto_info->reg)) {
>> +		err = PTR_ERR(crypto_info->reg);
>> +		goto err_ioremap;
>> +	}
>> +
>> +	crypto_info->aclk = devm_clk_get(&pdev->dev, "aclk");
>> +	if (IS_ERR(crypto_info->aclk)) {
>> +		err = PTR_ERR(crypto_info->aclk);
>> +		goto err_ioremap;
>> +	}
>> +
>> +	crypto_info->hclk = devm_clk_get(&pdev->dev, "hclk");
>> +	if (IS_ERR(crypto_info->hclk)) {
>> +		err = PTR_ERR(crypto_info->hclk);
>> +		goto err_ioremap;
>> +	}
>> +
>> +	crypto_info->sclk = devm_clk_get(&pdev->dev, "sclk");
>> +	if (IS_ERR(crypto_info->sclk)) {
>> +		err = PTR_ERR(crypto_info->sclk);
>> +		goto err_ioremap;
>> +	}
>> +
>> +	crypto_info->dmaclk = devm_clk_get(&pdev->dev, "apb_pclk");
>> +	if (IS_ERR(crypto_info->dmaclk)) {
>> +		err = PTR_ERR(crypto_info->dmaclk);
>> +		goto err_ioremap;
>> +	}
>> +
>> +	crypto_info->irq = platform_get_irq(pdev, 0);
>> +	if (crypto_info->irq < 0) {
>> +		dev_warn(crypto_info->dev,
>> +			 "control Interrupt is not available.\n");
>> +		err = crypto_info->irq;
>> +		goto err_ioremap;
>> +	}
>> +
>> +	err = devm_request_irq(&pdev->dev, crypto_info->irq, crypto_irq_handle,
>> +			       IRQF_SHARED, "rk-crypto", pdev);
>> +
>> +	if (err) {
>> +		dev_err(crypto_info->dev, "irq request failed.\n");
>> +		goto err_ioremap;
>> +	}
>> +
>> +	crypto_info->dev = &pdev->dev;
>> +	platform_set_drvdata(pdev, crypto_info);
>> +	crypto_p = crypto_info;
>> +
>> +	tasklet_init(&crypto_info->crypto_tasklet,
>> +		     rk_crypto_tasklet_cb, (unsigned long)crypto_info);
>> +	crypto_init_queue(&crypto_info->queue, 50);
>> +
>> +	crypto_info->enable_clk = rk_crypto_enable_clk;
>> +	crypto_info->disable_clk = rk_crypto_disable_clk;
>> +	crypto_info->load_data = rk_load_data;
>> +	crypto_info->unload_data = rk_unload_data;
>> +
>> +	err = rk_crypto_register();
>> +	if (err) {
>> +		dev_err(dev, "err in register alg");
>> +		goto err_reg_alg;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_reg_alg:
>> +	free_irq(crypto_info->irq, crypto_info);
>> +err_ioremap:
>> +	crypto_p = NULL;
>> +
>> +	return err;
>> +}
>> +
> [...]
>
>> diff --git a/drivers/crypto/rockchip/rk3288_crypto.h b/drivers/crypto/rockchip/rk3288_crypto.h
>> new file mode 100644
>> index 0000000..cf4cd18
>> --- /dev/null
>> +++ b/drivers/crypto/rockchip/rk3288_crypto.h
>> @@ -0,0 +1,222 @@
> [...]
>
>> +struct crypto_info_t {
> this is highly rockchip specific, so should probably be named
>         rk_crypto_info
> or similar instead of a generic sounding crypto_info_t
Ok! Done!
>
> [...]
>
>> +	struct device			*dev;
>> +	struct clk			*aclk;
>> +	struct clk			*hclk;
>> +	struct clk			*sclk;
>> +	struct clk			*dmaclk;
>> +	void __iomem			*reg;
>> +	int				irq;
>> +	struct crypto_queue		queue;
>> +	struct tasklet_struct		crypto_tasklet;
>> +	struct ablkcipher_request	*ablk_req;
>> +	/* device lock */
>> +	spinlock_t			lock;
>> +
>> +	/* the public variable */
>> +	struct scatterlist		*sg_src;
>> +	struct scatterlist		*sg_dst;
>> +	struct scatterlist		sg_tmp;
>> +	struct scatterlist		*first;
>> +	unsigned int			left_bytes;
>> +	void				*addr_vir;
>> +	int				aligned;
>> +	int				align_size;
>> +	size_t				nents;
>> +	unsigned int			total;
>> +	unsigned int			count;
>> +	u32				mode;
>> +	dma_addr_t			addr_in;
>> +	dma_addr_t			addr_out;
>> +	int (*start)(struct crypto_info_t *dev);
>> +	int (*update)(struct crypto_info_t *dev);
>> +	void (*complete)(struct crypto_info_t *dev, int err);
>> +	int (*enable_clk)(struct crypto_info_t *dev);
>> +	void (*disable_clk)(struct crypto_info_t *dev);
>> +	int (*load_data)(struct crypto_info_t *dev,
>> +			 struct scatterlist *sg_src,
>> +			 struct scatterlist *sg_dst);
>> +	void (*unload_data)(struct crypto_info_t *dev);
>> +};
>> +
>> +/* the private variable of cipher */
>> +struct rk_cipher_ctx {
>> +	struct crypto_info_t		*dev;
>> +	unsigned int			keylen;
>> +};
>> +
>> +extern struct crypto_info_t *crypto_p;
>> +
>> +extern struct crypto_alg rk_ecb_aes_alg;
>> +extern struct crypto_alg rk_cbc_aes_alg;
>> +extern struct crypto_alg rk_ecb_des_alg;
>> +extern struct crypto_alg rk_cbc_des_alg;
>> +extern struct crypto_alg rk_ecb_des3_ede_alg;
>> +extern struct crypto_alg rk_cbc_des3_ede_alg;
>> +
>> +#endif
>> diff --git a/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
>> new file mode 100644
>> index 0000000..28b49c9
>> --- /dev/null
>> +++ b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
>> @@ -0,0 +1,511 @@
> [...]
>
>> +static int rk_ablk_cra_init(struct crypto_tfm *tfm)
>> +{
>> +	struct rk_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
>> +
>> +	ctx->dev = crypto_p;
> please no static pointers for devices.
>
> For example, sunxi_ss does the following to transport the core device-data
> into the init function:
>
>         struct sun4i_tfm_ctx *op = crypto_tfm_ctx(tfm);
>         struct crypto_alg *alg = tfm->__crt_alg;
>         struct sun4i_ss_alg_template *algt;
>
>         algt = container_of(alg, struct sun4i_ss_alg_template, alg.crypto);
>         op->ss = algt->ss;
>
> so you could probably do something similar
>
> Same of course for the other crypto_p users.
Ok! Done!
>
>
> Thanks
> Heiko
>
>
>
Thanks
Zain

--
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