Parity Error on keys used for DES crypto test
Hi,
I'm altering the Freescale Talitos Driver in order to support the SEC1
security engine, and I have a big issue with the DES test vectors in
testmgr.h:
The Sec Engine reports key parity error.
Looking at the keys defined in testmgr.h for DES3, it looks like there
is a real parity issue with the test vectors. A DES key is supposed to
have all bytes with an odd number of ones. It is not the case in the key
below. At least the second byte 0xC0 has an even number of ones.
static struct cipher_testvec des3_ede_cbc_enc_tv_template[] = {
{ /* Generated from openssl */
.key= \xE9\xC0\xFF\x2E\x76\x0B\x64\x24
\x44\x4D\x99\x5A\x12\xD6\x40\xC0
\xEA\xC2\x84\xE8\x14\x95\xDB\xE8,
So, how can this test vector work ?
Christophe
--
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: RFC: Crypto API User-interface
Hi, This is regarding the hash computation over a file with AF_ALG from user space. [without OpenSSL] The following link has the mail from Herbert with subject : RFC: Crypto API User-interface http://lwn.net/Articles/410848/ I was trying to take help from the code snippet he has put in his mail and I was doing the following to compute hash over a file of 16 bytes. The file contents are 123456789012345\n. I read the first 10 bytes first and computed hash over it. Then I read the next 6 bytes from the file and computed the hash and I had taken care of using the flags MSG_MORE and MSG_OOB. Following is what I am doing: 1. send(opfd, sbuf, len, MSG_MORE); 2. read(opfd, state, 20); 3. send(opfd, state, 20, MSG_OOB); 4. send(opfd, sbuf, len, MSG_MORE); 5. read(opfd, state, 20); 6. /*restore from a partial hash state */ 7. send(opfd, state, 20, MSG_OOB); 8. /* finalize */ 9. send(opfd, sbuf, 0, 0); 10. read(opfd, buf, 20); sbuf in line 1 contains: 1234567890 (the is not part of the data, the data is plain 10 ascii bytes for 1 through 9), len is 10. the state after line 2 contains correct hash of the above data. sbuf in line 4 contains: 12345\n. Len is 6. The hash I am seeing in buf after line 10 is not correct one. Is there anything obviously wrong with the above code? I am using 2.6.39.4 kernel on rhel 6.4 (santiago) Thanks for the help! ~Jitendra -- 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] crypto_user: Fix out-of-bounds read
On Tue, Apr 22, 2014 at 12:30:28PM -0700, Andy Lutomirski wrote: This is unlikely to be exploitable for anything except an OOPS. Cc: sta...@vger.kernel.org Signed-off-by: Andy Lutomirski l...@amacapital.net --- Notes: This is entirely untested, but it looks obviously correct to me. crypto/crypto_user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index 1512e41..bc7c4b5 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -460,7 +460,7 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) int type, err; type = nlh-nlmsg_type; - if (type CRYPTO_MSG_MAX) + if (type CRYPTO_MSG_BASE || type CRYPTO_MSG_MAX) return -EINVAL; Adding a check here is obviously harmless but I think this is only called from netlink_rcv_skb() which already checks: if (nlh-nlmsg_type NLMSG_MIN_TYPE) goto ack; NLMSG_MIN_TYPE is 0x10 as well, so I don't think we can hit your condition. Your patch freaked me out a little because this is one of the bugs that I should have caught throught static analysis. regards, dan carpenter -- 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] crypto_user: Fix out-of-bounds read
On Apr 23, 2014 4:40 AM, Dan Carpenter dan.carpen...@oracle.com wrote: On Tue, Apr 22, 2014 at 12:30:28PM -0700, Andy Lutomirski wrote: This is unlikely to be exploitable for anything except an OOPS. Cc: sta...@vger.kernel.org Signed-off-by: Andy Lutomirski l...@amacapital.net --- Notes: This is entirely untested, but it looks obviously correct to me. crypto/crypto_user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index 1512e41..bc7c4b5 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -460,7 +460,7 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) int type, err; type = nlh-nlmsg_type; - if (type CRYPTO_MSG_MAX) + if (type CRYPTO_MSG_BASE || type CRYPTO_MSG_MAX) return -EINVAL; Adding a check here is obviously harmless but I think this is only called from netlink_rcv_skb() which already checks: if (nlh-nlmsg_type NLMSG_MIN_TYPE) goto ack; NLMSG_MIN_TYPE is 0x10 as well, so I don't think we can hit your condition. Your patch freaked me out a little because this is one of the bugs that I should have caught throught static analysis. BUILD_BUG_ON(NLMSG_MIN_TYPE != CRYPTO_MSG_BASE) might be a better thing to add, then. I don't feel so bad for missing this. regards, dan carpenter -- 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 crypto 2/2] crypto: caam - add allocation failure handling in SPRINTFCAT macro
On 4/23/2014 2:56 AM, Marek Vasut wrote:
On Friday, April 18, 2014 at 12:01:42 PM, Horia Geanta wrote:
GFP_ATOMIC memory allocation could fail.
In this case, avoid NULL pointer dereference and notify user.
Cc: sta...@vger.kernel.org # 3.2+
If I recall correctly, you need to get the patch accepted into mainline before
sending it for -stable .
From Documentation/stable_kernel_rules.txt
- To have the patch automatically included in the stable tree, add the tag
Cc: sta...@vger.kernel.org
in the sign-off area. Once the patch is merged it will be applied to
the stable tree without anything else needing to be done by the author
or subsystem maintainer.
Cc: Kim Phillips kim.phill...@freescale.com
Signed-off-by: Horia Geanta horia.gea...@freescale.com
---
drivers/crypto/caam/error.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
index 9f25f5296029..0eabd81e1a90 100644
--- a/drivers/crypto/caam/error.c
+++ b/drivers/crypto/caam/error.c
@@ -16,9 +16,13 @@
char *tmp; \
\
tmp = kmalloc(sizeof(format) + max_alloc, GFP_ATOMIC); \
- sprintf(tmp, format, param);\
- strcat(str, tmp); \
- kfree(tmp); \
+ if (likely(tmp)) { \
+ sprintf(tmp, format, param);\
+ strcat(str, tmp); \
+ kfree(tmp); \
+ } else {\
+ strcat(str, kmalloc failure in SPRINTFCAT); \
This entire macro looks somewhat strange.
I am trying to fix it with minimal changes, so the patch qualifies for
-stable.
1) Can't you just snprintf() into $str + some offset ? Something like:
snprintf(str + strlen(str), str_total_sz - strlen(str), format, param);
I think this would work. It also gets rid of memory allocation.
Note that strlen(str) is undefined if str is not initialized /
null-terminated.
However, all code paths seem to touch this line in caam_jr_strstatus():
sprintf(outstr, %s: , status_src[ssrc].error);
before reaching SPRINTFCAT macros, so str is null-terminated.
I'll send v2.
2) Why is noone checking if the $str has enough space for contents of $tmp ?
All call sites reach this macro via caam_jr_strstatus(tmp, ...), which
is always called having:
char tmp[CAAM_ERROR_STR_MAX];
CAAM_ERROR_STR_MAX is 302, probably enough according to commit
de2954d66408da3ae34effda777bb564fd17781b (crypto: caam - fix printk
recursion for long error texts).
Horia
--
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 crypto 2/2] crypto: caam - add allocation failure handling in SPRINTFCAT macro
On Wednesday, April 23, 2014 at 06:35:45 PM, Horia Geantă wrote: [...] This entire macro looks somewhat strange. I am trying to fix it with minimal changes, so the patch qualifies for -stable. This is just broken and you're not fixing it. You're just feeding this slimy monster called technical debt more and more code, so it can grow and get uglier and uglier. I hope you have no attachment to this abomination, since I'd like to see it dead. 1) Can't you just snprintf() into $str + some offset ? Something like: snprintf(str + strlen(str), str_total_sz - strlen(str), format, param); I think this would work. It also gets rid of memory allocation. Note that strlen(str) is undefined if str is not initialized / null-terminated. However, all code paths seem to touch this line in caam_jr_strstatus(): sprintf(outstr, %s: , status_src[ssrc].error); before reaching SPRINTFCAT macros, so str is null-terminated. I'll send v2. No, let us first agree on how to fix this insane abomination please. But while I am looking, I see stuff like: caam_jr_strstatus() can call report_ccb_status( , CCB); (basically with a fixed-size string argument): 265 if (status_src[ssrc].report_ssed) 266 status_src[ssrc].report_ssed(status, outstr); Report_ccb_status( , CCB); will call report_jump_idx( , CCB); (still with fixed-size string arg), which contains your SPRINTFCAT() macro. This will expand to: ... strcat(CCB, tmp); ... So basically you are writing into a fixed-size string? But the string is three- bytes long, so you are overwriting kernel memory ? If I read the code wrong, I really apologize in advance. 2) Why is noone checking if the $str has enough space for contents of $tmp ? All call sites reach this macro via caam_jr_strstatus(tmp, ...), which is always called having: char tmp[CAAM_ERROR_STR_MAX]; CAAM_ERROR_STR_MAX is 302, probably enough according to commit de2954d66408da3ae34effda777bb564fd17781b (crypto: caam - fix printk recursion for long error texts). You are digging in Linux's crypto code, not OpenSSL. We need accurate fixes and accurate discussion ... using 'probably' does not work here. Best regards, Marek Vasut -- 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 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big
On Mon, 2014-04-21 at 20:45 +0200, Christian Engelmayer wrote:
Fix a potential memory leak in the error handling of test_aead_speed(). In
case
the size check on the associate data length parameter fails, the function goes
through the wrong exit label. Reported by Coverity - CID 1163870.
Signed-off-by: Christian Engelmayer cenge...@gmx.at
Acked.
Tim
---
crypto/tcrypt.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 870be7b..1856d7f 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -282,6 +282,11 @@ static void test_aead_speed(const char *algo, int enc,
unsigned int sec,
unsigned int *b_size;
unsigned int iv_len;
+ if (aad_size = PAGE_SIZE) {
+ pr_err(associate data length (%u) too big\n, aad_size);
+ return;
+ }
+
if (enc == ENCRYPT)
e = encryption;
else
@@ -323,14 +328,7 @@ static void test_aead_speed(const char *algo, int enc,
unsigned int sec,
b_size = aead_sizes;
do {
assoc = axbuf[0];
-
- if (aad_size PAGE_SIZE)
- memset(assoc, 0xff, aad_size);
- else {
- pr_err(associate data length (%u) too big\n,
- aad_size);
- goto out_nosg;
- }
+ memset(assoc, 0xff, aad_size);
sg_init_one(asg[0], assoc, aad_size);
if ((*keysize + *b_size) TVMEMSIZE * PAGE_SIZE) {
--
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 2/3] crypto: Fix potential leak in test_aead_speed() if crypto_alloc_aead() fails
On Mon, 2014-04-21 at 20:46 +0200, Christian Engelmayer wrote:
Fix a potential memory leak in the error handling of test_aead_speed(). In
case
crypto_alloc_aead() fails, the function returns without going through the
centralized cleanup path. Reported by Coverity - CID 1163870.
Signed-off-by: Christian Engelmayer cenge...@gmx.at
Acked.
Tim
---
crypto/tcrypt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1856d7f..1849155 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -313,7 +313,7 @@ static void test_aead_speed(const char *algo, int enc,
unsigned int sec,
if (IS_ERR(tfm)) {
pr_err(alg: aead: Failed to load transform for %s: %ld\n,
algo,
PTR_ERR(tfm));
- return;
+ goto out_notfm;
}
req = aead_request_alloc(tfm, GFP_KERNEL);
@@ -391,6 +391,7 @@ static void test_aead_speed(const char *algo, int enc,
unsigned int sec,
out:
crypto_free_aead(tfm);
+out_notfm:
kfree(sg);
out_nosg:
testmgr_free_buf(xoutbuf);
--
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: Fix leak of struct aead_request in test_aead_speed()
On Mon, 2014-04-21 at 20:47 +0200, Christian Engelmayer wrote:
Fix leakage of memory for struct aead_request that is allocated via
aead_request_alloc() but not released via aead_request_free().
Reported by Coverity - CID 1163869.
Signed-off-by: Christian Engelmayer cenge...@gmx.at
Acked. Thanks for fixing the leak.
Tim
---
crypto/tcrypt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1849155..09c93ff2 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -320,7 +320,7 @@ static void test_aead_speed(const char *algo, int enc,
unsigned int sec,
if (!req) {
pr_err(alg: aead: Failed to allocate request for %s\n,
algo);
- goto out;
+ goto out_noreq;
}
i = 0;
@@ -390,6 +390,8 @@ static void test_aead_speed(const char *algo, int enc,
unsigned int sec,
} while (*keysize);
out:
+ aead_request_free(req);
+out_noreq:
crypto_free_aead(tfm);
out_notfm:
kfree(sg);
--
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 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big
On Wed, 23 Apr 2014 01:33:05 +0200, Marek Vasut ma...@denx.de wrote:
On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
+ if (aad_size = PAGE_SIZE) {
On an unrelated note ... Won't if (aad_size PAGE_SIZE) be sufficient here?
From what I have seen how the buffers are allocated via __get_free_page() I
thought so too. However, as it previously read
if (aad_size PAGE_SIZE)
memset(assoc, 0xff, aad_size);
else {
my intention was simply to make the modification so that the bug is addressed
without introducing an additional change.
Regards,
Christian
signature.asc
Description: PGP signature
Re: [PATCH 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big
On Wednesday, April 23, 2014 at 07:43:35 PM, Christian Engelmayer wrote:
On Wed, 23 Apr 2014 01:33:05 +0200, Marek Vasut ma...@denx.de wrote:
On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
+ if (aad_size = PAGE_SIZE) {
On an unrelated note ... Won't if (aad_size PAGE_SIZE) be sufficient
here?
From what I have seen how the buffers are allocated via __get_free_page() I
thought so too. However, as it previously read
if (aad_size PAGE_SIZE)
memset(assoc, 0xff, aad_size);
else {
my intention was simply to make the modification so that the bug is
addressed without introducing an additional change.
I fully agree with you. I was just curious about the comparison here.
Best regards,
Marek Vasut
--
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: Parity Error on keys used for DES crypto test
On Wed, 23 Apr 2014 10:20:16 +0200
leroy christophe christophe.le...@c-s.fr wrote:
I'm altering the Freescale Talitos Driver in order to support the SEC1
security engine, and I have a big issue with the DES test vectors in
testmgr.h:
The Sec Engine reports key parity error.
Looking at the keys defined in testmgr.h for DES3, it looks like there
is a real parity issue with the test vectors. A DES key is supposed to
have all bytes with an odd number of ones. It is not the case in the key
below. At least the second byte 0xC0 has an even number of ones.
static struct cipher_testvec des3_ede_cbc_enc_tv_template[] = {
{ /* Generated from openssl */
.key= \xE9\xC0\xFF\x2E\x76\x0B\x64\x24
\x44\x4D\x99\x5A\x12\xD6\x40\xC0
\xEA\xC2\x84\xE8\x14\x95\xDB\xE8,
So, how can this test vector work ?
I'm not going to comment on the validity of the test key vector
other than to say that you can turn off key parity errors in the
SEC1 in the DEU Interrupt Control Register.
Kim
--
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
