Re: [PATCH v2] drivers/crypto/nx: saves chaining value from co-processor
Hi Fin, I don't know anything about crypto so I can only critique you on your patch submission technique :) ... On Wed, Aug 07, 2013 at 06:15:50PM -0500, Fionnuala Gunter wrote: This patch fixes a bug that is triggered when cts(cbc(aes)) is used with nx-crypto driver on input larger than 32 bytes. The chaining value from co-processor was not being saved. This value is needed because it is used as the IV by cts(cbc(aes)). Signed-off-by: Fionnuala Gunter f...@linux.vnet.ibm.com Reviewed-by: Marcelo Cerri mhce...@linux.vnet.ibm.com --- v2. changed signed-off-by to reviewed-by and added more details to description This bug appeared in the original submission (v3.5) Ideally this should identify the commit, so: This bug was introduced in the original submission (v3.5), commit 856d673 powerpc/crypto: AES-CBC mode routines for nx encryption. Including the subject of the commit is handy in case the patch has been backported somewhere, in which case the commit sha will be different. It should definitely be part of the commit message, not below the ---. And Ben might disagree but I think with a clear cut bug fix like this it should include the CC to stable, so: Cc: sta...@vger.kernel.org # 3.5+ cheers -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] ARM: dts: mxs: set dcp to disabled by default
Reintroduce 'status = disabled' for the dcp node that was dropped by
commit 519d8b1a Added support for Freescale's DCP co-processor.
Explicitly enable it in imx28-evk which is referenced in the commit
message of that commit.
Signed-off-by: Lothar Waßmann l...@karo-electronics.de
---
arch/arm/boot/dts/imx28-evk.dts |4
arch/arm/boot/dts/imx28.dtsi|3 ++-
2 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
index dff2279..ac790bb 100644
--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -361,3 +361,7 @@
default-brightness-level = 6;
};
};
+
+dcp {
+ status = okay;
+};
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index e459d63..ea99d09 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -794,9 +794,10 @@
};
dcp: dcp@80028000 {
+ compatible = fsl-dcp;
reg = 0x80028000 0x2000;
interrupts = 52 53 54;
- compatible = fsl-dcp;
+ status = disabled;
};
pxp: pxp@8002a000 {
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] crypto: dcp - cleanup: commas at end of struct initializers where appropriate
Signed-off-by: Lothar Waßmann l...@karo-electronics.de
---
drivers/crypto/dcp.c | 11 +--
1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/dcp.c b/drivers/crypto/dcp.c
index a8a7dd4..6a2495e 100644
--- a/drivers/crypto/dcp.c
+++ b/drivers/crypto/dcp.c
@@ -651,8 +651,7 @@ static struct crypto_alg algs[] = {
.encrypt = dcp_aes_cbc_encrypt,
.decrypt = dcp_aes_cbc_decrypt,
.ivsize = AES_KEYSIZE_128,
- }
-
+ },
},
};
@@ -890,8 +889,8 @@ static int dcp_remove(struct platform_device *pdev)
}
static struct of_device_id fs_dcp_of_match[] = {
- { .compatible = fsl-dcp},
- {},
+ { .compatible = fsl-dcp, },
+ {}
};
static struct platform_driver fs_dcp_driver = {
@@ -900,8 +899,8 @@ static struct platform_driver fs_dcp_driver = {
.driver = {
.name = fsl-dcp,
.owner = THIS_MODULE,
- .of_match_table = fs_dcp_of_match
- }
+ .of_match_table = fs_dcp_of_match,
+ },
};
module_platform_driver(fs_dcp_driver);
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Questions about the Crypto API
Thanks for Marcelo and Herbert for the questions and answers. I have a few more questions related to the same subject of API. 1. In the crypto_async_request, is the list element available to the driver to use? I see most of drivers do crypto_enqueue_request() to keep track of the outstanding async requests. The only exception I have seen so far is talitos driver where they implement their FIFO to keep track the outstanding async requests. 2. The async driver simply returns instead of sleep. The requestor of the async request, does wait_for_completion() for the completion callback from driver. Can it be wait_for_completion_interruptible() such as testmgr.c does? If the sleep can be interruptible, how does driver know the request has been aborted? The request can be still in the driver queue waiting for the hw to finish execution. How is driver aware to dequeue this aborted request? If not, the link list can be corrupted and cause kernel to crash potentially. Chemin -Original Message- From: linux-crypto-ow...@vger.kernel.org [mailto:linux-crypto-ow...@vger.kernel.org] On Behalf Of Herbert Xu Sent: Thursday, August 08, 2013 1:02 AM To: Marcelo Cerri Cc: linux-crypto@vger.kernel.org Subject: Re: Questions about the Crypto API On Tue, Aug 06, 2013 at 11:16:12AM -0300, Marcelo Cerri wrote: Herbert, Let me include a few more questions. There are flags in several structures such as crypto_async_request, blkcipher_desc and crypto_tfm. How they are intended to be used? For example if I want to explicitly make a call that shouldn't sleep, should I clear the CRYPTO_TFM_REQ_MAY_SLEEP in one of these structures? And in which one? Since cryptographic methods can be called in softirq contexts, is the caller responsible for setting this flag correctly based on the context it is running? Yes, although MAY_SLEEP is mostly used by synchronous implementations since async drivers can simply return instead of sleeping. Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] crypto: dcp: rename 'compatible' property to 'fsl,dcp'
Leave the old 'fsl-dcp' value in place with an appropriate comment
until external users have updated their DTBs.
Signed-off-by: Lothar Waßmann l...@karo-electronics.de
---
arch/arm/boot/dts/imx28.dtsi |2 +-
drivers/crypto/dcp.c |2 ++
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index ea99d09..0584935 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -794,7 +794,7 @@
};
dcp: dcp@80028000 {
- compatible = fsl-dcp;
+ compatible = fsl,dcp;
reg = 0x80028000 0x2000;
interrupts = 52 53 54;
status = disabled;
diff --git a/drivers/crypto/dcp.c b/drivers/crypto/dcp.c
index 6a2495e..72196c0 100644
--- a/drivers/crypto/dcp.c
+++ b/drivers/crypto/dcp.c
@@ -889,6 +889,8 @@ static int dcp_remove(struct platform_device *pdev)
}
static struct of_device_id fs_dcp_of_match[] = {
+ { .compatible = fsl,dcp, },
+ /* To be removed when the DT blobs referencing this have been updated */
{ .compatible = fsl-dcp, },
{}
};
--
1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] ARM: dts: mxs: dcp cleanup patches
This patch series does the following: - let the driver be disabled by default in imx28.dtsi - coding style cleanups in drivers/crypto/dcp.c - use the official 'fsl,' prefix in the 'compatible' property The last patch adds a new entry to the of_match_table of the driver, so that current DT blobs will still work. When out of tree users (mentioned in the commit log of 519d8b1a Added support for Freescale's DCP co-processor) have updated their DTBs the old entry 'fsl-dsp' can be removed from the driver. This patch series requires at least [PATCH 2/8] ARM: dts: mxs: add labels to most nodes for easier reference from the patch series: 1375966287-6784-1-git-send-email...@karo-electronics.de to be applied. arch/arm/boot/dts/imx28.dtsi |2 +- b/arch/arm/boot/dts/imx28-evk.dts |4 b/arch/arm/boot/dts/imx28.dtsi|3 ++- b/drivers/crypto/dcp.c| 11 +-- drivers/crypto/dcp.c |2 ++ 5 files changed, 14 insertions(+), 8 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Questions about the Crypto API
Hi Herbert -Original Message- From: linux-crypto-ow...@vger.kernel.org [mailto:linux-crypto- ow...@vger.kernel.org] On Behalf Of Herbert Xu Sent: Tuesday, August 06, 2013 12:30 PM To: Marcelo Cerri Cc: linux-crypto@vger.kernel.org Subject: Re: Questions about the Crypto API On Mon, Aug 05, 2013 at 05:25:57PM -0300, Marcelo Cerri wrote: My first doubt is regarding which kind of concurrency the Crypto API allows. For example, can a single `struct crypto_tfm` be used by two concurrent calls? I'm asking about that because I noticed that for Yes. In this post, you mentioned that tfm is single threaded. Am I misinterpreting your statement? http://www.mail-archive.com/linux-crypto@vger.kernel.org/msg08689.html blkcipher the only implementation-specific context that can be used is allocated inside the tfm struct. Both blkcipher and ablkcipher are meant to be fully reentrant. So you must take necessary precautions if your implementation is not reentrant, e.g., by locking. I'm working to fix some bugs in the NX driver (located in drivers/crypto/nx), and one issue that we are facing is that NFS when using Kerberos uses the same tfm with different kthreads. That causes concurrent accesses to the internal data stored into the context and incorrect results. So my question here is: should this type of concurrency be handled by the driver or a caller is not allowed to use the same tfm for concurrent calls? From what you've said NFS seems to be doing the right thing, so the bug would be in the driver. My second doubt is regarding the difference between ablkcipher and blkcipher. I do understand their difference from caller's point of view. But I'm not sure what are the consequences of implementing a driver using one or another option. For example, can a blkcipher implementation be used asynchronously and vice versa? In general which model you pick for drivers depend on what your hardware looks like. For example, padlock-aes uses the blkcipher model because the hardware presents itself through a synchronous CPU instruction, while most other drivers use the ablkcipher interface because the underlying hardware completes asynchronously. A blkcipher implementation is always available through both the blkcipher and the ablkcipher interface. While an ablkcipher implementaiton can only be used through the ablkcipher interface. Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] crypto: dcp: rename 'compatible' property to 'fsl,dcp'
Hello.
On 08/08/2013 05:30 PM, Lothar Waßmann wrote:
Leave the old 'fsl-dcp' value in place with an appropriate comment
until external users have updated their DTBs.
Signed-off-by: Lothar Waßmann l...@karo-electronics.de
---
arch/arm/boot/dts/imx28.dtsi |2 +-
drivers/crypto/dcp.c |2 ++
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index ea99d09..0584935 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -794,7 +794,7 @@
};
dcp: dcp@80028000 {
By the way, I see that this is a crypto device, so it should be named
crypto according to ePAPR spec. [1]
- compatible = fsl-dcp;
+ compatible = fsl,dcp;
reg = 0x80028000 0x2000;
interrupts = 52 53 54;
status = disabled;
[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf
WBR, Sergei
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] ARM: dts: mxs: dcp cleanup patches
On Aug 8, 2013, at 8:30 AM, Lothar Waßmann wrote: This patch series does the following: - let the driver be disabled by default in imx28.dtsi - coding style cleanups in drivers/crypto/dcp.c - use the official 'fsl,' prefix in the 'compatible' property The last patch adds a new entry to the of_match_table of the driver, so that current DT blobs will still work. When out of tree users (mentioned in the commit log of 519d8b1a Added support for Freescale's DCP co-processor) have updated their DTBs the old entry 'fsl-dsp' can be removed from the driver. This patch series requires at least [PATCH 2/8] ARM: dts: mxs: add labels to most nodes for easier reference from the patch series: 1375966287-6784-1-git-send-email...@karo-electronics.de to be applied. arch/arm/boot/dts/imx28.dtsi |2 +- b/arch/arm/boot/dts/imx28-evk.dts |4 b/arch/arm/boot/dts/imx28.dtsi|3 ++- b/drivers/crypto/dcp.c| 11 +-- drivers/crypto/dcp.c |2 ++ 5 files changed, 14 insertions(+), 8 deletions(-) -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Is there an actual binding spec for fsl,dcp? If not, there really should be. - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
