Re: crypto: algif_aead - Switch to new AEAD interface
Am Mittwoch, 27. Mai 2015, 17:24:41 schrieb Herbert Xu:
Hi Herbert,
after testing of the new algif_aead interface, I am wondering about the
following changes which seem to alter the way how the tag is supposed to be
handled:
- return (ctx-used = (ctx-aead_assoclen + (ctx-enc ? 0 : as)));
+ return ctx-used = ctx-aead_assoclen + as;
This change requires that the buffer handed in by user space always has room
for the tag, regardless whether it is needed or not. Is that intended?
- /* add the size needed for the auth tag to be created */
- outlen += as;
- } else {
- /* output data size is input without the authentication tag */
- outlen = used - as;
The removal of these make me wonder: with those missing, the output of the
cipher operation does not have CT || tag (in case of encryption) or PT (in
case of encryption.
Note, I have updated my user space code to require space for the AD in the
output buffer. When reverting those changes with the following patch, the code
works nicely. If I do not apply the patch, the beginning of the CT or PT is as
expected, but the end is bogus. Also, the tag would be missing.
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 38a6cab..b6af158 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -72,7 +72,7 @@ static inline bool aead_sufficient_data(struct aead_ctx
*ctx)
{
unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(ctx-
aead_req));
- return ctx-used = ctx-aead_assoclen + as;
+ return ctx-used = ctx-aead_assoclen + (ctx-enc ? 0 : as);
}
static void aead_put_sgl(struct sock *sk)
@@ -403,13 +403,19 @@ static int aead_recvmsg(struct socket *sock, struct
msghdr *msg, size_t ignored,
if (!aead_sufficient_data(ctx))
goto unlock;
- outlen = used;
+ if (ctx-enc) {
+ /* add the size needed for the auth tag to be created */
+ outlen = used + as;
+ } else {
+ /* output data size is input without the authentication tag */
+ outlen = used - as;
+ }
/*
* The cipher operation input data is reduced by the associated data
* length as this data is processed separately later on.
*/
- used -= ctx-aead_assoclen + (ctx-enc ? as : 0);
+ used -= ctx-aead_assoclen;
/* convert iovecs of output buffers into scatterlists */
while (iov_iter_count(msg-msg_iter)) {
However, when use those changes and I perform the test of libkcapi/test/kcapi
-y -s, I get the following strange crash which i have no idea where to look
for the cause (normal sendmsg and vmsplice tests with libkcapi/test/kcapi -y
and libkcapi/test/kcapi -y -v work flawless)
[ 177.112195] Modules linked in: crypto_user ccm algif_aead(E) af_alg
nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT
nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4
nf_defrag_ipv4 xt_conntrack nf_conntrack cfg80211 ebtable_nat ebtable_broute
bridge stp llc ebtable_filter ebtables ip6table_mangle ip6table_security
ip6table_raw ip6table_filter ip6_tables iptable_mangle iptable_security
iptable_raw crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel
aesni_intel aes_x86_64 glue_helper ablk_helper microcode joydev pcspkr
serio_raw virtio_balloon i2c_piix4 acpi_cpufreq qxl virtio_blk virtio_net
drm_kms_helper ttm drm virtio_pci virtio_ring virtio [last unloaded:
algif_aead]
[ 177.112306] CPU: 1 PID: 2012 Comm: kcapi Tainted: GE 4.0.0+
#228
[ 177.112312] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.7.5-20140709_153950- 04/01/2014
[ 177.112319] task: 88007aaa3300 ti: 88007c0a4000 task.ti:
88007c0a4000
[ 177.112324] RIP: 0010:[8118fb6a] [8118fb6a]
ksize+0x4a/0xf0
[ 177.112337] RSP: 0018:88007c0a7d98 EFLAGS: 00010286
[ 177.112344] RAX: 0188000680c0 RBX: eb88000680c0 RCX:
[ 177.112350] RDX: 0010 RSI: ea0001a033c2 RDI:
77ff8000
[ 177.112356] RBP: 88007c0a7da8 R08: ea0001efa2e0 R09:
0007
[ 177.112361] R10: 880079419bb0 R11: 88007aac8b10 R12:
0010
[ 177.112367] R13: 0010 R14: 88007d0bc920 R15:
8800796acc00
[ 177.112375] FS: 7f2e2fd8a700() GS:88007fd0()
knlGS:
[ 177.112381] CS: 0010 DS: ES: CR0: 80050033
[ 177.112386] CR2: eb88000680c0 CR3: 7c044000 CR4:
000407e0
[ 177.112402] Stack:
[ 177.112407] 88007c0a7db8 ea0001a033c2 88007c0a7dc8
811636bc
[ 177.112418] 88007c0a7de8 88007c278800 88007c0a7de8
81563ddf
[ 177.112428] 88007c278800 88007a404000 88007c0a7e18
a028f694
[ 177.112438] Call Trace:
[ 177.112452] [811636bc] kzfree+0x1c/0x40
[ 177.112478]
Re: crypto: algif_aead - Switch to new AEAD interface
On Fri, May 29, 2015 at 12:09:35PM +0200, Stephan Mueller wrote:
- return (ctx-used = (ctx-aead_assoclen + (ctx-enc ? 0 : as)));
+ return ctx-used = ctx-aead_assoclen + as;
This change requires that the buffer handed in by user space always has room
for the tag, regardless whether it is needed or not. Is that intended?
Yes for two reasons. One is that sometimes we need to enforce
in-place processing, in which case dst must be at least as big
as src. The other reason is to eventually allow in-place processing
through algif_aead. Unless the two SG lists were of the same length,
it isn't possible to do that.
- /* add the size needed for the auth tag to be created */
- outlen += as;
- } else {
- /* output data size is input without the authentication tag */
- outlen = used - as;
The removal of these make me wonder: with those missing, the output of the
cipher operation does not have CT || tag (in case of encryption) or PT (in
case of encryption.
Note, I have updated my user space code to require space for the AD in the
output buffer. When reverting those changes with the following patch, the
code
works nicely. If I do not apply the patch, the beginning of the CT or PT is
as
expected, but the end is bogus. Also, the tag would be missing.
Well used is now supposed to always contain the tag in both cases.
Can you send me the code that you're using to test this and I'll
do some tests on it.
However, when use those changes and I perform the test of libkcapi/test/kcapi
-y -s, I get the following strange crash which i have no idea where to look
for the cause (normal sendmsg and vmsplice tests with libkcapi/test/kcapi -y
and libkcapi/test/kcapi -y -v work flawless)
This is clearly not good. It looks like memory corruption.
Thanks,
--
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
Re: crypto: algif_aead - Switch to new AEAD interface
Am Freitag, 29. Mai 2015, 21:28:40 schrieb Herbert Xu: Hi Herbert, On Fri, May 29, 2015 at 12:09:35PM +0200, Stephan Mueller wrote: - return (ctx-used = (ctx-aead_assoclen + (ctx-enc ? 0 : as))); + return ctx-used = ctx-aead_assoclen + as; This change requires that the buffer handed in by user space always has room for the tag, regardless whether it is needed or not. Is that intended? Yes for two reasons. One is that sometimes we need to enforce in-place processing, in which case dst must be at least as big as src. The other reason is to eventually allow in-place processing through algif_aead. Unless the two SG lists were of the same length, it isn't possible to do that. Do we really need to copy in and copy out unneeded data? That sounds very inefficient. Besides, can't we leave it to user space to build the right memory structure? I.e. if user space wants in-place operation, it needs to ensure that the one buffer is sufficient for the requested operation (i.e. that the requirements for src lengths and dst lengths are covered). However, when use those changes and I perform the test of libkcapi/test/kcapi -y -s, I get the following strange crash which i have no idea where to look for the cause (normal sendmsg and vmsplice tests with libkcapi/test/kcapi -y and libkcapi/test/kcapi -y -v work flawless) This is clearly not good. It looks like memory corruption. It seems it is triggered by my change suggestions. It is triggered in the while() loop in the recvmsg when allocating an output buffer larger than 16 pages. So, this is nothing in the current upstream code. Ciao Stephan -- 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: crypto: algif_aead - Switch to new AEAD interface
Am Freitag, 29. Mai 2015, 16:50:50 schrieb Stephan Mueller: Hi Herbert, Do we really need to copy in and copy out unneeded data? That sounds very inefficient. Besides, can't we leave it to user space to build the right memory structure? I.e. if user space wants in-place operation, it needs to ensure that the one buffer is sufficient for the requested operation (i.e. that the requirements for src lengths and dst lengths are covered). Please disregard my comment for the memory structure -- using IOVECs, userspace is free to format the memory layout as necessary. PS: I tested all code paths that I see for the new algif_aead and it works with the code currently in your tree. Please disregard the discussion around code changes. -- Ciao Stephan -- 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: crypto: algif_aead - Switch to new AEAD interface
On Wed, May 27, 2015 at 12:10:03PM +0200, Stephan Mueller wrote:
-
-if (ctx-enc) {
-/* round up output buffer to multiple of block size */
-outlen = ((used + bs - 1) / bs * bs);
Why wouldn't the round up for the output not be needed any more? If the
caller
provides input data that is not multiple of block sizes and the output buffer
is also not multiple of block sizes, wouldn't an encrypt overstep boundaries?
No the AEAD algorithm should fail them instead. We do the same
thing in algif_skcipher where it's up to the underlying algorithm
to fail requests that do not contain full blocks.
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
crypto: algif_aead - Switch to new AEAD interface
This patch makes use of the new AEAD interface which uses a single
SG list instead of separate lists for the AD and plain text.
Note that the user-space interface now requires both input and
output to be of the same length, and both must include space for
the AD as well as the authentication tag.
Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 53702e9..72a94dc 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -72,7 +72,7 @@ static inline bool aead_sufficient_data(struct aead_ctx *ctx)
{
unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(ctx-aead_req));
- return (ctx-used = (ctx-aead_assoclen + (ctx-enc ? 0 : as)));
+ return ctx-used = ctx-aead_assoclen + as;
}
static void aead_put_sgl(struct sock *sk)
@@ -353,12 +353,8 @@ static int aead_recvmsg(struct socket *sock, struct msghdr
*msg, size_t ignored,
struct sock *sk = sock-sk;
struct alg_sock *ask = alg_sk(sk);
struct aead_ctx *ctx = ask-private;
- unsigned bs = crypto_aead_blocksize(crypto_aead_reqtfm(ctx-aead_req));
unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(ctx-aead_req));
struct aead_sg_list *sgl = ctx-tsgl;
- struct scatterlist *sg = NULL;
- struct scatterlist assoc[ALG_MAX_PAGES];
- size_t assoclen = 0;
unsigned int i = 0;
int err = -EINVAL;
unsigned long used = 0;
@@ -407,23 +403,13 @@ static int aead_recvmsg(struct socket *sock, struct
msghdr *msg, size_t ignored,
if (!aead_sufficient_data(ctx))
goto unlock;
+ outlen = used;
+
/*
* The cipher operation input data is reduced by the associated data
* length as this data is processed separately later on.
*/
- used -= ctx-aead_assoclen;
-
- if (ctx-enc) {
- /* round up output buffer to multiple of block size */
- outlen = ((used + bs - 1) / bs * bs);
- /* add the size needed for the auth tag to be created */
- outlen += as;
- } else {
- /* output data size is input without the authentication tag */
- outlen = used - as;
- /* round up output buffer to multiple of block size */
- outlen = ((outlen + bs - 1) / bs * bs);
- }
+ used -= ctx-aead_assoclen + (ctx-enc ? as : 0);
/* convert iovecs of output buffers into scatterlists */
while (iov_iter_count(msg-msg_iter)) {
@@ -453,47 +439,11 @@ static int aead_recvmsg(struct socket *sock, struct
msghdr *msg, size_t ignored,
if (usedpages outlen)
goto unlock;
- sg_init_table(assoc, ALG_MAX_PAGES);
- assoclen = ctx-aead_assoclen;
- /*
-* Split scatterlist into two: first part becomes AD, second part
-* is plaintext / ciphertext. The first part is assigned to assoc
-* scatterlist. When this loop finishes, sg points to the start of the
-* plaintext / ciphertext.
-*/
- for (i = 0; i ctx-tsgl.cur; i++) {
- sg = sgl-sg + i;
- if (sg-length = assoclen) {
- /* AD is larger than one page */
- sg_set_page(assoc + i, sg_page(sg),
- sg-length, sg-offset);
- assoclen -= sg-length;
- if (i = ctx-tsgl.cur)
- goto unlock;
- } else if (!assoclen) {
- /* current page is to start of plaintext / ciphertext */
- if (i)
- /* AD terminates at page boundary */
- sg_mark_end(assoc + i - 1);
- else
- /* AD size is zero */
- sg_mark_end(assoc);
- break;
- } else {
- /* AD does not terminate at page boundary */
- sg_set_page(assoc + i, sg_page(sg),
- assoclen, sg-offset);
- sg_mark_end(assoc + i);
- /* plaintext / ciphertext starts after AD */
- sg-length -= assoclen;
- sg-offset += assoclen;
- break;
- }
- }
+ sg_mark_end(sgl-sg + sgl-cur - 1);
- aead_request_set_assoc(ctx-aead_req, assoc, ctx-aead_assoclen);
- aead_request_set_crypt(ctx-aead_req, sg, ctx-rsgl[0].sg, used,
- ctx-iv);
+ aead_request_set_crypt(ctx-aead_req, sgl-sg, ctx-rsgl[0].sg,
+ used, ctx-iv);
+ aead_request_set_ad(ctx-aead_req, ctx-aead_assoclen);
err = af_alg_wait_for_completion(ctx-enc ?
crypto_aead_encrypt(ctx-aead_req) :
--
Email: Herbert
Re: crypto: algif_aead - Switch to new AEAD interface
Am Mittwoch, 27. Mai 2015, 17:24:41 schrieb Herbert Xu:
Hi Herbert,
-
- if (ctx-enc) {
- /* round up output buffer to multiple of block size */
- outlen = ((used + bs - 1) / bs * bs);
Why wouldn't the round up for the output not be needed any more? If the caller
provides input data that is not multiple of block sizes and the output buffer
is also not multiple of block sizes, wouldn't an encrypt overstep boundaries?
- /* add the size needed for the auth tag to be created */
- outlen += as;
- } else {
- /* output data size is input without the authentication tag */
- outlen = used - as;
- /* round up output buffer to multiple of block size */
- outlen = ((outlen + bs - 1) / bs * bs);
Same here.
- }
+ used -= ctx-aead_assoclen + (ctx-enc ? as : 0);
Ciao
Stephan
--
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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
On Mon, May 25, 2015 at 07:53:41PM +0800, Herbert Xu wrote: On Mon, May 25, 2015 at 01:50:55PM +0200, Stephan Mueller wrote: When you have my code local, simply execute libkcapi/test/kcapi -y twice or three times. That triggered the crash. Aha that's what I was missing. I'll look into the crash. OK I forgot to initialise the SG list. This patch fixes it for me. diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c index 1d08483..35556a6 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -454,6 +454,7 @@ static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored, if (usedpages outlen) goto unlock; + sg_init_table(dstbuf, ALG_MAX_PAGES + 1); sg_mark_end(sgl-sg + sgl-cur); assoclen = ctx-aead_assoclen; /* -- 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
Re: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Dienstag, 26. Mai 2015, 14:24:33 schrieb Herbert Xu: Hi Herbert, On Mon, May 25, 2015 at 07:53:41PM +0800, Herbert Xu wrote: On Mon, May 25, 2015 at 01:50:55PM +0200, Stephan Mueller wrote: When you have my code local, simply execute libkcapi/test/kcapi -y twice or three times. That triggered the crash. Aha that's what I was missing. I'll look into the crash. OK I forgot to initialise the SG list. This patch fixes it for me. Confirmed. I see no more issues on the AF_ALG side. -- Ciao Stephan -- 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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
On Tue, May 26, 2015 at 08:44:29AM +0200, Stephan Mueller wrote: Confirmed. I see no more issues on the AF_ALG side. OK it works but there is a more serious issue. In particular, the user-space interface only provides spaces for the AD in the source and not in the destination. That means if we need to modify the AD (e.g., to rearrange headers for IPsec) then we'd have to copy it. So what I'd like to do is to make the recvmsg also provide space for the AD. That way we can always copy the AD into that space and modify it if necessary. In order to allow this to be done I'm going to disable the AEAD user-space interface in 4.1 so that we have time to fix it properly for 4.2. 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
Re: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Dienstag, 26. Mai 2015, 15:36:05 schrieb Herbert Xu: Hi Herbert, In order to allow this to be done I'm going to disable the AEAD user-space interface in 4.1 so that we have time to fix it properly for 4.2. Ok. Would you look into that one or shall I do that? Ciao Stephan -- 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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
On Tue, May 26, 2015 at 09:57:51AM +0200, Stephan Mueller wrote: Ok. Would you look into that one or shall I do that? I'll reenable it immediately after the patch to convert it to the new interface is merged. 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
Re: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
On Mon, May 25, 2015 at 01:50:55PM +0200, Stephan Mueller wrote: When you have my code local, simply execute libkcapi/test/kcapi -y twice or three times. That triggered the crash. Aha that's what I was missing. I'll look into the crash. Thanks, -- 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
Re: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Montag, 25. Mai 2015, 18:20:21 schrieb Herbert Xu: Hi Herbert, On Sun, May 24, 2015 at 12:52:02PM +0200, Stephan Mueller wrote: [ 29.653113] BUG: unable to handle kernel NULL pointer dereference at 000c Weird. I tried running your test but it appears to pass. The only failures were the nonsense strings and everything else says pased. To simply verify that all passes is to check for the return code: the return code tells you the number of failures --- the value of 0 indicates that all pass. And I see a simple test problem: I added a debug return that I forgot to remove in the test.sh. Thus, the large test is not executed with test.sh. When you have my code local, simply execute libkcapi/test/kcapi -y twice or three times. That triggered the crash. It certainly didn't crash for me. Considering that I just killed cryptoff in my local tree, it is entirely possible that the patches that you are running are no longer the same as mine. So let me merge the cryptoff patches and then I'll repost the algif_aead patch and ask you to retest. Thanks, -- Ciao Stephan -- 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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Sonntag, 24. Mai 2015, 11:34:20 schrieb Herbert Xu: Hi Herbert, On Sat, May 23, 2015 at 08:04:19PM +0200, Stephan Mueller wrote: Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu: Hi Herbert, This patch makes use of the new AEAD interface which uses a single SG list instead of separate lists for the AD and plain text. After applying your additional patch, the normal AEAD operation works. But with long messages (16 filled pages), I get the following. To test, simply use [1], cd libkcapi/test, compile and execute ./kcapi -y Thanks for testing! Does this patch help? Yes and no. Executing the test with 16 pages once passes. Executing it again (same test, same vectors) causes: [ 29.653113] BUG: unable to handle kernel NULL pointer dereference at 000c [ 29.653118] IP: [812b6d78] scatterwalk_ffwd+0x28/0xd0 [ 29.653123] PGD 7b775067 PUD 7b699067 PMD 0 [ 29.653125] Oops: [#1] SMP [ 29.653128] Modules linked in: crypto_user ccm algif_aead af_alg nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack cfg80211 ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_mangle iptable_security iptable_raw crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 glue_helper ablk_helper virtio_balloon microcode joydev pcspkr serio_raw i2c_piix4 acpi_cpufreq virtio_net virtio_blk qxl drm_kms_helper ttm drm virtio_pci virtio_ring virtio [ 29.653151] CPU: 1 PID: 1759 Comm: kcapi Not tainted 4.0.0+ #220 [ 29.653153] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140709_153950- 04/01/2014 [ 29.653154] task: 88007b798880 ti: 88007a434000 task.ti: 88007a434000 [ 29.653156] RIP: 0010:[812b6d78] [812b6d78] scatterwalk_ffwd+0x28/0xd0 [ 29.653158] RSP: 0018:88007a437a98 EFLAGS: 00010202 [ 29.653160] RAX: RBX: 6fe0 RCX: ea0001eef580 [ 29.653161] RDX: 1000 RSI: 88007a437b38 RDI: 88007a437c18 [ 29.653162] RBP: 88007a437aa8 R08: R09: 88007a437cf8 [ 29.653163] R10: R11: R12: 88007b1fed90 [ 29.653164] R13: 88007c0d7ac0 R14: 88007b1fed50 R15: 88007b1fc000 [ 29.653165] FS: 7fb3d7ace700() GS:88007fd0() knlGS: [ 29.653167] CS: 0010 DS: ES: CR0: 80050033 [ 29.653168] CR2: 000c CR3: 7b779000 CR4: 000407e0 [ 29.653171] Stack: [ 29.653172] 88007b1fecf0 a02a0380 88007a437ad8 812b7b40 [ 29.653175] 88007b1fecb0 88007a437cf8 8800798bf400 [ 29.653177] 88007a437ae8 812b7c0d 88007a437d88 a029a246 [ 29.653179] Call Trace: [ 29.653182] [a02a0380] ? crypto_ccm_decrypt+0x350/0x350 [ccm] [ 29.653185] [812b7b40] old_crypt+0x50/0xe0 [ 29.653187] [812b7c0d] old_encrypt+0x1d/0x20 [ 29.653189] [a029a246] aead_recvmsg+0x6f6/0x860 [algif_aead] [ 29.653192] [8114a672] ? __alloc_pages_nodemask+0x1a2/0x9d0 [ 29.653195] [81687b7a] ? _raw_spin_unlock_bh+0x1a/0x20 [ 29.653197] [a0299849] ? aead_sendmsg+0x429/0x4c0 [algif_aead] [ 29.653201] [81561528] sock_recvmsg+0x38/0x50 [ 29.653203] [815615c8] sock_read_iter+0x88/0xd0 [ 29.653205] [811a9990] __vfs_read+0x90/0xc0 [ 29.653207] [811aa12a] vfs_read+0x8a/0x140 [ 29.653209] [811aab56] SyS_read+0x46/0xb0 [ 29.653210] [8168812e] system_call_fastpath+0x12/0x71 [ 29.653211] Code: 0f 1f 00 66 66 66 66 90 55 85 d2 48 89 f0 48 89 e5 41 54 53 89 d3 74 28 8b 56 0c 49 89 fc 39 d3 73 10 eb 27 0f 1f 80 00 00 00 00 8b 50 0c 39 da 77 19 29 d3 48 89 c7 e8 87 a9 05 00 85 db 75 eb [ 29.653233] RIP [812b6d78] scatterwalk_ffwd+0x28/0xd0 [ 29.653235] RSP 88007a437a98 [ 29.653236] CR2: 000c [ 29.653238] ---[ end trace b579ecce490b2e88 ]--- -- Ciao Stephan -- 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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Samstag, 23. Mai 2015, 05:58:18 schrieb Herbert Xu: Hi Herbert, On Fri, May 22, 2015 at 11:04:39PM +0200, Stephan Mueller wrote: Note, gcm(aes) looks good. Only rfc4106(gcm(aes)) causes the crash. Actually it looks like the culprit hasn't been merged yet so I'll just respin the series. Anyway, this patch should fix your crash: That fixes the rfc4106(gcm(aes)) issue. Thanks. -- Ciao Stephan -- 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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu: Hi Herbert, This patch makes use of the new AEAD interface which uses a single SG list instead of separate lists for the AD and plain text. After applying your additional patch, the normal AEAD operation works. But with long messages (16 filled pages), I get the following. To test, simply use [1], cd libkcapi/test, compile and execute ./kcapi -y [ 59.441841] BUG: unable to handle kernel NULL pointer dereference at 000c [ 59.441853] IP: [812b6d78] scatterwalk_ffwd+0x28/0xd0 [ 59.441866] PGD 78ad6067 PUD 799f5067 PMD 0 [ 59.441874] Oops: [#1] SMP [ 59.441880] Modules linked in: ansi_cprng drbg algif_rng ccm gcm algif_aead algif_skcipher sha512_ssse3 sha512_generic mcryptd sha1_ssse3 sha1_generic crypto_user des3_ede_x86_64 des_generic cmac algif_hash af_alg nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack cfg80211 ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_mangle iptable_security iptable_raw crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 glue_helper ablk_helper microcode joydev pcspkr serio_raw virtio_balloon i2c_piix4 acpi_cpufreq virtio_net qxl virtio_blk drm_kms_helper [ 59.441958] ttm drm virtio_pci virtio_ring virtio [ 59.441970] CPU: 1 PID: 2338 Comm: kcapi Not tainted 4.0.0+ #220 [ 59.441975] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140709_153950- 04/01/2014 [ 59.441995] task: 88007aa1e600 ti: 880035998000 task.ti: 880035998000 [ 59.441999] RIP: 0010:[812b6d78] [812b6d78] scatterwalk_ffwd+0x28/0xd0 [ 59.442007] RSP: 0018:88003599ba98 EFLAGS: 00010202 [ 59.442007] RAX: RBX: 6fe0 RCX: ea0001eaa500 [ 59.442007] RDX: 1000 RSI: 88003599bb38 RDI: 88003599bc18 [ 59.442007] RBP: 88003599baa8 R08: 88003599bcf8 R09: [ 59.442007] R10: R11: 1000 R12: 88007b802d90 [ 59.442007] R13: 88007b3f3c40 R14: 88007b802d50 R15: 88007b80 [ 59.442007] FS: 7f6cf9da6700() GS:88007fd0() knlGS: [ 59.442007] CS: 0010 DS: ES: CR0: 80050033 [ 59.442007] CR2: 000c CR3: 799a6000 CR4: 000407e0 [ 59.442007] Stack: [ 59.442007] 88007b802cf0 a02f6380 88003599bad8 812b7b40 [ 59.442007] 88007b802cb0 88007b88 88007aa04000 [ 59.442007] 88003599bae8 812b7c0d 88003599bd88 a02e5252 [ 59.442007] Call Trace: [ 59.442007] [a02f6380] ? crypto_ccm_decrypt+0x350/0x350 [ccm] [ 59.442007] [812b7b40] old_crypt+0x50/0xe0 [ 59.442007] [812b7c0d] old_encrypt+0x1d/0x20 [ 59.442007] [a02e5252] aead_recvmsg+0x702/0x862 [algif_aead] [ 59.442007] [8114a672] ? __alloc_pages_nodemask+0x1a2/0x9d0 [ 59.442007] [81687b7a] ? _raw_spin_unlock_bh+0x1a/0x20 [ 59.442007] [a02e4849] ? aead_sendmsg+0x429/0x4c0 [algif_aead] [ 59.442007] [81561528] sock_recvmsg+0x38/0x50 [ 59.442007] [815615c8] sock_read_iter+0x88/0xd0 [ 59.442007] [811a9990] __vfs_read+0x90/0xc0 [ 59.442007] [811aa12a] vfs_read+0x8a/0x140 [ 59.442007] [811aab56] SyS_read+0x46/0xb0 [ 59.442007] [8168812e] system_call_fastpath+0x12/0x71 [ 59.442007] Code: 0f 1f 00 66 66 66 66 90 55 85 d2 48 89 f0 48 89 e5 41 54 53 89 d3 74 28 8b 56 0c 49 89 fc 39 d3 73 10 eb 27 0f 1f 80 00 00 00 00 8b 50 0c 39 da 77 19 29 d3 48 89 c7 e8 87 a9 05 00 85 db 75 eb [ 59.442007] RIP [812b6d78] scatterwalk_ffwd+0x28/0xd0 [ 59.442007] RSP 88003599ba98 [ 59.442007] CR2: 000c [ 59.442368] ---[ end trace 09389ca31f370515 ]--- -- Ciao Stephan -- 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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
On Sat, May 23, 2015 at 08:04:19PM +0200, Stephan Mueller wrote: Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu: Hi Herbert, This patch makes use of the new AEAD interface which uses a single SG list instead of separate lists for the AD and plain text. After applying your additional patch, the normal AEAD operation works. But with long messages (16 filled pages), I get the following. To test, simply use [1], cd libkcapi/test, compile and execute ./kcapi -y Thanks for testing! Does this patch help? diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c index a483a6f..1d08483 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -494,11 +494,11 @@ static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored, else if (outlen) /* AD size is non-zero */ scatterwalk_crypto_chain( - dst, ctx-rsgl[0].sg, + dst + i - 1, ctx-rsgl[0].sg, sg_page(ctx-rsgl[0].sg) == sg_page(dst + i - 1) ctx-rsgl[0].sg[0].offset == dst[i - 1].offset + dst[i - 1].length, - i + 1); + 2); else /* AD only */ sg_mark_end(dst + i); 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
[v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
This patch makes use of the new AEAD interface which uses a single
SG list instead of separate lists for the AD and plain text.
Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
---
crypto/algif_aead.c | 61 ++--
1 file changed, 36 insertions(+), 25 deletions(-)
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 53702e9..5674a33 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -26,7 +26,7 @@
struct aead_sg_list {
unsigned int cur;
- struct scatterlist sg[ALG_MAX_PAGES];
+ struct scatterlist sg[ALG_MAX_PAGES + 1];
};
struct aead_ctx {
@@ -357,7 +357,8 @@ static int aead_recvmsg(struct socket *sock, struct msghdr
*msg, size_t ignored,
unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(ctx-aead_req));
struct aead_sg_list *sgl = ctx-tsgl;
struct scatterlist *sg = NULL;
- struct scatterlist assoc[ALG_MAX_PAGES];
+ struct scatterlist dstbuf[ALG_MAX_PAGES + 1];
+ struct scatterlist *dst = dstbuf;
size_t assoclen = 0;
unsigned int i = 0;
int err = -EINVAL;
@@ -453,7 +454,7 @@ static int aead_recvmsg(struct socket *sock, struct msghdr
*msg, size_t ignored,
if (usedpages outlen)
goto unlock;
- sg_init_table(assoc, ALG_MAX_PAGES);
+ sg_mark_end(sgl-sg + sgl-cur);
assoclen = ctx-aead_assoclen;
/*
* Split scatterlist into two: first part becomes AD, second part
@@ -465,35 +466,45 @@ static int aead_recvmsg(struct socket *sock, struct
msghdr *msg, size_t ignored,
sg = sgl-sg + i;
if (sg-length = assoclen) {
/* AD is larger than one page */
- sg_set_page(assoc + i, sg_page(sg),
+ sg_set_page(dst + i, sg_page(sg),
sg-length, sg-offset);
assoclen -= sg-length;
- if (i = ctx-tsgl.cur)
- goto unlock;
- } else if (!assoclen) {
- /* current page is to start of plaintext / ciphertext */
- if (i)
- /* AD terminates at page boundary */
- sg_mark_end(assoc + i - 1);
- else
- /* AD size is zero */
- sg_mark_end(assoc);
- break;
- } else {
+ continue;
+ }
+
+ if (assoclen) {
/* AD does not terminate at page boundary */
- sg_set_page(assoc + i, sg_page(sg),
+ sg_set_page(dst + i, sg_page(sg),
assoclen, sg-offset);
- sg_mark_end(assoc + i);
- /* plaintext / ciphertext starts after AD */
- sg-length -= assoclen;
- sg-offset += assoclen;
- break;
+ assoclen = 0;
+ i++;
}
+
+ break;
}
- aead_request_set_assoc(ctx-aead_req, assoc, ctx-aead_assoclen);
- aead_request_set_crypt(ctx-aead_req, sg, ctx-rsgl[0].sg, used,
- ctx-iv);
+ /* This should never happen because of aead_sufficient_data. */
+ if (WARN_ON_ONCE(assoclen))
+ goto unlock;
+
+ /* current page is the start of plaintext / ciphertext */
+ if (!i)
+ /* AD size is zero */
+ dst = ctx-rsgl[0].sg;
+ else if (outlen)
+ /* AD size is non-zero */
+ scatterwalk_crypto_chain(
+ dst, ctx-rsgl[0].sg,
+ sg_page(ctx-rsgl[0].sg) == sg_page(dst + i - 1)
+ ctx-rsgl[0].sg[0].offset == dst[i - 1].offset +
+dst[i - 1].length,
+ i + 1);
+ else
+ /* AD only */
+ sg_mark_end(dst + i);
+
+ aead_request_set_crypt(ctx-aead_req, sgl-sg, dst, used, ctx-iv);
+ aead_request_set_ad(ctx-aead_req, ctx-aead_assoclen, 0);
err = af_alg_wait_for_completion(ctx-enc ?
crypto_aead_encrypt(ctx-aead_req) :
--
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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
On Fri, May 22, 2015 at 11:04:39PM +0200, Stephan Mueller wrote:
Note, gcm(aes) looks good. Only rfc4106(gcm(aes)) causes the crash.
Actually it looks like the culprit hasn't been merged yet so I'll
just respin the series.
Anyway, this patch should fix your crash:
diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
index b3dded4..b15d797 100644
--- a/crypto/ablkcipher.c
+++ b/crypto/ablkcipher.c
@@ -586,6 +586,13 @@ static int crypto_givcipher_default(struct crypto_alg
*alg, u32 type, u32 mask)
if (!tmpl)
goto kill_larval;
+ if (tmpl-create) {
+ err = tmpl-create(tmpl, tb);
+ if (err)
+ goto put_tmpl;
+ goto ok;
+ }
+
inst = tmpl-alloc(tb);
err = PTR_ERR(inst);
if (IS_ERR(inst))
@@ -597,6 +604,7 @@ static int crypto_givcipher_default(struct crypto_alg *alg,
u32 type, u32 mask)
goto put_tmpl;
}
+ok:
/* Redo the lookup to use the instance we just registered. */
err = -EAGAIN;
diff --git a/crypto/aead.c b/crypto/aead.c
index 8b26613..070e4b9 100644
--- a/crypto/aead.c
+++ b/crypto/aead.c
@@ -570,6 +570,13 @@ static int crypto_nivaead_default(struct crypto_alg *alg,
u32 type, u32 mask)
if (!tmpl)
goto kill_larval;
+ if (tmpl-create) {
+ err = tmpl-create(tmpl, tb);
+ if (err)
+ goto put_tmpl;
+ goto ok;
+ }
+
inst = tmpl-alloc(tb);
err = PTR_ERR(inst);
if (IS_ERR(inst))
@@ -581,6 +588,7 @@ static int crypto_nivaead_default(struct crypto_alg *alg,
u32 type, u32 mask)
goto put_tmpl;
}
+ok:
/* Redo the lookup to use the instance we just registered. */
err = -EAGAIN;
--
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
Re: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu: Hi Herbert, This patch makes use of the new AEAD interface which uses a single SG list instead of separate lists for the AD and plain text. Using an up-to date tree with the full set of patches of this patch set, I get the following oops. It can easily be reproduced by using [1]: go to libkcapi/test/ and compile with make. Then execute ./test.sh [1] http://www.chronox.de/libkcapi.html [ 22.680910] BUG: unable to handle kernel NULL pointer dereference at (null) [ 22.680915] IP: [ (null)] (null) [ 22.680917] PGD 3c62e067 PUD 3b28e067 PMD 0 [ 22.680919] Oops: 0010 [#1] SMP [ 22.680921] Modules linked in: seqiv ccm gcm crypto_null algif_aead algif_skcipher sha512_ssse3 sha512_generic mcryptd sha1_ssse3 sha1_generic crypto_user des3_ede_x86_64 des_generic algif_hash af_alg nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack cfg80211 ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_mangle iptable_security iptable_raw crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 glue_helper ablk_helper joydev microcode virtio_console serio_raw virtio_balloon pcspkr i2c_piix4 acpi_cpufreq qxl drm_kms_helper ttm drm virtio_net [ 22.680948] virtio_blk virtio_pci virtio_ring virtio [ 22.680952] CPU: 1 PID: 1889 Comm: kcapi Not tainted 4.0.0+ #122 [ 22.680954] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 22.680955] task: 88003c08cc80 ti: 88003b30 task.ti: 88003b30 [ 22.680956] RIP: 0010:[] [ (null)] (null) [ 22.680958] RSP: 0018:88003b303ce0 EFLAGS: 00010282 [ 22.680959] RAX: a02f5080 RBX: a0138b20 RCX: 0001 [ 22.680960] RDX: 0001 RSI: a02f5368 RDI: 88003b303cf8 [ 22.680961] RBP: 88003b303d88 R08: R09: eaecbd00 [ 22.680962] R10: 810676b4 R11: 88003c275240 R12: 88003b1ff200 [ 22.680963] R13: fffe R14: a02f5080 R15: 0203 [ 22.680965] FS: 7fade1fe8700() GS:88003fd0() knlGS: [ 22.680966] CS: 0010 DS: ES: CR0: 80050033 [ 22.680967] CR2: CR3: 3bdc9000 CR4: 000407e0 [ 22.680971] Stack: [ 22.680973] 812b7e6d 0002000c0003 020f0203 88003b303cec [ 22.680975] 88003b303d14 00010044812b49c4 2d36303134636672 [ 22.680977] 6e7365612d6d6367 0069 [ 22.680979] Call Trace: [ 22.680984] [812b7e6d] ? crypto_nivaead_default+0x14d/0x1a0 [ 22.680986] [812b7f5a] crypto_lookup_aead+0x9a/0xf0 [ 22.680989] [812b4f33] crypto_alloc_tfm+0x63/0x130 [ 22.680992] [81193dd1] ? kmem_cache_alloc_trace+0x1f1/0x230 [ 22.680994] [812b7fc9] crypto_alloc_aead+0x19/0x20 [ 22.680996] [a02d638e] aead_bind+0xe/0x10 [algif_aead] [ 22.680999] [a02848d0] alg_bind+0x60/0x130 [af_alg] [ 22.681003] [81561f68] SYSC_bind+0xb8/0xf0 [ 22.681003] [811c7eb5] ? fd_install+0x25/0x30 [ 22.681003] [81562850] ? SyS_socket+0x90/0xd0 [ 22.681003] [8104a0f7] ? trace_do_page_fault+0x37/0xb0 [ 22.681003] [81562ade] SyS_bind+0xe/0x10 [ 22.681003] [81687f6e] system_call_fastpath+0x12/0x71 [ 22.681003] Code: Bad RIP value. [ 22.681003] RIP [ (null)] (null) [ 22.681003] RSP 88003b303ce0 [ 22.681003] CR2: [ 22.681053] ---[ end trace c1a8ba963ebab541 ]--- -- Ciao Stephan -- 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: [v2 PATCH 13/13] crypto: algif_aead - Switch to new AEAD interface
Am Freitag, 22. Mai 2015, 22:59:34 schrieb Stephan Mueller: Hi Stephan, Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu: Hi Herbert, This patch makes use of the new AEAD interface which uses a single SG list instead of separate lists for the AD and plain text. Using an up-to date tree with the full set of patches of this patch set, I get the following oops. It can easily be reproduced by using [1]: go to libkcapi/test/ and compile with make. Then execute ./test.sh [1] http://www.chronox.de/libkcapi.html Note, gcm(aes) looks good. Only rfc4106(gcm(aes)) causes the crash. -- Ciao Stephan -- 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
